2.6.19-rc1: oom killer fixes

RFC PATCH In review Nick Piggin 4 participants 15 messages Oct 12, 2006 – Oct 13, 2006

Subsystems (8)

Memory management Sound PowerPC MM core +4 more subsystems

lore

Share & tools

lore

Nick Piggin

author

2006-10-12 14:09 UTC #0 lore raw

cover 2.6.19-rc1: oom killer fixes

I've been prompted to take another look through the OOM killer because it turns out it is killing tasks that have had their oom_adj set to -17 (which is supposed to make them unkillable).

So there are a number of problems, firstly, the child and sibling thread killing routines do not account for -17 children/siblings.

Secondly, most architecture specific pagefault handlers do a direct kill of the current process if it takes a VM_FAULT_OOM. This is a pretty rare thing to happen, because there isn't a lot of higher order allocations happening, but it is not impossible. I think we can just call into the OOM killer here, and return to userspace... but I'd like comments about this.

Thanks,
Nick
--
SuSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Nick Piggin

author

2006-10-12 14:09 UTC #1 lore raw

PATCH 1/5 oom: don't kill unkillable children or siblings

Abort the kill if any of our threads have OOM_DISABLE set. Having this test here also prevents any OOM_DISABLE child of the "selected" process from being killed.

Signed-off-by Nick Piggin <[email protected]>

mm/oom_kill.c +11 -2

Index: linux-2.6/mm/oom_kill.c
===================================================================
@@ -312,15 +312,24 @@ static int oom_kill_task(struct task_str
 	if (mm == NULL)
 		return 1;
 
+	/*
+	 * Don't kill the process if any threads are set to OOM_DISABLE
+	 */
+	do_each_thread(g, q) {
+		if (q->mm == mm && p->oomkilladj == OOM_DISABLE)
+			return 1;
+	} while_each_thread(g, q);
+
 	__oom_kill_task(p, message);
+
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),
 	 * but are in a different thread group
 	 */
-	do_each_thread(g, q)
+	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
 			__oom_kill_task(q, message);
-	while_each_thread(g, q);
+	} while_each_thread(g, q);
 
 	return 0;
 }

Andrew Morton

2006-10-12 22:00 UTC #2 lore raw

Re: [patch 1/5] oom: don't kill unkillable children or siblings

Quoted 26 lines · On Thu, 12 Oct 2006 16:09:43 +0200 (CEST) Nick Piggin <[email protected]> wrote:

Abort the kill if any of our threads have OOM_DISABLE set. Having this test here also prevents any OOM_DISABLE child of the "selected" process from being killed.
Signed-off-by Nick Piggin <[email protected]>
Index: linux-2.6/mm/oom_kill.c
===================================================================
mm/oom_kill.c +9 -0
@@ -312,15 +312,24 @@ static int oom_kill_task(struct task_str
 	if (mm == NULL)
 		return 1;
 
+	/*
+	 * Don't kill the process if any threads are set to OOM_DISABLE
+	 */
+	do_each_thread(g, q) {
+		if (q->mm == mm && p->oomkilladj == OOM_DISABLE)
+			return 1;
+	} while_each_thread(g, q);
+
 	__oom_kill_task(p, message);
+
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),

mm/oom_kill.c static int oom_kill_task(struct task_str · line 334

Quoted 10 lines

mm/oom_kill.c +2 -2

 	 * but are in a different thread group
 	 */
-	do_each_thread(g, q)
+	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
 			__oom_kill_task(q, message);
-	while_each_thread(g, q);
+	} while_each_thread(g, q);
 
 	return 0;

One wonders whether OOM_DISABLE should be a property of the mm_struct, not of the task_struct.

Nick Piggin

2006-10-13 06:31 UTC #3 lore raw

Re: [patch 1/5] oom: don't kill unkillable children or siblings

Quoted 39 lines · Andrew Morton

Quoted 35 lines · On Thu, 12 Oct 2006 16:09:43 +0200 (CEST) Nick Piggin <[email protected]> wrote:
Abort the kill if any of our threads have OOM_DISABLE set. Having this test here also prevents any OOM_DISABLE child of the "selected" process from being killed.
Signed-off-by Nick Piggin <[email protected]>
Index: linux-2.6/mm/oom_kill.c
===================================================================
mm/oom_kill.c +11 -2
@@ -312,15 +312,24 @@ static int oom_kill_task(struct task_str
 	if (mm == NULL)
 		return 1;
 
+	/*
+	 * Don't kill the process if any threads are set to OOM_DISABLE
+	 */
+	do_each_thread(g, q) {
+		if (q->mm == mm && p->oomkilladj == OOM_DISABLE)
+			return 1;
+	} while_each_thread(g, q);
+
 	__oom_kill_task(p, message);
+
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),
 	 * but are in a different thread group
 	 */
-	do_each_thread(g, q)
+	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
 			__oom_kill_task(q, message);
-	while_each_thread(g, q);
+	} while_each_thread(g, q);
 

Quoted 6 lines

