oom killer (Core)

The oom killer has currently some strange effects when triggered. It gets invoked multiple times and the selection of the task to kill does not take processes into account which fork a lot of child processes.

The patch solves this by
- Preventing reentrancy
- Checking for memory threshold before selection and kill.
- Taking child processes into account when selecting the process to kill

---
 oom_kill.c   |  113 ++++++++++++++++++++++++++++++++++++++++++-----------------
 page_alloc.c |    5 ++
 2 files changed, 86 insertions(+), 32 deletions(-)

---

diff -urN 2.6.10-rc2-mm4.orig/mm/oom_kill.c 2.6.10-rc2-mm4/mm/oom_kill.c
@@ -45,8 +45,10 @@
 static unsigned long badness(struct task_struct *p, unsigned long uptime)
 {
 	unsigned long points, cpu_time, run_time, s;
+        struct list_head *tsk;
 
-	if (!p->mm)
+	/* Ignore mm-less tasks and init */
+	if (!p->mm || p->pid == 1)
 		return 0;
 
 	if (p->flags & PF_MEMDIE)

@@ -57,6 +59,19 @@
 	points = p->mm->total_vm;
 
 	/*
+	 * Processes which fork a lot of child processes are likely 
+	 * a good choice. We add the vmsize of the childs if they
+	 * have an own mm. This prevents forking servers to flood the
+	 * machine with an endless amount of childs
+	 */
+	list_for_each(tsk, &p->children) {
+		struct task_struct *chld;
+		chld = list_entry(tsk, struct task_struct, sibling);
+		if (chld->mm != p->mm && chld->mm)
+			points += chld->mm->total_vm;
+	}
+
+	/*
 	 * CPU time is in tens of seconds and run time is in thousands
          * of seconds. There is no particular reason for this other than
          * that it turned out to work very well in practice.

@@ -176,6 +191,27 @@
 	return mm;
 }
 
+static struct mm_struct *oom_kill_process(task_t *p)
+{
+	struct mm_struct *mm;
+	struct task_struct *g, *q;
+
+	mm = oom_kill_task(p);
+	if (!mm)
+		return NULL;
+	/*
+	 * kill all processes that share the ->mm (i.e. all threads),
+	 * but are in a different thread group
+	 */
+	do_each_thread(g, q)
+		if (q->mm == mm && q->tgid != p->tgid)
+			__oom_kill_task(q);
+
+	while_each_thread(g, q);
+	if (!p->mm)
+		printk(KERN_INFO "Fixed up OOM kill of mm-less task\n");
+	return mm;
+}
 
 /**
  * oom_kill - kill the "best" process when we run out of memory

@@ -188,7 +224,9 @@
 void oom_kill(void)
 {
 	struct mm_struct *mm;
-	struct task_struct *g, *p, *q;
+	struct task_struct *c, *p;
+	struct list_head *tsk;
+	int mmcnt = 0;
 	
 	read_lock(&tasklist_lock);
 retry:

@@ -200,21 +238,32 @@
 		panic("Out of memory and no killable processes...\n");
 	}
 
-	mm = oom_kill_task(p);
-	if (!mm)
-		goto retry;
 	/*
-	 * kill all processes that share the ->mm (i.e. all threads),
-	 * but are in a different thread group
+	 * Kill the forked child processes first
 	 */
-	do_each_thread(g, q)
-		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q);
-	while_each_thread(g, q);
-	if (!p->mm)
-		printk(KERN_INFO "Fixed up OOM kill of mm-less task\n");
+	list_for_each(tsk, &p->children) {
+		c = list_entry(tsk, struct task_struct, sibling);
+		/* Do not touch threads, as they get killed later */
+		if (c->mm == p->mm)
+			continue;
+		mm = oom_kill_process(c);
+		if (mm) {
+			mmcnt ++;
+			mmput(mm);
+		}
+	}
+
+	/*
+	 * If we managed to kill one or more child processes
+	 * then let the parent live for now
+	 */
+	if (!mmcnt) {
+		mm = oom_kill_process(p);
+		if (!mm)
+			goto retry;
+		mmput(mm);
+	}
 	read_unlock(&tasklist_lock);
-	mmput(mm);
 	return;
 }
 

@@ -224,14 +273,21 @@
 void out_of_memory(int gfp_mask)
 {
 	/*
-	 * oom_lock protects out_of_memory()'s static variables.
-	 * It's a global lock; this is not performance-critical.
-	 */
-	static DEFINE_SPINLOCK(oom_lock);
+ 	 * inprogress protects out_of_memory()'s static variables
+	 * and prevents reentrancy.
+  	 */
+ 	static unsigned long inprogress;
+ 	static unsigned int  freepages = 1000000;
 	static unsigned long first, last, count, lastkill;
 	unsigned long now, since;
 
-	spin_lock(&oom_lock);
+ 	if (test_and_set_bit(0, &inprogress))
+ 		return;
+ 	
+ 	/* Check, if memory was freed since the last oom kill */
+ 	if (freepages < nr_free_pages())
+ 		goto out_unlock;
+
 	now = jiffies;
 	since = now - last;
 	last = now;

@@ -271,12 +327,11 @@
 	 * Ok, really out of memory. Kill something.
 	 */
 	lastkill = now;
-
-	printk("oom-killer: gfp_mask=0x%x\n", gfp_mask);
+	printk(KERN_ERR "oom-killer: gfp_mask=0x%x\n", gfp_mask);
 	show_free_areas();
-
-	/* oom_kill() sleeps */
-	spin_unlock(&oom_lock);
+	dump_stack();
+	/* Store free pages  * 2 for the check above */
+	freepages = (nr_free_pages() << 1);
 	oom_kill();
 	/*
 	 * Make kswapd go out of the way, so "p" has a good chance of

@@ -284,17 +339,11 @@
 	 * for more memory.
 	 */
 	yield();
-	spin_lock(&oom_lock);
 
 reset:
-	/*
-	 * We dropped the lock above, so check to be sure the variable
-	 * first only ever increases to prevent false OOM's.
-	 */
-	if (time_after(now, first))
-		first = now;
+	first = jiffies;
 	count = 0;
 
 out_unlock:
-	spin_unlock(&oom_lock);
+	clear_bit(0, &inprogress);
 }

diff -urN 2.6.10-rc2-mm4.orig/mm/page_alloc.c 2.6.10-rc2-mm4/mm/page_alloc.c
@@ -872,6 +872,11 @@
 	p->reclaim_state = NULL;
 	p->flags &= ~PF_MEMALLOC;
 
