[patch -mm 0/5] oom: fixes and cleanup

[patch -mm 0/5] oom: fixes and cleanup

[David Rientjes]

This patchset fixes a couple of issues with the oom killer, namely
tasklist_lock locking requirements and sending SIGKILLs to already
exiting tasks.  It also cleans up a couple functions, __oom_kill_task()
and oom_badness().

This patchset is based on mmotm-2010-03-24-14-48.

Many thanks to Oleg Nesterov <oleg@redhat.com> for interest in this work.
---
 fs/proc/base.c      |    5 +---
 include/linux/oom.h |    2 +-
 mm/oom_kill.c       |   58 ++++++++++++++++++++++-----------------------------
 3 files changed, 27 insertions(+), 38 deletions(-)

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

[patch -mm 1/5 v2] oom: hold tasklist_lock when dumping tasks

[David Rientjes]

dump_header() always requires tasklist_lock to be held because it calls 
dump_tasks() which iterates through the tasklist.  There are a few places
where this isn't maintained, so make sure tasklist_lock is always held
whenever calling dump_header().

This also fixes the pagefault case where oom_kill_process() is called on
current without tasklist_lock.  It is necessary to hold a readlock for
both calling dump_header() and iterating its children.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/oom_kill.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -395,6 +395,9 @@ static void dump_tasks(const struct mem_cgroup *mem)
 	} while_each_thread(g, p);
 }
 