mm/oom_kill.c static int oom_kill_task(struct task_str · line 335
Quoted 2 lines
mm/oom_kill.c +0 -0
 	return 0;
 
One wonders whether OOM_DISABLE should be a property of the mm_struct, not of the task_struct.

Hmm... I don't think I could argue with that. I think this patch is needed in the meantime though.

Send instant messages to your online friends http://au.messenger.yahoo.com

Nick Piggin

author

2006-10-12 14:09 UTC #4 lore raw

PATCH 2/5 oom: cleanup messages

In reply to Nick Piggin

Clean up the OOM killer messages to be more consistent.

Signed-off-by Nick Piggin <[email protected]>

mm/oom_kill.c +13 ~~-14~~

Index: linux-2.6/mm/oom_kill.c
===================================================================
@@ -263,7 +263,7 @@ static struct task_struct *select_bad_pr
  * flag though it's unlikely that  we select a process with CAP_SYS_RAW_IO
  * set.
  */
-static void __oom_kill_task(struct task_struct *p, const char *message)
+static void __oom_kill_task(struct task_struct *p, int verbose)
 {
 	if (is_init(p)) {
 		WARN_ON(1);

@@ -277,10 +277,8 @@ static void __oom_kill_task(struct task_
 		return;
 	}
 
-	if (message) {
-		printk(KERN_ERR "%s: Killed process %d (%s).\n",
-				message, p->pid, p->comm);
-	}
+	if (verbose)
+		printk(KERN_ERR "Killed process %d (%s).\n", p->pid, p->comm);
 
 	/*
 	 * We give our sacrificial lamb high priority and access to

@@ -293,7 +291,7 @@ static void __oom_kill_task(struct task_
 	force_sig(SIGKILL, p);
 }
 
-static int oom_kill_task(struct task_struct *p, const char *message)
+static int oom_kill_task(struct task_struct *p)
 {
 	struct mm_struct *mm;
 	struct task_struct *g, *q;

@@ -320,15 +318,15 @@ static int oom_kill_task(struct task_str
 			return 1;
 	} while_each_thread(g, q);
 
-	__oom_kill_task(p, message);
+	__oom_kill_task(p, 1);
 
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),
-	 * but are in a different thread group
+	 * but are in a different thread group.
 	 */
 	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q, message);
+			__oom_kill_task(q, 1);
 	} while_each_thread(g, q);
 
 	return 0;

@@ -345,21 +343,22 @@ static int oom_kill_process(struct task_
 	 * its children or threads, just set TIF_MEMDIE so it can die quickly
 	 */
 	if (p->flags & PF_EXITING) {
-		__oom_kill_task(p, NULL);
+		__oom_kill_task(p, 0);
 		return 0;
 	}
 
-	printk(KERN_ERR "Out of Memory: Kill process %d (%s) score %li"
-			" and children.\n", p->pid, p->comm, points);
+	printk(KERN_ERR "%s: kill process %d (%s) score %li or a child.\n",
+					message, p->pid, p->comm, points);
+
 	/* 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;
-		if (!oom_kill_task(c, message))
+		if (!oom_kill_task(c))
 			return 0;
 	}
-	return oom_kill_task(p, message);
+	return oom_kill_task(p);
 }
 
 static BLOCKING_NOTIFIER_HEAD(oom_notify_list);

Nick Piggin

author

2006-10-12 14:10 UTC #5 lore raw

PATCH 3/5 oom: less memdie

In reply to Nick Piggin

Don't cause all threads in all other thread groups to gain TIF_MEMDIE otherwise we'll get a thundering herd eating out memory reserve. This may not be the optimal scheme, but it fits our policy of allowing just one TIF_MEMDIE in the system at once.

Signed-off-by Nick Piggin <[email protected]>

mm/oom_kill.c +3 -2

Index: linux-2.6/mm/oom_kill.c
===================================================================
@@ -322,11 +322,12 @@ static int oom_kill_task(struct task_str
 
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),
-	 * but are in a different thread group.
+	 * but are in a different thread group. Don't let them have access
+	 * to memory reserves though, otherwise we might deplete all memory.
 	 */
 	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q, 1);
+			force_sig(SIGKILL, p);
 	} while_each_thread(g, q);
 
 	return 0;

Andrew Morton

2006-10-12 22:03 UTC #6 lore raw

Re: [patch 3/5] oom: less memdie

Quoted 15 lines · On Thu, 12 Oct 2006 16:10:01 +0200 (CEST) Nick Piggin <[email protected]> wrote:

Don't cause all threads in all other thread groups to gain TIF_MEMDIE otherwise we'll get a thundering herd eating out memory reserve. This may not be the optimal scheme, but it fits our policy of allowing just one TIF_MEMDIE in the system at once.
Signed-off-by Nick Piggin <[email protected]>
Index: linux-2.6/mm/oom_kill.c
===================================================================
mm/oom_kill.c +0 -0
@@ -322,11 +322,12 @@ static int oom_kill_task(struct task_str
 
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),

mm/oom_kill.c static int oom_kill_task(struct task_str · line 332

Quoted 10 lines

mm/oom_kill.c +3 -2

-	 * but are in a different thread group.
+	 * but are in a different thread group. Don't let them have access
+	 * to memory reserves though, otherwise we might deplete all memory.
 	 */
 	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q, 1);
+			force_sig(SIGKILL, p);
 	} while_each_thread(g, q);

Curious. How much testing did you do of this stuff? I assume there were some observed problems. What were they, and what was the observed effect of these changes?

Thanks.

Nick Piggin

2006-10-13 06:38 UTC #7 lore raw

Re: [patch 3/5] oom: less memdie

Quoted 26 lines · Andrew Morton