+	/* Check if try_to_free_pages called out_of_memory and
+	 * if the current task is the sacrifice  */
+	if ((p->flags & PF_MEMDIE) && !in_interrupt())
+		goto nopage;
+
 	/* go through the zonelist yet one more time */
 	for (i = 0; (z = zones[i]) != NULL; i++) {
 		if (!zone_watermark_ok(z, order, z->pages_min,

You didn't move the invocation in page_alloc.c which is the major bug I can see (besides the other hacks in oom_kill.c). I'd try fixing the major bug first.

Where do you want to move it ?

I don't buy that moving the invocation to any place will solve the problem of multiple invocations.

The multiple invocations happen with SMP and UP + PREEMPT, when the lock is dropped in out_of_memory()

	spin_unlock(&oom_lock);
	oom_kill();
	spin_lock(&oom_lock);

How does moving the invocation change this ?

tglx

It has to check the levels of free memory before calling oom_kill.c and that's the usual check that alloc_pages does.

Could you please test this? Perhaps it won't be as effective as the hacks I nuked, but it fixed basic things for me in a quick test so I like it more. It's important to use pages_high to avoid livelocks.

Thanks.

Index: x/mm/oom_kill.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/oom_kill.c,v retrieving revision 1.31

diff -u -p -r1.31 oom_kill.c
@@ -229,72 +229,8 @@ retry:
  */
 void out_of_memory(int gfp_mask)
 {
-	/*
-	 * oom_lock protects out_of_memory()'s static variables.
-	 * It's a global lock; this is not performance-critical.
-	 */
-	static spinlock_t oom_lock = SPIN_LOCK_UNLOCKED;
-	static unsigned long first, last, count, lastkill;
-	unsigned long now, since;
-
-	spin_lock(&oom_lock);
-	now = jiffies;
-	since = now - last;
-	last = now;
-
-	/*
-	 * If it's been a long time since last failure,
-	 * we're not oom.
-	 */
-	if (since > 5*HZ)
-		goto reset;
-
-	/*
-	 * If we haven't tried for at least one second,
-	 * we're not really oom.
-	 */
-	since = now - first;
-	if (since < HZ)
-		goto out_unlock;
-
-	/*
-	 * If we have gotten only a few failures,
-	 * we're not really oom. 
-	 */
-	if (++count < 10)
-		goto out_unlock;
-
-	/*
-	 * If we just killed a process, wait a while
-	 * to give that task a chance to exit. This
-	 * avoids killing multiple processes needlessly.
-	 */
-	since = now - lastkill;
-	if (since < HZ*5)
-		goto out_unlock;
-
-	/*
-	 * Ok, really out of memory. Kill something.
-	 */
-	lastkill = now;
-
 	printk("oom-killer: gfp_mask=0x%x\n", gfp_mask);
 	show_free_areas();
-
-	/* oom_kill() sleeps */
-	spin_unlock(&oom_lock);
+	dump_stack();
 	oom_kill();
-	spin_lock(&oom_lock);
-
-reset:
-	/*
-	 * We dropped the lock above, so check to be sure the variable
-	 * first only ever increases to prevent false OOM's.
-	 */
-	if (time_after(now, first))
-		first = now;
-	count = 0;
-
-out_unlock:
-	spin_unlock(&oom_lock);
 }

Index: x/mm/page_alloc.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/page_alloc.c,v retrieving revision 1.236

diff -u -p -r1.236 page_alloc.c
@@ -611,6 +611,7 @@ __alloc_pages(unsigned int gfp_mask, uns
 	int alloc_type;
 	int do_retry;
 	int can_try_harder;
+	int did_some_progress;
 
 	might_sleep_if(wait);
 

@@ -686,18 +687,19 @@ rebalance:
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
-	try_to_free_pages(zones, gfp_mask, order);
+	did_some_progress = try_to_free_pages(zones, gfp_mask, order);
 
 	p->reclaim_state = NULL;
 	p->flags &= ~PF_MEMALLOC;
 
-	/* go through the zonelist yet one more time */
+	/*
+	 * Go through the zonelist yet one more time, keep
+	 * very high watermark here, this is only to catch
+	 * a parallel oom killing, we must fail if we're still
+	 * under heavy pressure.
+	 */
 	for (i = 0; (z = zones[i]) != NULL; i++) {
-		min = z->pages_min;
-		if (gfp_mask & __GFP_HIGH)
-			min /= 2;
-		if (can_try_harder)
-			min -= min / 4;
+		min = z->pages_high;
 		min += (1<<order) + z->protection[alloc_type];
 
 		if (z->free_pages < min)

@@ -708,6 +710,9 @@ rebalance:
 			goto got_pg;
 	}
 
+	if (unlikely(!did_some_progress) && (gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY))
+		out_of_memory(gfp_mask);
+
 	/*
 	 * Don't let big-order allocations loop unless the caller explicitly
 	 * requests that.  Wait for some write requests to complete then retry.

Index: x/mm/vmscan.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/vmscan.c,v retrieving revision 1.225

diff -u -p -r1.225 vmscan.c
@@ -935,8 +935,6 @@ int try_to_free_pages(struct zone **zone
 		if (sc.nr_scanned && priority < DEF_PRIORITY - 2)
 			blk_congestion_wait(WRITE, HZ/10);
 	}
-	if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY))
-		out_of_memory(gfp_mask);
 out:
 	for (i = 0; zones[i] != 0; i++)
 		zones[i]->prev_priority = zones[i]->temp_priority;

Sorry man. The behaviour is worse than before, but I expected that :)

It kills sshd, the application, the controlling terminal and some other innocent processes like hotplug.

Take a decent UP machine enable PREEMPT and start # hackbench NN , where NN is a number of processes, which exhausts the memory of the machine and see yourself.

If there is the point where memory is exhausted you have to kill a process _and_ wait until the memory is freed before even thinking about another invocation of oom_kill().

There is usually more than one process on a machine which requests memory. So as long as the first kill did not proceed to the point where the memory is actually freed, you will call out_of_memory() from different processes or kernel functions and make the machine less usable than neccecary.

The oom_kill is invoked about 100 times. It ends up killing hotplug when already 112MB memory are available. See below.

I agree, that the timed hack stuff in out_of_memory is ugly, but we need some mechanism to avoid

1. reentrancy into oom_kill
2. invocation of oom_kill until the already killed process has finally released memory.

Further we need a better selection of whom to kill.

tglx

<SNIP>
Free pages: 112360kB (0kB HighMem)
<SNIP>

 [<c0132151>] out_of_memory+0x21/0x30
 [<c0132f96>] __alloc_pages+0x2f6/0x380
 [<c013303f>] __get_free_pages+0x1f/0x40
 [<c013665f>] kmem_getpages+0x1f/0xd0
 [<c0137339>] cache_grow+0xb9/0x180
 [<c0137573>] cache_alloc_refill+0x173/0x220
 [<c01378d4>] __kmalloc+0x74/0x80
 [<c02534d7>] alloc_skb+0x47/0xf0
 [<c0252717>] sock_alloc_send_pskb+0xc7/0x1d0
 [<c025284d>] sock_alloc_send_skb+0x2d/0x40
 [<c02ad466>] unix_stream_sendmsg+0x196/0x400
 [<c012f987>] filemap_nopage+0x207/0x3a0
 [<c024fa66>] sock_aio_write+0xf6/0x120
 [<c014c967>] do_sync_write+0xb7/0xf0
 [<c016048b>] do_pollfd+0x5b/0xa0
 [<c0126f90>] autoremove_wake_function+0x0/0x60
 [<c016053a>] do_poll+0x6a/0xd0
 [<c014ca9f>] vfs_write+0xff/0x130
 [<c014cba1>] sys_write+0x51/0x80
 [<c01027cb>] syscall_call+0x7/0xb

Out of Memory: Killed process 3037 (hotplug).

Attached patch solves this problems more elegant than the previous attempts and removes the ugly time, count, threshold stuff.

Reentrancy and follow up calls of oom_kill() are blocked until the task which was killed by the first oom_kill() has actually released the resources. I added a callback which is called from do_exit() when the PF_MEMDIE flag is set. The reentrancy blocking is released inside the callback.

The whom to select modification is still neccecary to prevent that innocent processes are killed.

It now kills exactly one process and comes back to the terminal without further damages. Also other scenarios which I used to test the oom- killer are not showing any strange side effects.

blocking until the blocking is released in the callback, out_of_memory is called more than 300 times.

Interesting is that the patch applied to the current code in 2.6.10-rc2- mm4, where the call to out_of_memory is still in vmscan.c, has the same effect.

So it's up to you VM guys to fight out from which place you want call out_of_memory(). I don't care as both places have exactly the same bad side effects.

My concern is to make it reliable when it is finally invoked.

tglx

diff -urN 2.6.10-rc2.orig/include/linux/swap.h 2.6.10-rc2.oom/include/linux/swap.h
@@ -149,6 +149,7 @@
 
 /* linux/mm/oom_kill.c */
 extern void out_of_memory(int gfp_mask);
+extern void oom_task_killed(void);
 
 /* linux/mm/memory.c */
 extern void swapin_readahead(swp_entry_t, unsigned long, struct vm_area_struct *);

diff -urN 2.6.10-rc2.orig/kernel/exit.c 2.6.10-rc2.oom/kernel/exit.c
@@ -820,6 +820,9 @@
 	exit_thread();
 	exit_keys(tsk);
 
+	if (current->flags & PF_MEMDIE)
+		oom_task_killed();
+
 	if (group_dead && tsk->signal->leader)
 		disassociate_ctty(1);
 

This logic will certainly fix it, and I'm not against this logic to fix the problem in a definitive way. It's only unfortunate PF_MEMDIE is smp racy (plus having to check for PF_MEMDIE in the fast path).

I believe the thing you're hiding with the callback, is some screwup in the VM. It shouldn't fire oom 300 times in a row.

zone->all_unreclaimable and zone->pages_scanned _must_ be set to 0 by the oom_killer invocation, did you try to fix that?

the reason for oom in page_alloc.c, is that you must check the free pages levels correctly, your previous nr_free_pages() attempt was the unreliable way to do that (it couldn't take into account all the lowmem_reserve levels etc..).

This approach of using a callback will sure work fine for that but the 300 times invocation of oom kill shows something is wrong in the VM, and I believe the screwup is zone->all_unreclaimable and zone->pages_scanned not being resetted by the oom killer invocation. I suspect if you fix that single bit, things will start working even without the callback.

Note that in 2.4 only one task gets killed, and there's no need of any callback in any fast path to make it work. I'm not conceptually against the callback to "guarantee" oom-killing racing avoidance, but right now it sounds like it's hiding some more fundamental problem in 2.6.

Thanks.

Well no ;)

Thomas, could you please put together a description of how to reproduce this behaviour?

:)

As I mentioned before. I'm using a PIII,500Mhz,128MB Machine. Kernel compiled with PREEMPT=y.
I boot into runlevel 3 and start

# hackbench 40
from the shell.

Just adjust the number so hackbench eats up all the memory.

I have some more test cases with real applications, but they are not so easy to reproduce. Hackbench resembles the forking server quite well.

tglx

How much swap space is online?

I bet zone->all_unreclaimable is one of the main issues here as Andrea notes.

A simple "fillmem" works for me - the OOM killer kills the hog and the bash which its being ran from. Have you tried that?

If you fire up a few fillmem's at the same time I bet you'll see the problem in a greater degree.

Could you explain why do we need all_unreclaimable? What is the point of all_unreclaimable if we bypass it at priority = 0? Just to loop a few times (between DEF_PRIORITY and 1) at 100% cpu load?

OTOH we must not forget 2.4(-aa) calls do_exit synchronously and it never sends signals. That might be why 2.4 doesn't kill more than one task by mistake, even without a callback-wakeup. So if we keep sending signals I certainly agree with Thomas that using a callback to stop the VM until the task is rescheduled is a good idea, and potentially it may be even the only possible fix when the oom killer is enabled like in 2.6 (though the 300 kills in between SIGKILL and the reschedule sounds like the VM isn't even trying anymore). Otherwise perhaps his workload is spawning enough tasks, that it takes an huge time for the rechedule (that would explain it too).

Actually this should fix it too btw:

- if (p->flags & PF_MEMDIE)
- return 0;

Thomas can you try the above?

I'd rather fix this by removing buggy code, than by adding additional logics on top of already buggy code (i.e. setting PF_MEMDIE is a smp race and can corrupt other bitflags), but at least the oom-wakeup-callback from do_exit still makes a lot of sense (even if PF_MEMDIE must be dropped since it's buggy, and something else should be used instead).

Whatever we change I'd like to change it on top of my last patch that already removes the 5 seconds fixed waits, and does the right watermark checks before killing the next task (Thomas already attempted that with a not accurate nr_free_pages check, so he at least agrees about the need of checking watermarks before firing up the oom killer).

BTW, checking for pid == 1 like in Thomas's patch is a must, I advocated it for years but nobody listened yet, hope Thomas will be better at convincing the VM mainline folks than me.

It addresses the situation where all of a zone's pages are pinned down for some reason (say, they're all mlocked, or we're out of swapspace). There's no point in chewing tons of CPU scanning this zone on every reclaim attempt, so the VM marks the zone as being full of unreclaimable pages.

When the zone is marked all_unreclaimable, we *logically* don't scan it at all - just skip it. But in practice we do need to detect the situation where some of the zone's pages have become reclaimable again. That could happen because more swapspace has become available, or an app ran munlock() or whatever. So to perform this detection we perform tiny scans to just poll the zone. If that tiny scan results in a page getting reclaimed then we clear all_reclaimable and the zone returns to normal state.

As an alternative to the tiny-scan-polling we could clear all_unreclaimable in all zones when releasing swapspace, when running munlock(), etc.

Probably free_pages_bulk() should only be clearing all_unreclaimable if current->reclaim_state != NULL, because random page freeings in the zone don't necessarily mean that any pages have become reclaimable. Or clear all_unreclaimable in shrink_list() rather than free_pages_bulk().

Could be, I dunno. I've never done any work on the oom-killer and I tend to regard it as a stupid thing which is only there so you can get back into the machine to shut down and restart everything.

I mean, if you ran the machine out of memory then it is underprovisioned and it *will* become unreliable whatever we do. The answer is to Not Do That. As long as the oom-killer allows you to get in and admin the machine later on then that's good enough as far as I'm concerned. Others probably disagree ;)