+/*
+ * Call with tasklist_lock read-locked.
+ */
 static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
 							struct mem_cgroup *mem)
 {
@@ -641,8 +644,8 @@ retry:
 
 	/* Found nothing?!?! Either we hang forever, or we panic. */
 	if (!p) {
-		read_unlock(&tasklist_lock);
 		dump_header(NULL, gfp_mask, order, NULL);
+		read_unlock(&tasklist_lock);
 		panic("Out of memory and no killable processes...\n");
 	}
 
@@ -675,11 +678,6 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
 		/* Got some memory back in the last second. */
 		return;
 
-	if (sysctl_panic_on_oom == 2) {
-		dump_header(NULL, gfp_mask, order, NULL);
-		panic("out of memory. Compulsory panic_on_oom is selected.\n");
-	}
-
 	/*
 	 * Check if there were limitations on the allocation (only relevant for
 	 * NUMA) that may require different handling.
@@ -688,15 +686,12 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
 						&totalpages);
 	read_lock(&tasklist_lock);
 	if (unlikely(sysctl_panic_on_oom)) {
-		/*
-		 * panic_on_oom only affects CONSTRAINT_NONE, the kernel
-		 * should not panic for cpuset or mempolicy induced memory
-		 * failures.
-		 */
-		if (constraint == CONSTRAINT_NONE) {
+		if (sysctl_panic_on_oom == 2 || constraint == CONSTRAINT_NONE) {
 			dump_header(NULL, gfp_mask, order, NULL);
 			read_unlock(&tasklist_lock);
-			panic("Out of memory: panic_on_oom is enabled\n");
+			panic("Out of memory: %s panic_on_oom is enabled\n",
+				sysctl_panic_on_oom == 2 ? "compulsory" :
+							   "system-wide");
 		}
 	}
 	__out_of_memory(gfp_mask, order, totalpages, constraint, nodemask);
@@ -724,8 +719,10 @@ void pagefault_out_of_memory(void)
 
 	if (try_set_system_oom()) {
 		constrained_alloc(NULL, 0, NULL, &totalpages);
+		read_lock(&tasklist_lock);
 		err = oom_kill_process(current, 0, 0, 0, totalpages, NULL,
 					"Out of memory (pagefault)");
+		read_unlock(&tasklist_lock);
 		if (err)
 			out_of_memory(NULL, 0, 0, NULL);
 		clear_system_oom();

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

Re: [patch -mm 1/5 v2] oom: hold tasklist_lock when dumping tasks

[KAMEZAWA Hiroyuki]

On Thu, 1 Apr 2010 12:44:28 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> dump_header() always requires tasklist_lock to be held because it calls 
> dump_tasks() which iterates through the tasklist.  There are a few places
> where this isn't maintained, so make sure tasklist_lock is always held
> whenever calling dump_header().
> 
> This also fixes the pagefault case where oom_kill_process() is called on
> current without tasklist_lock.  It is necessary to hold a readlock for
> both calling dump_header() and iterating its children.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: David Rientjes <rientjes@google.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

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

[patch -mm 2/5 v2] oom: give current access to memory reserves if it has been killed

[David Rientjes]

It's possible to livelock the page allocator if a thread has mm->mmap_sem and 
fails to make forward progress because the oom killer selects another thread 
sharing the same ->mm to kill that cannot exit until the semaphore is dropped.

The oom killer will not kill multiple tasks at the same time; each oom killed 
task must exit before another task may be killed.  Thus, if one thread is 
holding mm->mmap_sem and cannot allocate memory, all threads sharing the same 
->mm are blocked from exiting as well.  In the oom kill case, that means the
thread holding mm->mmap_sem will never free additional memory since it cannot
get access to memory reserves and the thread that depends on it with access to
memory reserves cannot exit because it cannot acquire the semaphore.  Thus,
the page allocators livelocks.

When the oom killer is called and current happens to have a pending SIGKILL,
this patch automatically gives it access to memory reserves and returns.  Upon
returning to the page allocator, its allocation will hopefully succeed so it
can quickly exit and free its memory.  If not, the page allocator will fail
the allocation if it is not __GFP_NOFAIL.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/oom_kill.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -679,6 +679,16 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
 		return;
 
 	/*
+	 * If current has a pending SIGKILL, then automatically select it.  The
+	 * goal is to allow it to allocate so that it may quickly exit and free
+	 * its memory.
+	 */
+	if (fatal_signal_pending(current)) {
+		set_tsk_thread_flag(current, TIF_MEMDIE);
+		return;
+	}
+
+	/*
 	 * Check if there were limitations on the allocation (only relevant for
 	 * NUMA) that may require different handling.
 	 */

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

Re: [patch -mm 2/5 v2] oom: give current access to memory reserves if it has been killed

[KAMEZAWA Hiroyuki]

On Thu, 1 Apr 2010 12:44:31 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> It's possible to livelock the page allocator if a thread has mm->mmap_sem and 
> fails to make forward progress because the oom killer selects another thread 
> sharing the same ->mm to kill that cannot exit until the semaphore is dropped.
> 
> The oom killer will not kill multiple tasks at the same time; each oom killed 
> task must exit before another task may be killed.  Thus, if one thread is 
> holding mm->mmap_sem and cannot allocate memory, all threads sharing the same 
> ->mm are blocked from exiting as well.  In the oom kill case, that means the
> thread holding mm->mmap_sem will never free additional memory since it cannot
> get access to memory reserves and the thread that depends on it with access to
> memory reserves cannot exit because it cannot acquire the semaphore.  Thus,
> the page allocators livelocks.
> 
> When the oom killer is called and current happens to have a pending SIGKILL,
> this patch automatically gives it access to memory reserves and returns.  Upon
> returning to the page allocator, its allocation will hopefully succeed so it
> can quickly exit and free its memory.  If not, the page allocator will fail
> the allocation if it is not __GFP_NOFAIL.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


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

[patch -mm 3/5] oom: avoid sending exiting tasks a SIGKILL

[David Rientjes]

It's unnecessary to SIGKILL a task that is already PF_EXITING and can
actually cause a NULL pointer dereference of the sighand if it has
already been detached.  Instead, simply set TIF_MEMDIE so it has access
to memory reserves and can quickly exit as the comment implies.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/oom_kill.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -462,7 +462,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	 * its children or threads, just set TIF_MEMDIE so it can die quickly
 	 */
 	if (p->flags & PF_EXITING) {
-		__oom_kill_task(p);
+		set_tsk_thread_flag(p, TIF_MEMDIE);
 		return 0;
 	}
 

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

Re: [patch -mm 3/5] oom: avoid sending exiting tasks a SIGKILL

[KAMEZAWA Hiroyuki]

On Thu, 1 Apr 2010 12:44:34 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> It's unnecessary to SIGKILL a task that is already PF_EXITING and can
> actually cause a NULL pointer dereference of the sighand if it has
> already been detached.  Instead, simply set TIF_MEMDIE so it has access
> to memory reserves and can quickly exit as the comment implies.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


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

[patch -mm 4/5] oom: cleanup oom_kill_task

[David Rientjes]

__oom_kill_task() only has a single caller, so merge it into that
function.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/oom_kill.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -415,17 +415,6 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
 		dump_tasks(mem);
 }
 
-/*
- * Give the oom killed task high priority and access to memory reserves so that
- * it may quickly exit and free its memory.
- */
-static void __oom_kill_task(struct task_struct *p)
-{
-	p->rt.time_slice = HZ;
-	set_tsk_thread_flag(p, TIF_MEMDIE);
-	force_sig(SIGKILL, p);
-}
-
 #define K(x) ((x) << (PAGE_SHIFT-10))
 static int oom_kill_task(struct task_struct *p)
 {
@@ -440,7 +429,9 @@ static int oom_kill_task(struct task_struct *p)
 	       K(get_mm_counter(p->mm, MM_FILEPAGES)));
 	task_unlock(p);
 
-	__oom_kill_task(p);
+	p->rt.time_slice = HZ;
+	set_tsk_thread_flag(p, TIF_MEMDIE);
+	force_sig(SIGKILL, p);
 	return 0;
 }
 #undef K

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