Quoted 22 lines · On Thu, 12 Oct 2006 16:10:01 +0200 (CEST) Nick Piggin <[email protected]> wrote:
Don't cause all threads in all other thread groups to gain TIF_MEMDIE otherwise we'll get a thundering herd eating out memory reserve. This may not be the optimal scheme, but it fits our policy of allowing just one TIF_MEMDIE in the system at once.
Signed-off-by Nick Piggin <[email protected]>
Index: linux-2.6/mm/oom_kill.c
===================================================================
mm/oom_kill.c +2 -2
@@ -322,11 +322,12 @@ static int oom_kill_task(struct task_str
 
 	/*
 	 * kill all processes that share the ->mm (i.e. all threads),
-	 * but are in a different thread group.
+	 * but are in a different thread group. Don't let them have access
+	 * to memory reserves though, otherwise we might deplete all memory.
 	 */
 	do_each_thread(g, q) {
 		if (q->mm == mm && q->tgid != p->tgid)
-			__oom_kill_task(q, 1);

Quoted 9 lines

mm/oom_kill.c static int oom_kill_task(struct task_str
Quoted 4 lines
mm/oom_kill.c +1 -0
+			force_sig(SIGKILL, p);
 	} while_each_thread(g, q);
 
 
Curious. How much testing did you do of this stuff? I assume there were some observed problems. What were they, and what was the observed effect of these changes?

This change I actually didn't really test because I don't have any apps to speak of which use multiple thread groups.

I stumbled on it by inspection when trying to fix the killing of OOM_DISABLE tasks. Basically -- we don't set TIF_MEMDIE or boost the priority of *any* other thread in our same group, so we shouldn't do it for *all* other threds of all other groups.

Consider an OOM situation, where there will likely be a lot of threads stuck in __alloc_pages. If a large number of these suddenly get a big timeslice and full access to memory reserves, they'll eat into more than we'd like.

Now I'm not sure that our current OOM killing / memory reserving scheme is perfect -- indeed if we only allow a single TIF_MEMDIE thread at once, we can get deadlocks. However, that's the direction we've chosen, and it seems to work reasonably well. Mostly.

This is just an enforcement of that policy rather than a change in direction. Criticism is always welcome though.

Send instant messages to your online friends http://au.messenger.yahoo.com

Nick Piggin

author

2006-10-12 14:10 UTC #8 lore raw

PATCH 4/5 mm: incorrect VM_FAULT_OOM returns from drivers

In reply to Nick Piggin

Some drivers are returning OOM when it is not in response to a memory shortage.

Signed-off-by Nick Piggin <[email protected]>

drivers/char/drm/drm_vm.c +4 -4

Index: linux-2.6/drivers/char/drm/drm_vm.c
===================================================================
@@ -147,14 +147,14 @@ static __inline__ struct page *drm_do_vm
 	if (address > vma->vm_end)
 		return NOPAGE_SIGBUS;	/* Disallow mremap */
 	if (!map)
-		return NOPAGE_OOM;	/* Nothing allocated */
+		return NOPAGE_SIGBUS;	/* Nothing allocated */
 
 	offset = address - vma->vm_start;
 	i = (unsigned long)map->handle + offset;
 	page = (map->type == _DRM_CONSISTENT) ?
 		virt_to_page((void *)i) : vmalloc_to_page((void *)i);
 	if (!page)
-		return NOPAGE_OOM;
+		return NOPAGE_SIGBUS;
 	get_page(page);
 
 	DRM_DEBUG("shm_nopage 0x%lx\n", address);

@@ -272,7 +272,7 @@ static __inline__ struct page *drm_do_vm
 	if (address > vma->vm_end)
 		return NOPAGE_SIGBUS;	/* Disallow mremap */
 	if (!dma->pagelist)
-		return NOPAGE_OOM;	/* Nothing allocated */
+		return NOPAGE_SIGBUS;	/* Nothing allocated */
 
 	offset = address - vma->vm_start;	/* vm_[pg]off[set] should be 0 */
 	page_nr = offset >> PAGE_SHIFT;

@@ -310,7 +310,7 @@ static __inline__ struct page *drm_do_vm
 	if (address > vma->vm_end)
 		return NOPAGE_SIGBUS;	/* Disallow mremap */
 	if (!entry->pagelist)
-		return NOPAGE_OOM;	/* Nothing allocated */
+		return NOPAGE_SIGBUS;	/* Nothing allocated */
 
 	offset = address - vma->vm_start;
 	map_offset = map->offset - (unsigned long)dev->sg->virtual;

sound/core/pcm_native.c +5 -5

Index: linux-2.6/sound/core/pcm_native.c
===================================================================
@@ -3025,7 +3025,7 @@ static struct page * snd_pcm_mmap_status
 	struct page * page;
 	
 	if (substream == NULL)
-		return NOPAGE_OOM;
+		return NOPAGE_SIGBUS;
 	runtime = substream->runtime;
 	page = virt_to_page(runtime->status);
 	get_page(page);

@@ -3068,7 +3068,7 @@ static struct page * snd_pcm_mmap_contro
 	struct page * page;
 	
 	if (substream == NULL)
-		return NOPAGE_OOM;
+		return NOPAGE_SIGBUS;
 	runtime = substream->runtime;
 	page = virt_to_page(runtime->control);
 	get_page(page);

@@ -3129,18 +3129,18 @@ static struct page *snd_pcm_mmap_data_no
 	size_t dma_bytes;
 	
 	if (substream == NULL)
-		return NOPAGE_OOM;
+		return NOPAGE_SIGBUS;
 	runtime = substream->runtime;
 	offset = area->vm_pgoff << PAGE_SHIFT;
 	offset += address - area->vm_start;
-	snd_assert((offset % PAGE_SIZE) == 0, return NOPAGE_OOM);
+	snd_assert((offset % PAGE_SIZE) == 0, return NOPAGE_SIGBUS);
 	dma_bytes = PAGE_ALIGN(runtime->dma_bytes);
 	if (offset > dma_bytes - PAGE_SIZE)
 		return NOPAGE_SIGBUS;
 	if (substream->ops->page) {
 		page = substream->ops->page(substream, offset);
 		if (! page)
-			return NOPAGE_OOM;
+			return NOPAGE_OOM; /* XXX: is this really due to OOM? */
 	} else {
 		vaddr = runtime->dma_area + offset;
 		page = virt_to_page(vaddr);

sound/oss/via82cxxx_audio.c +2 -2

Index: linux-2.6/sound/oss/via82cxxx_audio.c
===================================================================
@@ -2120,8 +2120,8 @@ static struct page * via_mm_nopage (stru
 		return NOPAGE_SIGBUS; /* Disallow mremap */
 	}
         if (!card) {
-		DPRINTK ("EXIT, returning NOPAGE_OOM\n");
-		return NOPAGE_OOM;	/* Nothing allocated */
+		DPRINTK ("EXIT, returning NOPAGE_SIGBUS\n");
+		return NOPAGE_SIGBUS;	/* Nothing allocated */
 	}
 
 	pgoff = vma->vm_pgoff + ((address - vma->vm_start) >> PAGE_SHIFT);

sound/usb/usx2y/usX2Yhwdep.c +1 -1

Index: linux-2.6/sound/usb/usx2y/usX2Yhwdep.c
===================================================================
@@ -48,7 +48,7 @@ static struct page * snd_us428ctls_vm_no
 	
 	offset = area->vm_pgoff << PAGE_SHIFT;
 	offset += address - area->vm_start;
-	snd_assert((offset % PAGE_SIZE) == 0, return NOPAGE_OOM);
+	snd_assert((offset % PAGE_SIZE) == 0, return NOPAGE_SIGBUS);
 	vaddr = (char*)((struct usX2Ydev *)area->vm_private_data)->us428ctls_sharedmem + offset;
 	page = virt_to_page(vaddr);
 	get_page(page);

Nick Piggin

author

2006-10-12 14:10 UTC #9 lore raw

PATCH 5/5 oom: invoke OOM killer from pagefault handler

In reply to Nick Piggin

Rather than have the pagefault handler kill a process directly if it gets a VM_FAULT_OOM, have it call into the OOM killer.

Only converted a few architectures so far - this is just an RFC.

mm/oom_kill.c +54 ~~-29~~

Index: linux-2.6/mm/oom_kill.c
===================================================================
@@ -376,6 +376,57 @@ int unregister_oom_notifier(struct notif
 }
 EXPORT_SYMBOL_GPL(unregister_oom_notifier);
 
+/*
+ * Must be called with cpuset_lock and tasklist_lock held for read.
+ */
+void __out_of_memory(void)
+{
+	unsigned long points = 0;
+	struct task_struct *p;
+
+	if (sysctl_panic_on_oom)
+		panic("out of memory. panic_on_oom is selected\n");
+retry:
+	/*
+	 * Rambo mode: Shoot down a process and hope it solves whatever
+	 * issues we may have.
+	 */
+	p = select_bad_process(&points);
+
+	if (PTR_ERR(p) == -1UL)
+		return;
+
+	/* Found nothing?!?! Either we hang forever, or we panic. */
+	if (!p) {
+		read_unlock(&tasklist_lock);
+		cpuset_unlock();
+		panic("Out of memory and no killable processes...\n");
+	}
+
+	if (oom_kill_process(p, points, "Out of memory"))
+		goto retry;
+}
+
+/*
+ * pagefault handler calls into here because it is out of memory but
+ * doesn't know exactly how or why.
+ */
+void pagefault_out_of_memory(void)
+{
+	if (printk_ratelimit()) {
+		printk(KERN_WARNING "%s invoked oom-killer from pagefault: "
+			"oomkilladj=%d\n", current->oomkilladj);
+		dump_stack();
+		show_mem();
+	}
+
+	cpuset_lock();
+	read_lock(&tasklist_lock);
+	__out_of_memory();
+	read_unlock(&tasklist_lock);
+	cpuset_unlock();
+}
+
 /**
  * out_of_memory - kill the "best" process when we run out of memory
  *

@@ -386,8 +437,6 @@ EXPORT_SYMBOL_GPL(unregister_oom_notifie
  */
 void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order)
 {
-	struct task_struct *p;
-	unsigned long points = 0;
 	unsigned long freed = 0;
 
 	blocking_notifier_call_chain(&oom_notify_list, 0, &freed);

@@ -412,42 +461,18 @@ void out_of_memory(struct zonelist *zone
 	 */
 	switch (constrained_alloc(zonelist, gfp_mask)) {
 	case CONSTRAINT_MEMORY_POLICY:
-		oom_kill_process(current, points,
-				"No available memory (MPOL_BIND)");
+		oom_kill_process(current, 0, "No available memory (MPOL_BIND)");
 		break;
 
 	case CONSTRAINT_CPUSET:
-		oom_kill_process(current, points,
-				"No available memory in cpuset");
+		oom_kill_process(current, 0, "No available memory in cpuset");
 		break;
 
 	case CONSTRAINT_NONE:
-		if (sysctl_panic_on_oom)
-			panic("out of memory. panic_on_oom is selected\n");
-retry:
-		/*
-		 * Rambo mode: Shoot down a process and hope it solves whatever
-		 * issues we may have.
-		 */
-		p = select_bad_process(&points);
-
-		if (PTR_ERR(p) == -1UL)
-			goto out;
-
-		/* Found nothing?!?! Either we hang forever, or we panic. */
-		if (!p) {
-			read_unlock(&tasklist_lock);
-			cpuset_unlock();
-			panic("Out of memory and no killable processes...\n");
-		}
-
-		if (oom_kill_process(p, points, "Out of memory"))
-			goto retry;
-
+		__out_of_memory();
 		break;
 	}
 
-out:
 	read_unlock(&tasklist_lock);
 	cpuset_unlock();

arch/alpha/mm/fault.c +6 ~~-13~~

Index: linux-2.6/arch/alpha/mm/fault.c
===================================================================
@@ -143,7 +143,6 @@ do_page_fault(unsigned long address, uns
 			goto bad_area;
 	}
 
- survive:
 	/* If for any reason at all we couldn't handle the fault,
 	   make sure we exit gracefully rather than endlessly redo
 	   the fault.  */

@@ -190,19 +189,13 @@ do_page_fault(unsigned long address, uns
 	die_if_kernel("Oops", regs, cause, (unsigned long*)regs - 16);
 	do_exit(SIGKILL);
 
-	/* We ran out of memory, or some other thing happened to us that
-	   made us unable to handle the page fault gracefully.  */
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
  out_of_memory:
-	if (is_init(current)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk(KERN_ALERT "VM: killing process %s(%d)\n",
-	       current->comm, current->pid);
-	if (!user_mode(regs))
-		goto no_context;
-	do_exit(SIGKILL);
+	pagefault_out_of_memory();
+	return;
 
  do_sigbus:
 	/* Send a sigbus, regardless of whether we were in kernel

arch/i386/mm/fault.c +6 ~~-14~~

Index: linux-2.6/arch/i386/mm/fault.c
===================================================================
@@ -444,7 +444,6 @@ good_area:
 				goto bad_area;
 	}
 
- survive:
 	/*
 	 * If for any reason at all we couldn't handle the fault,
 	 * make sure we exit gracefully rather than endlessly redo

@@ -583,21 +582,14 @@ no_context:
 	bust_spinlocks(0);
 	do_exit(SIGKILL);
 
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
 out_of_memory:
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
 	up_read(&mm->mmap_sem);
-	if (is_init(tsk)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk("VM: killing process %s\n", tsk->comm);
-	if (error_code & 4)
-		do_exit(SIGKILL);
-	goto no_context;
+	pagefault_out_of_memory();
+	return;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);

arch/ia64/mm/fault.c +6 ~~-10~~

Index: linux-2.6/arch/ia64/mm/fault.c
===================================================================
@@ -155,7 +155,6 @@ ia64_do_page_fault (unsigned long addres
 	if ((vma->vm_flags & mask) != mask)
 		goto bad_area;
 
-  survive:
 	/*
 	 * If for any reason at all we couldn't handle the fault, make
 	 * sure we exit gracefully rather than endlessly redo the

@@ -280,13 +279,10 @@ ia64_do_page_fault (unsigned long addres
 
   out_of_memory:
 	up_read(&mm->mmap_sem);
-	if (is_init(current)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk(KERN_CRIT "VM: killing process %s\n", current->comm);
-	if (user_mode(regs))
-		do_exit(SIGKILL);
-	goto no_context;
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
+	pagefault_out_of_memory();
+	return;
 }

arch/powerpc/mm/fault.c +6 ~~-14~~

Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
@@ -342,7 +342,6 @@ good_area:
 	 * make sure we exit gracefully rather than endlessly redo
 	 * the fault.
 	 */
- survive:
 	switch (handle_mm_fault(mm, vma, address, is_write)) {
 
 	case VM_FAULT_MINOR:

@@ -380,21 +379,14 @@ bad_area_nosemaphore:
 
 	return SIGSEGV;
 
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
 out_of_memory:
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
 	up_read(&mm->mmap_sem);
-	if (is_init(current)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk("VM: killing process %s\n", current->comm);
-	if (user_mode(regs))
-		do_exit(SIGKILL);
-	return SIGKILL;
+	pagefault_out_of_memory();
+	return 0;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);

arch/x86_64/mm/fault.c +6 ~~-13~~

Index: linux-2.6/arch/x86_64/mm/fault.c
===================================================================
@@ -407,7 +407,6 @@ asmlinkage void __kprobes do_page_fault(
 	if (unlikely(in_atomic() || !mm))
 		goto bad_area_nosemaphore;
 
- again:
 	/* When running in the kernel we expect faults to occur only to
 	 * addresses in user space.  All other faults represent errors in the
 	 * kernel and should generate an OOPS.  Unfortunatly, in the case of an

@@ -574,20 +573,14 @@ no_context:
 	oops_end(flags);
 	do_exit(SIGKILL);
 
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
 out_of_memory:
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
 	up_read(&mm->mmap_sem);
-	if (is_init(current)) {
-		yield();
-		goto again;
-	}
-	printk("VM: killing process %s\n", tsk->comm);
-	if (error_code & 4)
-		do_exit(SIGKILL);
-	goto no_context;
+	pagefault_out_of_memory();
+	return;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);

include/linux/mm.h +5 -0

Index: linux-2.6/include/linux/mm.h
===================================================================
@@ -617,6 +617,11 @@ static inline int page_mapped(struct pag
  */
 #define VM_FAULT_WRITE	0x10
 
+/*
+ * Can be called by the pagefault handler when it gets a VM_FAULT_OOM.
+ */
+extern void pagefault_out_of_memory(void);
+
 #define offset_in_page(p)	((unsigned long)(p) & ~PAGE_MASK)
 
 extern void show_free_areas(void);

arch/um/kernel/trap.c +7 -8

Index: linux-2.6/arch/um/kernel/trap.c
===================================================================
@@ -75,7 +75,6 @@ good_area:
                 goto out;
 
 	do {
-survive:
 		switch (handle_mm_fault(mm, vma, address, is_write)){
 		case VM_FAULT_MINOR:
 			current->min_flt++;

@@ -119,13 +118,13 @@ out_nosemaphore:
  * us unable to handle the page fault gracefully.
  */
 out_of_memory:
-	if (is_init(current)) {
-		up_read(&mm->mmap_sem);
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	goto out;
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
+	up_read(&mm->mmap_sem);
+	pagefault_out_of_memory();
+	goto out_nosemaphore;
 }
 
 void segv_handler(int sig, union uml_pt_regs *regs)

Kirill Korotaev

2006-10-12 15:06 UTC #10 lore raw

Re: [patch 5/5] oom: invoke OOM killer from pagefault handler

Nick,

AFAICS, 1 page allocation which is done in page fault handler can fail in the only case - OOM kills current, so if we failed we should have TIF_MEMDIE and just kill current. Selecting another process for killing if page fault fails means taking another victim with the one being already killed.

my 2 cents.

Thanks,
Kirill

Quoted 366 lines

Rather than have the pagefault handler kill a process directly if it gets a VM_FAULT_OOM, have it call into the OOM killer.

Only converted a few architectures so far - this is just an RFC.

Index: linux-2.6/mm/oom_kill.c
===================================================================

mm/oom_kill.c +54 ~~-29~~

@@ -376,6 +376,57 @@ int unregister_oom_notifier(struct notif
 }
 EXPORT_SYMBOL_GPL(unregister_oom_notifier);
 
+/*
+ * Must be called with cpuset_lock and tasklist_lock held for read.
+ */
+void __out_of_memory(void)
+{
+	unsigned long points = 0;
+	struct task_struct *p;
+
+	if (sysctl_panic_on_oom)
+		panic("out of memory. panic_on_oom is selected\n");
+retry:
+	/*
+	 * Rambo mode: Shoot down a process and hope it solves whatever
+	 * issues we may have.
+	 */
+	p = select_bad_process(&points);
+
+	if (PTR_ERR(p) == -1UL)
+		return;
+
+	/* Found nothing?!?! Either we hang forever, or we panic. */
+	if (!p) {
+		read_unlock(&tasklist_lock);
+		cpuset_unlock();
+		panic("Out of memory and no killable processes...\n");
+	}
+
+	if (oom_kill_process(p, points, "Out of memory"))
+		goto retry;
+}
+
+/*
+ * pagefault handler calls into here because it is out of memory but
+ * doesn't know exactly how or why.
+ */
+void pagefault_out_of_memory(void)
+{
+	if (printk_ratelimit()) {
+		printk(KERN_WARNING "%s invoked oom-killer from pagefault: "
+			"oomkilladj=%d\n", current->oomkilladj);
+		dump_stack();
+		show_mem();
+	}
+
+	cpuset_lock();
+	read_lock(&tasklist_lock);
+	__out_of_memory();
+	read_unlock(&tasklist_lock);
+	cpuset_unlock();
+}
+
 /**
  * out_of_memory - kill the "best" process when we run out of memory
  *

@@ -386,8 +437,6 @@ EXPORT_SYMBOL_GPL(unregister_oom_notifie
  */
 void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order)
 {
-	struct task_struct *p;
-	unsigned long points = 0;
 	unsigned long freed = 0;
 
 	blocking_notifier_call_chain(&oom_notify_list, 0, &freed);

@@ -412,42 +461,18 @@ void out_of_memory(struct zonelist *zone
 	 */
 	switch (constrained_alloc(zonelist, gfp_mask)) {
 	case CONSTRAINT_MEMORY_POLICY:
-		oom_kill_process(current, points,
-				"No available memory (MPOL_BIND)");
+		oom_kill_process(current, 0, "No available memory (MPOL_BIND)");
 		break;
 
 	case CONSTRAINT_CPUSET:
-		oom_kill_process(current, points,
-				"No available memory in cpuset");
+		oom_kill_process(current, 0, "No available memory in cpuset");
 		break;
 
 	case CONSTRAINT_NONE:
-		if (sysctl_panic_on_oom)
-			panic("out of memory. panic_on_oom is selected\n");
-retry:
-		/*
-		 * Rambo mode: Shoot down a process and hope it solves whatever
-		 * issues we may have.
-		 */
-		p = select_bad_process(&points);
-
-		if (PTR_ERR(p) == -1UL)
-			goto out;
-
-		/* Found nothing?!?! Either we hang forever, or we panic. */
-		if (!p) {
-			read_unlock(&tasklist_lock);
-			cpuset_unlock();
-			panic("Out of memory and no killable processes...\n");
-		}
-
-		if (oom_kill_process(p, points, "Out of memory"))
-			goto retry;
-
+		__out_of_memory();
 		break;
 	}
 
-out:
 	read_unlock(&tasklist_lock);
 	cpuset_unlock();

Index: linux-2.6/arch/alpha/mm/fault.c
===================================================================

arch/alpha/mm/fault.c +6 ~~-13~~

@@ -143,7 +143,6 @@ do_page_fault(unsigned long address, uns
 			goto bad_area;
 	}
 
- survive:
 	/* If for any reason at all we couldn't handle the fault,
 	   make sure we exit gracefully rather than endlessly redo
 	   the fault.  */

@@ -190,19 +189,13 @@ do_page_fault(unsigned long address, uns
 	die_if_kernel("Oops", regs, cause, (unsigned long*)regs - 16);
 	do_exit(SIGKILL);
 
-	/* We ran out of memory, or some other thing happened to us that
-	   made us unable to handle the page fault gracefully.  */
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
  out_of_memory:
-	if (is_init(current)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk(KERN_ALERT "VM: killing process %s(%d)\n",
-	       current->comm, current->pid);
-	if (!user_mode(regs))
-		goto no_context;
-	do_exit(SIGKILL);
+	pagefault_out_of_memory();
+	return;
 
  do_sigbus:
 	/* Send a sigbus, regardless of whether we were in kernel

Index: linux-2.6/arch/i386/mm/fault.c
===================================================================

arch/i386/mm/fault.c +6 ~~-14~~

@@ -444,7 +444,6 @@ good_area:
 				goto bad_area;
 	}
 
- survive:
 	/*
 	 * If for any reason at all we couldn't handle the fault,
 	 * make sure we exit gracefully rather than endlessly redo

@@ -583,21 +582,14 @@ no_context:
 	bust_spinlocks(0);
 	do_exit(SIGKILL);
 
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
 out_of_memory:
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
 	up_read(&mm->mmap_sem);
-	if (is_init(tsk)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk("VM: killing process %s\n", tsk->comm);
-	if (error_code & 4)
-		do_exit(SIGKILL);
-	goto no_context;
+	pagefault_out_of_memory();
+	return;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);

Index: linux-2.6/arch/ia64/mm/fault.c
===================================================================

arch/ia64/mm/fault.c +6 ~~-10~~

@@ -155,7 +155,6 @@ ia64_do_page_fault (unsigned long addres
 	if ((vma->vm_flags & mask) != mask)
 		goto bad_area;
 
-  survive:
 	/*
 	 * If for any reason at all we couldn't handle the fault, make
 	 * sure we exit gracefully rather than endlessly redo the

@@ -280,13 +279,10 @@ ia64_do_page_fault (unsigned long addres
 
   out_of_memory:
 	up_read(&mm->mmap_sem);
-	if (is_init(current)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk(KERN_CRIT "VM: killing process %s\n", current->comm);
-	if (user_mode(regs))
-		do_exit(SIGKILL);
-	goto no_context;
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
+	pagefault_out_of_memory();
+	return;
 }

Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================

arch/powerpc/mm/fault.c +6 ~~-14~~

@@ -342,7 +342,6 @@ good_area:
 	 * make sure we exit gracefully rather than endlessly redo
 	 * the fault.
 	 */
- survive:
 	switch (handle_mm_fault(mm, vma, address, is_write)) {
 
 	case VM_FAULT_MINOR:

@@ -380,21 +379,14 @@ bad_area_nosemaphore:
 
 	return SIGSEGV;
 
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
 out_of_memory:
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
 	up_read(&mm->mmap_sem);
-	if (is_init(current)) {
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	printk("VM: killing process %s\n", current->comm);
-	if (user_mode(regs))
-		do_exit(SIGKILL);
-	return SIGKILL;
+	pagefault_out_of_memory();
+	return 0;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);

Index: linux-2.6/arch/x86_64/mm/fault.c
===================================================================

arch/x86_64/mm/fault.c +6 ~~-13~~

@@ -407,7 +407,6 @@ asmlinkage void __kprobes do_page_fault(
 	if (unlikely(in_atomic() || !mm))
 		goto bad_area_nosemaphore;
 
- again:
 	/* When running in the kernel we expect faults to occur only to
 	 * addresses in user space.  All other faults represent errors in the
 	 * kernel and should generate an OOPS.  Unfortunatly, in the case of an

@@ -574,20 +573,14 @@ no_context:
 	oops_end(flags);
 	do_exit(SIGKILL);
 
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
 out_of_memory:
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
 	up_read(&mm->mmap_sem);
-	if (is_init(current)) {
-		yield();
-		goto again;
-	}
-	printk("VM: killing process %s\n", tsk->comm);
-	if (error_code & 4)
-		do_exit(SIGKILL);
-	goto no_context;
+	pagefault_out_of_memory();
+	return;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +5 -0

@@ -617,6 +617,11 @@ static inline int page_mapped(struct pag
  */
 #define VM_FAULT_WRITE	0x10
 
+/*
+ * Can be called by the pagefault handler when it gets a VM_FAULT_OOM.
+ */
+extern void pagefault_out_of_memory(void);
+
 #define offset_in_page(p)	((unsigned long)(p) & ~PAGE_MASK)
 
 extern void show_free_areas(void);

Index: linux-2.6/arch/um/kernel/trap.c
===================================================================

arch/um/kernel/trap.c +7 -9

@@ -75,7 +75,6 @@ good_area:
                 goto out;
 
 	do {
-survive:
 		switch (handle_mm_fault(mm, vma, address, is_write)){
 		case VM_FAULT_MINOR:
 			current->min_flt++;

@@ -119,13 +118,13 @@ out_nosemaphore:
  * us unable to handle the page fault gracefully.
  */
 out_of_memory:
-	if (is_init(current)) {
-		up_read(&mm->mmap_sem);
-		yield();
-		down_read(&mm->mmap_sem);
-		goto survive;
-	}
-	goto out;
+	/*
+	 * We ran out of memory, call the OOM killer, and return to userspace
+	 * (the fault will be retried if we weren't killed)
+	 */
+	up_read(&mm->mmap_sem);
+	pagefault_out_of_memory();
+	goto out_nosemaphore;
 }
 
 void segv_handler(int sig, union uml_pt_regs *regs)
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Nick Piggin

author

2006-10-12 15:19 UTC #11 lore raw

Re: [patch 5/5] oom: invoke OOM killer from pagefault handler

Quoted 8 lines · Kirill Korotaev

Nick,
AFAICS, 1 page allocation which is done in page fault handler can fail in the only case - OOM kills current, so if we failed we should have TIF_MEMDIE and just kill current. Selecting another process for killing if page fault fails means taking another victim with the one being already killed.

Hi Kirill,

I don't quite understand you. If the page allocation fails in the fault handler, we don't want to kill current if it is marked as OOM_DISABLE or sysctl_panic_on_oom is set... imagine a critical service in a failover system.

It should be quite likely for another process to be kiled and provide enough memory to keep the system running. Presuming you have faith in the concept of the OOM killer ;)

Cheers,
Nick

Andrew Morton

2006-10-12 22:09 UTC #12 lore raw

Re: [patch 5/5] oom: invoke OOM killer from pagefault handler

Quoted 10 lines · On Thu, 12 Oct 2006 17:19:07 +0200 Nick Piggin <[email protected]> wrote:

Quoted 8 lines · Kirill Korotaev
Nick,
AFAICS, 1 page allocation which is done in page fault handler can fail in the only case - OOM kills current, so if we failed we should have TIF_MEMDIE and just kill current. Selecting another process for killing if page fault fails means taking another victim with the one being already killed.

Quoted 3 lines

Hi Kirill,
I don't quite understand you.

Kirill is claiming that the only occasion on which a pagefault handler would get an oom is when it killed itself in the oom handler.

Quoted 8 lines

If the page allocation fails in the
fault handler, we don't want to kill current if it is marked as OOM_DISABLE or sysctl_panic_on_oom is set... imagine a critical service in a failover system.
It should be quite likely for another process to be kiled and provide enough memory to keep the system running. Presuming you have faith in the concept of the OOM killer ;)

I'm a bit wobbly about this one. Some before-and-after testing results would help things along..

Nick Piggin

2006-10-13 06:45 UTC #13 lore raw

Re: [patch 5/5] oom: invoke OOM killer from pagefault handler

Quoted 15 lines · Andrew Morton

Quoted 11 lines · On Thu, 12 Oct 2006 17:19:07 +0200 Nick Piggin <[email protected]> wrote:
Quoted 9 lines · Kirill Korotaev
Nick,
AFAICS, 1 page allocation which is done in page fault handler can fail in the only case - OOM kills current, so if we failed we should have TIF_MEMDIE and just kill current. Selecting another process for killing if page fault fails means taking another victim with the one being already killed.

Quoted 8 lines

Quoted 4 lines
Hi Kirill,
I don't quite understand you.
Kirill is claiming that the only occasion on which a pagefault handler would get an oom is when it killed itself in the oom handler.

Well I don't think that should happen much. When the process gets OOM killed, it is given full access to all memory reserves, so it will be _less_ likely to go OOM maybe.

Actually if you work it through, maybe that isn't the case -- our infinite retry logic in the allocator means that non OOM killed tasks will never return NULL, while the OOM task might just use up every single free page in the system and will eventually return NULL. In this case the system is probably on death's door though, so I don't know if it is worth worrying about.

Quoted 5 lines

Quoted 5 lines
If the page allocation fails in the
fault handler, we don't want to kill current if it is marked as OOM_DISABLE or sysctl_panic_on_oom is set... imagine a critical service in a failover system.

Quoted 8 lines

Quoted 4 lines
It should be quite likely for another process to be kiled and provide enough memory to keep the system running. Presuming you have faith in the concept of the OOM killer ;)
I'm a bit wobbly about this one. Some before-and-after testing results would help things along..

I can force VM_FAULT_OOMs to happen, but it is difficult to make it happen in the real world because most fault handling paths don't allocate higher order allocations.

What I especially have in mind here is the OOM_DISABLE and panic_on_oom sysctl rather than expecting particularly much better general oom killing behaviour. Suppose you have a critical failover node or heartbeat process or something where you'd rather the system to panic and reboot instead of doing something silly...

Send instant messages to your online friends http://au.messenger.yahoo.com

Nick Piggin

2006-10-13 06:47 UTC #14 lore raw

Re: [patch 5/5] oom: invoke OOM killer from pagefault handler

Quoted 5 lines · Nick Piggin

What I especially have in mind here is the OOM_DISABLE and panic_on_oom sysctl rather than expecting particularly much better general oom killing behaviour. Suppose you have a critical failover node or heartbeat process or something where you'd rather the system to panic and reboot instead of doing something silly...

Oh, I already said that.

Well anyway, I'm not sure exactly how people use these tunables, but I expect those that do, _really_ want them to work.

Send instant messages to your online friends http://au.messenger.yahoo.com