Yes, that needs a separate task_struct field.

Probably.

yes, we should do that.

hm. I thought we were already doing that, but it seems not.

I agree, but the current situation made me drive 150km, because sshd or even init was killed.

I hit this problem, when a forking server application got out of control because there were suddenly connecting hundreds of clients due to a different problem.

As long as I can log into the machine and fix the crap it's ok. There's no need for anything else than accessability and a half way deterministic behaviour.

tglx

I just run the same test on 2.4.27 and the behaviour is completely different.

The box seems to be stuck in a swap in/out loop for quite a long time. During this time the box is not responsive at all. It finally stops the forking after quite a long time of swapping with
fork() (error: resource temporarily not available).

There is no output in dmesg, but I'm not able to remove the remaining hackbench processes as even a kill -SIGKILL returns with fork() (error: resource temporarily not available)

I'm not sure, which of the two scenarios I like better :)

FYI, I tried with 2.6 UP and PREEMPT=n. The result is more horrible. The box just gets stuck in an endless swap in/swap out and does not respond to anything else than SysRq-T and the reset button.

With the callback the machine did not come back after 20 Minutes.

Without the callback the machine dies after about 10 Minutes and killing all available processes except init with:

Kernel panic - not syncing: Out of memory and no killable processes...

Yeah, there are enough tasks and with preempt enabled more than one tasks requests memory. That explains the repetitive calls to out_of_memory(). This only happens on UP + PREEMPT=y and SMP. See above.