Re: [patch -mm 4/5] oom: cleanup oom_kill_task

[KAMEZAWA Hiroyuki]

On Thu, 1 Apr 2010 12:44:36 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> __oom_kill_task() only has a single caller, so merge it into that
> function.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

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

[patch -mm 5/5] oom: cleanup oom_badness

[David Rientjes]

oom_badness() no longer uses its uptime formal, so it can be removed.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 fs/proc/base.c      |    5 +----
 include/linux/oom.h |    2 +-
 mm/oom_kill.c       |   12 +++---------
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -431,17 +431,14 @@ static const struct file_operations proc_lstats_operations = {
 static int proc_oom_score(struct task_struct *task, char *buffer)
 {
 	unsigned long points;
-	struct timespec uptime;
 
-	do_posix_clock_monotonic_gettime(&uptime);
 	read_lock(&tasklist_lock);
 	points = oom_badness(task->group_leader,
 				global_page_state(NR_INACTIVE_ANON) +
 				global_page_state(NR_ACTIVE_ANON) +
 				global_page_state(NR_INACTIVE_FILE) +
 				global_page_state(NR_ACTIVE_FILE) +
-				total_swap_pages,
-				uptime.tv_sec);
+				total_swap_pages);
 	read_unlock(&tasklist_lock);
 	return sprintf(buffer, "%lu\n", points);
 }
diff --git a/include/linux/oom.h b/include/linux/oom.h
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -41,7 +41,7 @@ enum oom_constraint {
 };
 
 extern unsigned int oom_badness(struct task_struct *p,
-			unsigned long totalpages, unsigned long uptime);
+			unsigned long totalpages);
 extern int try_set_zone_oom(struct zonelist *zonelist, gfp_t gfp_flags);
 extern void clear_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
 
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -133,14 +133,12 @@ static unsigned long oom_forkbomb_penalty(struct task_struct *tsk)
  * oom_badness - heuristic function to determine which candidate task to kill
  * @p: task struct of which task we should calculate
  * @totalpages: total present RAM allowed for page allocation
- * @uptime: current uptime in seconds
  *
  * The heuristic for determining which task to kill is made to be as simple and
  * predictable as possible.  The goal is to return the highest value for the
  * task consuming the most memory to avoid subsequent oom conditions.
  */