You meant the one in badness() right ?
Well it makes it better, but I was able to have a second invocation before the first killed tasks exited. That's simple to explain. The task is on the way out and releases resources, so the VM size is reduced and the killer picks another process.

I think the callback is the only safe way to fix that. If PF_MEMDIE is racy then I'm sure we will find a different indicator for that.

Yep, but the reentrancy blocking with the callback makes the time, count crap and the watermark check go away, as it is safe to reenable the killer at this point because we definitely freed memory resources. So the watermark comes for free.

:)

tglx

There's a patch in -mm which causes the oom-killer to be invoked each time you hit sysrq-F, which sounds like a fine idea to me.

Unfortunately it calls the oom-killer from interrupt context, and I need to fix that up before the patch goes any further.

Can you please explain, how I can hit sysrq-F when I can't log into the remote machine ?

tglx

umm, in the same way you're using "SysRq-T and the reset button"?

You can issue sysrq commands over serial consoles too.

I know, but the console and the reset button are 150km away. When I dial into the machine or try to connect via the network, I cannot connect with the current kernels. Neither 2.4, because the fork fails, nor 2.6 because oom killed sshd. So I cannot send anything except a service man, who drives 150km to hit sysrq-F or the reset button.

If this happens the kernel just should make sure to make the machine accessible and let me repair the problem. Nothing else.

tglx

Get one of those terminal server/concentrators that export the serial consoles over IP. Or one of those KVM-over-IP extenders. Worth every penny.

[sorry thomas, forgot reply all first time aound, so you'll get a dupe.]

Makes totally sense.

I add 1000$ equipment to a 300$ embedded box because there is a software bug.

Is it possible that we are living in a different universe ?

tglx

You'd need a little old computer next to your server with serial console attached to it for it to be effective. But it sounds more like a last resort and it makes economical sense only if you're not hosting your server and you've your own server room. Normally one doesn't need to drive 150km just because you can call somebody at the phone to click reboot (worse than SYSRQ+F [unless it was the critical app itself hitting a memleak], but much cheaper than hosting a serial console server too).

The SYSRQ+F sounds more useful on a desktop usage, if you've tons and tons of swap. Don't forget that with an huge amount of swap, you're telling the kernel "please make my machine extremely slow if all apps uses the ram at the same time". In many situations it may be more efficicient for you to kill one of these apps, wait the other to finish, and restart the killed app from scratch (to avoid trashing, and the kernel _can't_ avoid trashing or it wouldn't be fair anymore). So if you do a mistake and you want to recover responsiveness in less than a second, the SYSRQ+F trick should make it.

The case of OOM killed sshd is fixable without touching the kernel:
Make sure sshd is started from init, init will then restart sshd whenever it quits for some reason. This will get you your essential sshd back assuming the machine is still running and the OOM killer managed to free up some memory by killing some other processes.

One might still wish for better OOM behaviour, but it is a case where something has to give.

Helge Hafting

Hey, are you kidding ?

2.4 lets me not in, because the fork of sshd fails. How do you fix this with changing the userspace ?

2.6 oom is plain buggy

I have no problem to help myself, but I want to get this fixed in a reliable way which meets the comment in oom_kill.c: "least surprise"

tglx

Please don't misunderstand. I'm not saing that 2.6 is fine, only that there is a way to automatically restart a sshd accidentally killed by the OOM killer. This could probably save you some of those trips, if you keep experimenting with 2.6.

Fork failing is another issue than a killed sshd. It is usually fixed by using ulimit so a buggy process simply cant fork off way too many children. (Or use up too much memory.)

Quite possible, but be aware that these things can happen with 2.4 too. It is possible to get 2.4 into a shortage where fork fails, you should use ulimit anyway to avoid that. Also, having
a sshd that is restarted by "init" is a good idea anyway
on such a remote machine. 2.4 might not kill sshd by kernel bug, but who knows what some future exploit or future bug could do.

I too want a well-behaved OOM killer. Workarounds are available though, until this happens.

Helge Hafting

Fork eventually failing is very reasonable if you're executing a fork loop.

Please try with 2.4.23aa3, I think there was some oom killer change after I had no resources to track 2.4 anymore. I'm not saying 2.4.23aa3 will work better though, but I would like to know if there's some corner case still open in 2.4-aa. Careful, 2.4.23aa3 has security bugs (only local security of course, i.e. normally not a big issue, sure good enough for a quick test).

I doubt your testcase simulates anything remotely realistic but anyway it's still informative.

What I'm simulating here is very real life scenario with a couple of apps allocating more memory than ram.

Was the oom killer invoked at all? If yes, and it works with preempt, that could mean a cond_resched is simply missing...

yes.

That's trivial to fix checking for PF_DEAD/PF_EXITING.

The callback adds overhead to the exit path. Plus strictly speaking it's not actually a callback, you're just "polling" for the bitflag :)

You can get an I/O race where your program is about to finish a failing try_to_free_pages pass (note that a task exiting won't make try_to_free_pages work any easier, try_to_free_pages has to free allocated memory, it doesn't care if there's 1M or 100M of free memory). If you don't check the watermarks after waiting for I/O, you're going to generate a suprious oom-killing. Your changes can't help.

Note that even the watermark checks leaves a race window open, but at least it's not an I/O window. While try_to_free_pages can wait for I/O and then fail.

I'll add to my last patch the removal of the PF_MEMDIE check in oom_kill plus I'll fix the remaining race with PF_EXITING/DEAD, and I'll add a cond_resched. Then you can try again with my simple way (w/ and w/o PREEMPT ;).

Thanks for the great feedback.

Ok, I expect this patch to fix the problem completely. The biggest difference is that it doesn't affect the exit fast path and it doesn't need notification. It's not even slower than your patch since you were really polling too. This schedule properly, if you get any PREEMPT=n problem I'd really like to know. This is on top of my previous patch so it does the checks for the watermarks correctly (those are not obsoleted by whatever thing we do in out_of_memory()). This still make use of the PF_MEMDIE info but not in kernel/, only in mm/oom_kill.c where it born and dies there, so the race is somewhat hidden in there. As you said converting PF_MEMDIE to a set_tsk_thread_flag or some other non-racy thing is a due change but I'm not doing it in the below patch.

With this thing, I doubt any wrong task will ever be killed again and you won't risk to drive 150km again (or at least not for this problem ;). Please test and apply to mainline if you agree.

 oom_kill.c   |  158 ++++++++++++++++++++---------------------------------------
 page_alloc.c |   60 ++++++++++++++++------
 swap_state.c |    2 
 vmscan.c     |    2 
 4 files changed, 103 insertions(+), 119 deletions(-)

Index: x/mm/oom_kill.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/oom_kill.c,v retrieving revision 1.31

diff -u -p -r1.31 oom_kill.c
@@ -49,8 +49,6 @@ static unsigned long badness(struct task
 	if (!p->mm)
 		return 0;
 
-	if (p->flags & PF_MEMDIE)
-		return 0;
 	/*
 	 * The memory size of the process is the basis for the badness.
 	 */

@@ -120,14 +118,25 @@ static struct task_struct * select_bad_p
 
 	do_posix_clock_monotonic_gettime(&uptime);
 	do_each_thread(g, p)
-		if (p->pid) {
-			unsigned long points = badness(p, uptime.tv_sec);
+		/* skip the init task with pid == 1 */
+		if (p->pid > 1) {
+			unsigned long points;
+
+			/*
+			 * This is in the process of releasing memory so wait it
+			 * to finish before killing some other task by mistake.
+			 */
+			if ((p->flags & PF_MEMDIE) ||
+			    ((p->flags & PF_EXITING) && !(p->flags & PF_DEAD)))
+				return ERR_PTR(-1UL);
+			if (p->flags & PF_SWAPOFF)
+				return p;
+
+			points = badness(p, uptime.tv_sec);
 			if (points > maxpoints) {
 				chosen = p;
 				maxpoints = points;
 			}
-			if (p->flags & PF_SWAPOFF)
-				return p;
 		}
 	while_each_thread(g, p);
 	return chosen;

@@ -140,6 +149,12 @@ static struct task_struct * select_bad_p
  */
 static void __oom_kill_task(task_t *p)
 {
+	if (p->pid == 1) {
+		WARN_ON(1);
+		printk(KERN_WARNING "tried to kill init!\n");
+		return;
+	}
+
 	task_lock(p);
 	if (!p->mm || p->mm == &init_mm) {
 		WARN_ON(1);

@@ -169,9 +184,25 @@ static void __oom_kill_task(task_t *p)
 static struct mm_struct *oom_kill_task(task_t *p)
 {
 	struct mm_struct *mm = get_task_mm(p);
-	if (!mm || mm == &init_mm)
+	task_t * g, * q;
+
+	if (!mm)
 		return NULL;
+	if (mm == &init_mm) {
+		mmput(mm);
+		return NULL;
+	}
+
 	__oom_kill_task(p);
+	/*
+	 * kill all processes that share the ->mm (i.e. all threads),
+	 * but are in a different thread group
+	 */
+	do_each_thread(g, q)
+		if (q->mm == mm && q->tgid != p->tgid)
+			__oom_kill_task(q);
+	while_each_thread(g, q);
+
 	return mm;
 }
 

@@ -184,117 +215,40 @@ static struct mm_struct *oom_kill_task(t
  * OR try to be smart about which process to kill. Note that we
  * don't have to be perfect here, we just have to be good.
  */
-static void oom_kill(void)
+void out_of_memory(int gfp_mask)
 {
-	struct mm_struct *mm;
-	struct task_struct *g, *p, *q;
-	
+	struct mm_struct *mm = NULL;
+	task_t * p;
+
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process();
 
+	if (PTR_ERR(p) == -1UL)
+		goto out;
+
 	/* Found nothing?!?! Either we hang forever, or we panic. */
 	if (!p) {
+		read_unlock(&tasklist_lock);
 		show_free_areas();
 		panic("Out of memory and no killable processes...\n");
 	}
 
+	printk("oom-killer: gfp_mask=0x%x\n", gfp_mask);
+	show_free_areas();
 	mm = oom_kill_task(p);
 	if (!mm)
 		goto retry;
-	/*
-	 * kill all processes that share the ->mm (i.e. all threads),
-	 * but are in a different thread group
-	 */
-	do_each_thread(g, q)
-		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q);
-	while_each_thread(g, q);
-	if (!p->mm)
-		printk(KERN_INFO "Fixed up OOM kill of mm-less task\n");
-	read_unlock(&tasklist_lock);
-	mmput(mm);
 
-	/*
-	 * Make kswapd go out of the way, so "p" has a good chance of
-	 * killing itself before someone else gets the chance to ask
-	 * for more memory.
-	 */
-	yield();
-	return;
-}
-
-/**
- * out_of_memory - is the system out of memory?
- */
-void out_of_memory(int gfp_mask)
-{
-	/*
-	 * oom_lock protects out_of_memory()'s static variables.
-	 * It's a global lock; this is not performance-critical.
-	 */
-	static spinlock_t oom_lock = SPIN_LOCK_UNLOCKED;
-	static unsigned long first, last, count, lastkill;
-	unsigned long now, since;
-
-	spin_lock(&oom_lock);
-	now = jiffies;
-	since = now - last;
-	last = now;
-
-	/*
-	 * If it's been a long time since last failure,
-	 * we're not oom.
-	 */
-	if (since > 5*HZ)
-		goto reset;
-
-	/*
-	 * If we haven't tried for at least one second,
-	 * we're not really oom.
-	 */
-	since = now - first;
-	if (since < HZ)
-		goto out_unlock;
-
-	/*
-	 * If we have gotten only a few failures,
-	 * we're not really oom. 
-	 */
-	if (++count < 10)
-		goto out_unlock;
-
-	/*
-	 * If we just killed a process, wait a while
-	 * to give that task a chance to exit. This
-	 * avoids killing multiple processes needlessly.
-	 */
-	since = now - lastkill;
-	if (since < HZ*5)
-		goto out_unlock;
+ out:
+	read_unlock(&tasklist_lock);
+	if (mm)
+		mmput(mm);
 
 	/*
-	 * Ok, really out of memory. Kill something.
+	 * Give "p" a good chance of killing itself before we
+	 * retry to allocate memory.
 	 */
-	lastkill = now;
-
-	printk("oom-killer: gfp_mask=0x%x\n", gfp_mask);
-	show_free_areas();
-
-	/* oom_kill() sleeps */
-	spin_unlock(&oom_lock);
-	oom_kill();
-	spin_lock(&oom_lock);
-
-reset:
-	/*
-	 * We dropped the lock above, so check to be sure the variable
-	 * first only ever increases to prevent false OOM's.
-	 */
-	if (time_after(now, first))
-		first = now;
-	count = 0;
-
-out_unlock:
-	spin_unlock(&oom_lock);
+	__set_current_state(TASK_INTERRUPTIBLE);
+	schedule_timeout(1);
 }

Index: x/mm/page_alloc.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/page_alloc.c,v retrieving revision 1.236

Index: x/mm/swap_state.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/swap_state.c,v retrieving revision 1.79

diff -u -p -r1.79 swap_state.c
@@ -59,6 +59,8 @@ void show_swap_cache_info(void)
 		swap_cache_info.add_total, swap_cache_info.del_total,
 		swap_cache_info.find_success, swap_cache_info.find_total,
 		swap_cache_info.noent_race, swap_cache_info.exist_race);
+	printk("Free swap  = %lukB\n", nr_swap_pages << (PAGE_SHIFT - 10));
+	printk("Total swap = %lukB\n", total_swap_pages << (PAGE_SHIFT - 10));
 }
 
 /*

Index: x/mm/vmscan.c
===================================================================
RCS file: /home/andrea/crypto/cvs/linux-2.5/mm/vmscan.c,v retrieving revision 1.225

diff -u -p -r1.225 vmscan.c
@@ -935,8 +935,6 @@ int try_to_free_pages(struct zone **zone
 		if (sc.nr_scanned && priority < DEF_PRIORITY - 2)
 			blk_congestion_wait(WRITE, HZ/10);
 	}
-	if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY))
-		out_of_memory(gfp_mask);
 out:
 	for (i = 0; zones[i] != 0; i++)
 		zones[i]->prev_priority = zones[i]->temp_priority;

You're right. oom-kill() did not do anything wrong. See log below

This is w/o PREEMPT. Is it neccecary to verify w/ PREEMPT too ?

If it would have booted it still would have killed sshd instead of the application which was forking a lot of childs.

tglx

Dentry cache hash table entries: 32768 (order: 5, 131072 bytes) Inode-cache hash table entries: 16384 (order: 4, 65536 bytes)
Memory: 126476k/131060k available (1690k kernel code, 4044k reserved, 732k data)Checking if this processor honours the WP bit even in supervisor mode... Ok.
Mount-cache hash table entries: 512 (order: 0, 4096 bytes)
CPU: L1 I cache: 16K, L1 D cache: 16K
CPU: L2 cache: 128K
Intel machine check architecture supported.
Intel machine check reporting enabled on CPU#0.
CPU: Intel Celeron (Mendocino) stepping 00
Enabling fast FPU save and restore... done.
Checking 'hlt' instruction... OK.

END OF LOG

Ooops, sorry it did add something to the Log after 10 minutes

Unable to handle kernel NULL pointer dereference at virtual address

0000000c
printing eip:
c011fe30
*pde = 00000000

Oops: 0000 [#1]
Modules linked in:
CPU:    0
EIP:    0060:[<c011fe30>]    Not tainted VLI
EFLAGS: 00010082   (2.6.10-rc2)

EIP is at __queue_work+0x20/0x60
eax: 00000000 ebx: c032cc80 ecx: 00000000 edx: 00000008
esi: c03acd14 edi: 00000282 ebp: c035ff64 esp: c035ff34
ds: 007b es: 007b ss: 0068
Process swapper (pid: 0, threadinfo=c035e000 task=c02ebb00)

Stack: c035ffa4 00000000 c03acd14 c035ff64 c011fe99 00000000 c032cc80
c01dd8b0
       c0119c20 00000000 00000000 c035ffa4 c035ff64 c035ff64 00000000
00000001
       c03a5868 0000000a 003d9007 c0115f3b c03a5868 00000046 00099100
c039e120
Call Trace:
 [<c011fe99>] queue_work+0x29/0x50
 [<c01dd8b0>] blank_screen_t+0x0/0x20
 [<c0119c20>] run_timer_softirq+0xb0/0x170
 [<c0115f3b>] __do_softirq+0x7b/0x90
 [<c0115f77>] do_softirq+0x27/0x30
 [<c010411e>] do_IRQ+0x1e/0x30
 [<c010271e>] common_interrupt+0x1a/0x20
 [<c0100623>] default_idle+0x23/0x40
 [<c01006b4>] cpu_idle+0x34/0x40
 [<c03607d8>] start_kernel+0x168/0x1b0
 [<c0360370>] unknown_bootoption+0x0/0x1e0
Code: eb e5 90 90 90 90 90 90 90 90 90 83 ec 10 8b 44 24 14 89 5c 24 04

8b 5c 2
<0>Kernel panic - not syncing: Fatal exception in interrupt

You mean my patch is preventing your machine to boot? Then you're doing something else wrong because it's impossible my patch is preventing your machine to boot. I also tested it before posting. You must have changed something more than the PREEMPT bit after applying my patch.

It can't be a transient error as far as I can tell, it's just like the issue of alloc_pages returning NULL (and potentially scheduling first) before mounting the root fs.

Well, the only way I see this happening is the process exiting followed by use_mm() on init_mm for unobvious reasons (perhaps reasons not in the tree).

-- wli

I don't see the problem with use_mm. use_mm has either the mm set to ctx->mm or to NULL, and ctx->mm is set to the mm of the process calling io_setup.

The only thing using init_mm is the idle task/swapper as far as I can tell, kernel threads and exiting tasks have a NULL mm.

Yet those don't appear in the tasklist, so some task in the tasklist has to get ->mm set to &init_mm. The notion above was that the init_mm check was to handle some out-of-tree attempt to do aio from kernel threads.

-- wli

Not sure to understand correctly but, aio has always been done through kernel threads, and that's the whole thing about aio. Not sure what you're doing out-of-tree, but you don't need to use init_mm to deal with kernel threads, infact kernel threads can only have ->mm = NULL. When switching mm with use_mm the aio thread is only going to use a real mm with mappings in userspace, so even in that case you don't need init_mm. I didn't see the out of tree code though.

Maybe the whole init_mm check should die since nothing in-tree could cause it?

-- wli

Well it's a bugcheck after all so it certainly can be removed. I wouldn't mind to remove it completely, but this is just to be sure nothing is going wrong, though I agree it isn't going to help very much ;).

I see some real bugfixes in here. I'll start up a fresh thread with some of those broken out and cc: the relevant people shortly. It's not really any "new material", just reorganizing things into smaller patches, in particular the ones I see needing merging ASAP that aren't controversial.

-- wli

Yes, it's reasonable, but the effect that any consequent command is aborted then is not so reasonable.

I try, if I find some time, but I'm not too interested in 2.4 :)

The hackbench testcase resembles the real life problem I encountered quite realistic. Of course we have fixed the application since then, but I bet that I'm not the only one who uses forking servers.

Use a forking server, connect a lot of clients and it is real life. :)

Yes, it was invoked

I know :)

True, I did not take the I/O into account.

Did you use the 4k stacks on 2.6 btw?

;)

Ok good.

Hmm, this thread seems to be serious. I'll audit the policy adjustments for issues with the mechanisms (e.g. killing kernel threads, races with timeouts).

-- wli

:)

Andrea provided a quite good fix for the invokation problem. I'm just tracking down a different problem which was revieled by his changes.

The selection mechanism is currently taking a couple of things into account:

- VM size
- nice value
- CPU time
- owner == root ?
- CAP_SYS_RAWIO

I found out, that it is neccecary to take the child processes into account to detect processes which fork a lot of childs.

The current mechanism rather kills sshd or portmap as they happen to have a bigger VM size than the process which forked a lot of childs.

So I added a check for the child processes with an own VM. This solved the problem quite well. Of course it does not count the childs of PID < 2.

tglx

Taking childs into account looks fine to me.

It appears the net functional change here is the reentrancy prevention; the choice of tasks is policy. The functional change may be accomplished with the following:

Index: linux-2.6.9/mm/oom_kill.c
===================================================================
@@ -237,7 +237,8 @@
 	static unsigned long first, last, count, lastkill;
 	unsigned long now, since;
 
-	spin_lock(&oom_lock);
+	if (!spin_trylock(&oom_lock))
+		return;
 	now = jiffies;
 	since = now - last;
 	last = now;

@@ -282,9 +283,7 @@
 	show_free_areas();
 
 	/* oom_kill() sleeps */
-	spin_unlock(&oom_lock);
 	oom_kill();
-	spin_lock(&oom_lock);
 
 reset:
 	/*

Your patch would call yield() with the lock held. On an UP machine you end up in the same code, as spin_locks are NOPs except for the preempt part.

It's now obsolete by the fixes which were done by Andrea.

I'm wondering why he did not post the final version. Andrea ???

Attached is the latest working and tested patch. It contains Andrea's fixes to the oom invocation and my modifications to the selection whom to kill.

This should really go into mainline.

tglx

diff -urN 2.6.10-rc2.orig/mm/oom_kill.c 2.6.10-rc2.oom/mm/oom_kill.c
@@ -45,18 +45,30 @@
 static unsigned long badness(struct task_struct *p, unsigned long uptime)
 {
 	unsigned long points, cpu_time, run_time, s;
+	struct list_head *tsk;
 
 	if (!p->mm)
 		return 0;
 
-	if (p->flags & PF_MEMDIE)
-		return 0;
 	/*
 	 * The memory size of the process is the basis for the badness.
 	 */
 	points = p->mm->total_vm;
 
 	/*
+	 * Processes which fork a lot of child processes are likely 
+	 * a good choice. We add the vmsize of the childs if they
+	 * have an own mm. This prevents forking servers to flood the
+	 * machine with an endless amount of childs
+	 */
+	list_for_each(tsk, &p->children) {
+		struct task_struct *chld;
+		chld = list_entry(tsk, struct task_struct, sibling);
+		if (chld->mm != p->mm && chld->mm)
+			points += chld->mm->total_vm;
+	}
+
+	/*
 	 * CPU time is in tens of seconds and run time is in thousands
          * of seconds. There is no particular reason for this other than
          * that it turned out to work very well in practice.

@@ -120,14 +132,25 @@
 
 	do_posix_clock_monotonic_gettime(&uptime);
 	do_each_thread(g, p)
-		if (p->pid) {
-			unsigned long points = badness(p, uptime.tv_sec);
+		/* skip the init task with pid == 1 */
+		if (p->pid > 1) {
+			unsigned long points;
+
+			/*
+			 * This is in the process of releasing memory so wait it
+			 * to finish before killing some other task by mistake.
+			 */
+			if ((p->flags & PF_MEMDIE) ||
+			    ((p->flags & PF_EXITING) && !(p->flags & PF_DEAD)))
+				return ERR_PTR(-1UL);
+			if (p->flags & PF_SWAPOFF)
+				return p;
+
+			points = badness(p, uptime.tv_sec);
 			if (points > maxpoints) {
 				chosen = p;
 				maxpoints = points;
 			}
-			if (p->flags & PF_SWAPOFF)
-				return p;
 		}
 	while_each_thread(g, p);
 	return chosen;

@@ -140,6 +163,12 @@
  */
 static void __oom_kill_task(task_t *p)
 {
+	if (p->pid == 1) {
+		WARN_ON(1);
+		printk(KERN_WARNING "tried to kill init!\n");
+		return;
+	}
+
 	task_lock(p);
 	if (!p->mm || p->mm == &init_mm) {
 		WARN_ON(1);

@@ -169,12 +198,45 @@
 static struct mm_struct *oom_kill_task(task_t *p)
 {
 	struct mm_struct *mm = get_task_mm(p);
-	if (!mm || mm == &init_mm)
+	task_t * g, * q;
+
+	if (!mm)
 		return NULL;
+	if (mm == &init_mm) {
+		mmput(mm);
+		return NULL;
+	}
+
 	__oom_kill_task(p);
+	/*
+	 * kill all processes that share the ->mm (i.e. all threads),
+	 * but are in a different thread group
+	 */
+	do_each_thread(g, q)
+		if (q->mm == mm && q->tgid != p->tgid)
+			__oom_kill_task(q);
+	while_each_thread(g, q);
+
 	return mm;
 }
 
+static struct mm_struct *oom_kill_process(task_t *p)
+{
+ 	struct mm_struct *mm;
+	struct task_struct *c;
+	struct list_head *tsk;
+
+	/* Try to kill a child first */
+	list_for_each(tsk, &p->children) {
+		c = list_entry(tsk, struct task_struct, sibling);
+		if (c->mm == p->mm)
+			continue;
+		mm = oom_kill_task(c);
+		if (mm)
+			return mm;
+	}
+	return oom_kill_task(p);
+}
 
 /**
  * oom_kill - kill the "best" process when we run out of memory

@@ -184,117 +246,40 @@
  * OR try to be smart about which process to kill. Note that we
  * don't have to be perfect here, we just have to be good.
  */
-static void oom_kill(void)
+void out_of_memory(int gfp_mask)
 {
-	struct mm_struct *mm;
-	struct task_struct *g, *p, *q;
-	
+	struct mm_struct *mm = NULL;
+	task_t * p;
+
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process();
 
+	if (PTR_ERR(p) == -1UL)
+		goto out;
+
 	/* Found nothing?!?! Either we hang forever, or we panic. */
 	if (!p) {
+		read_unlock(&tasklist_lock);
 		show_free_areas();
 		panic("Out of memory and no killable processes...\n");
 	}
 
-	mm = oom_kill_task(p);
-	if (!mm)
-		goto retry;
-	/*
-	 * kill all processes that share the ->mm (i.e. all threads),
-	 * but are in a different thread group
-	 */
-	do_each_thread(g, q)
-		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q);
-	while_each_thread(g, q);
-	if (!p->mm)
-		printk(KERN_INFO "Fixed up OOM kill of mm-less task\n");
-	read_unlock(&tasklist_lock);
-	mmput(mm);
-
-	/*
-	 * Make kswapd go out of the way, so "p" has a good chance of
-	 * killing itself before someone else gets the chance to ask
-	 * for more memory.
-	 */
-	yield();
-	return;
-}
-
-/**
- * out_of_memory - is the system out of memory?
- */
-void out_of_memory(int gfp_mask)
-{
-	/*
-	 * oom_lock protects out_of_memory()'s static variables.
-	 * It's a global lock; this is not performance-critical.
-	 */
-	static spinlock_t oom_lock = SPIN_LOCK_UNLOCKED;
-	static unsigned long first, last, count, lastkill;
-	unsigned long now, since;
-
-	spin_lock(&oom_lock);
-	now = jiffies;
-	since = now - last;
-	last = now;
-
-	/*
-	 * If it's been a long time since last failure,
-	 * we're not oom.
-	 */
-	if (since > 5*HZ)
-		goto reset;
-
-	/*
-	 * If we haven't tried for at least one second,
-	 * we're not really oom.
-	 */
-	since = now - first;
-	if (since < HZ)
-		goto out_unlock;
-
-	/*
-	 * If we have gotten only a few failures,
-	 * we're not really oom. 
-	 */
-	if (++count < 10)
-		goto out_unlock;
-
-	/*
-	 * If we just killed a process, wait a while
-	 * to give that task a chance to exit. This
-	 * avoids killing multiple processes needlessly.
-	 */
-	since = now - lastkill;
-	if (since < HZ*5)
-		goto out_unlock;
-
-	/*
-	 * Ok, really out of memory. Kill something.
-	 */
-	lastkill = now;
-
 	printk("oom-killer: gfp_mask=0x%x\n", gfp_mask);
 	show_free_areas();
+	mm = oom_kill_process(p);
+	if (!mm)
+		goto retry;
 
-	/* oom_kill() sleeps */
-	spin_unlock(&oom_lock);
-	oom_kill();
-	spin_lock(&oom_lock);
+ out:
+	read_unlock(&tasklist_lock);
+	if (mm)
+		mmput(mm);
 
-reset:
 	/*
-	 * We dropped the lock above, so check to be sure the variable
-	 * first only ever increases to prevent false OOM's.
+	 * Give "p" a good chance of killing itself before we
+	 * retry to allocate memory.
 	 */
-	if (time_after(now, first))
-		first = now;
-	count = 0;
-
-out_unlock:
-	spin_unlock(&oom_lock);
+	__set_current_state(TASK_INTERRUPTIBLE);
+	schedule_timeout(1);
 }

diff -urN 2.6.10-rc2.orig/mm/swap_state.c 2.6.10-rc2.oom/mm/swap_state.c
@@ -59,6 +59,8 @@
 		swap_cache_info.add_total, swap_cache_info.del_total,
 		swap_cache_info.find_success, swap_cache_info.find_total,
 		swap_cache_info.noent_race, swap_cache_info.exist_race);
+	printk("Free swap  = %lukB\n", nr_swap_pages << (PAGE_SHIFT - 10));
+	printk("Total swap = %lukB\n", total_swap_pages << (PAGE_SHIFT - 10));
 }
 
 /*

diff -urN 2.6.10-rc2.orig/mm/vmscan.c 2.6.10-rc2.oom/mm/vmscan.c
@@ -935,8 +935,6 @@
 		if (sc.nr_scanned && priority < DEF_PRIORITY - 2)
 			blk_congestion_wait(WRITE, HZ/10);
 	}
-	if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY))
-		out_of_memory(gfp_mask);
 out:
 	for (i = 0; zones[i] != 0; i++)
 		zones[i]->prev_priority = zones[i]->temp_priority;

ARGH, preempt.

-- wli

This should all be split up; it's doing a dozen things at a time.

-- wli

I already posted the final version since it had no bugs and I asked to get it merged twice. The only bugs are obviously in the drivers (or the callers) and they needs urgent fixing and additionally the might_sleep_if must stop checking if the system is running so these bugs can see the light of the day. Not being allowed to schedule in alloc_pages with __GFP_WAIT set is a mistake.

Your patch was orthogonal to mine, so I didn't merge it. Go figure that every time I post something it gets splitted into trivial pieces, so it's a waste of time to try to merge any additional patch and post a final one since it'll never be final anyway.

I am about to merge the things together for some other tree (not mainline), that is a worthwhile effort but with the split behaviour of mainline, for mainline it'd be a waste of time.

One last thing worth discussing on my side is if we should worry about the tiny race between the watermark checks and the entering of the oom killing. In theory we could wrap the thing around a semaphore and close the race completely, though current code is simpler and as you find it works fine in practice.

The easy way to fix that issue is to take the whole diff and break off pieces, with the remainder always as the last patch. That way the whole set of changes stays pending and appears intact at the end of the series.

I will personally be held responsible for identifying the causes of behavioral changes in the OOM killer, and am having to investigate several instances of bad OOM killer behavior already, so I have to do this anyway, and so it might as well be done for mainline.

-- wli

Actually, I'm dropping my whole public effort and am going to stick to using what I do in this respect for internal testing etc.

-- wli