-unsigned int oom_badness(struct task_struct *p, unsigned long totalpages,
-							unsigned long uptime)
+unsigned int oom_badness(struct task_struct *p, unsigned long totalpages)
 {
 	struct mm_struct *mm;
 	int points;
@@ -283,10 +281,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 {
 	struct task_struct *p;
 	struct task_struct *chosen = NULL;
-	struct timespec uptime;
 	*ppoints = 0;
 
-	do_posix_clock_monotonic_gettime(&uptime);
 	for_each_process(p) {
 		unsigned int points;
 
@@ -339,7 +335,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 			continue;
 
-		points = oom_badness(p, totalpages, uptime.tv_sec);
+		points = oom_badness(p, totalpages);
 		if (points > *ppoints || !chosen) {
 			chosen = p;
 			*ppoints = points;
@@ -443,7 +439,6 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	struct task_struct *victim = p;
 	struct task_struct *c;
 	unsigned int victim_points = 0;
-	struct timespec uptime;
 
 	if (printk_ratelimit())
 		dump_header(p, gfp_mask, order, mem);
@@ -460,7 +455,6 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	pr_err("%s: Kill process %d (%s) with score %d or sacrifice child\n",
 		message, task_pid_nr(p), p->comm, points);
 
-	do_posix_clock_monotonic_gettime(&uptime);
 	/* Try to sacrifice the worst child first */
 	list_for_each_entry(c, &p->children, sibling) {
 		unsigned int cpoints;
@@ -471,7 +465,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			continue;
 
 		/* oom_badness() returns 0 if the thread is unkillable */
-		cpoints = oom_badness(c, totalpages, uptime.tv_sec);
+		cpoints = oom_badness(c, totalpages);
 		if (cpoints > victim_points) {
 			victim = c;
 			victim_points = cpoints;

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

Re: [patch -mm 5/5] oom: cleanup oom_badness

[KAMEZAWA Hiroyuki]

On Thu, 1 Apr 2010 12:44:39 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> oom_badness() no longer uses its uptime formal, so it can be removed.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: David Rientjes <rientjes@google.com>

okay. BTW, only this patch has to depend on mmotm ?

Reviewd-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


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

Re: [patch -mm 5/5] oom: cleanup oom_badness

[David Rientjes]

On Fri, 2 Apr 2010, KAMEZAWA Hiroyuki wrote:

> > oom_badness() no longer uses its uptime formal, so it can be removed.
> > 
> > Reported-by: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: David Rientjes <rientjes@google.com>
> 
> okay. BTW, only this patch has to depend on mmotm ?
> 

Right, I hope my usage of "-mm" in the subject makes it clear, sorry if it 
was confusing.

> Reviewd-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 

Thanks for your reviews of this patchset!

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

Re: [patch -mm 0/5] oom: fixes and cleanup

[Oleg Nesterov]

On 04/01, David Rientjes wrote:
>
> This patchset fixes a couple of issues with the oom killer, namely
> tasklist_lock locking requirements and sending SIGKILLs to already
> exiting tasks.  It also cleans up a couple functions, __oom_kill_task()
> and oom_badness().

The whole series looks good to me.

Thanks David.

Oleg.

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

Re: [patch -mm 0/5] oom: fixes and cleanup

[KOSAKI Motohiro]

> On 04/01, David Rientjes wrote:
> >
> > This patchset fixes a couple of issues with the oom killer, namely
> > tasklist_lock locking requirements and sending SIGKILLs to already
> > exiting tasks.  It also cleans up a couple functions, __oom_kill_task()
> > and oom_badness().
> 
> The whole series looks good to me.
> 
> Thanks David.

sorry for the delay. recently I'm very busy and now I still have 
>1000 unreaded mail. I haven't read this thread completely. but yes,
I also ack this series.

I have no doubt I still have a lot of yet reviewed patch. I'm going to
assimilate it as far as fast.

thanks.


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