[patch v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[patch v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[David Rientjes]

The oom killer's goal is to kill a memory-hogging task so that it may
exit, free its memory, and allow the current context to allocate the
memory that triggered it in the first place.  Thus, killing a task is
pointless if other threads sharing its mm cannot be killed because of its
/proc/pid/oom_adj or /proc/pid/oom_score_adj value.

This patch checks all user threads on the system to determine whether
oom_badness(p) should return 0 for p, which means it should not be killed.
If a thread shares p's mm and is unkillable, p is considered to be
unkillable as well.

Kthreads are not considered toward this rule since they only temporarily
assume a task's mm via use_mm().

Signed-off-by: David Rientjes <rientjes@google.com>
---
 v2: change do_each_thread() to for_each_process() as suggested by Oleg.

 It's actually not possible to move this logic to oom_kill_task() because
 it's racy: oom_badness() is not a constant score and depends on the state 
 of the VM when it is called.  This leads to unnecessarily panicking the 
 machine in that case as wel as when the same child to sacrifice is 
 repeatedly selected in oom_kill_process() based on the parent's badness 
 score.

 mm/oom_kill.c |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -83,6 +83,25 @@ static bool has_intersects_mems_allowed(struct task_struct *tsk,
 #endif /* CONFIG_NUMA */
 
 /*
+ * Determines whether an mm is unfreeable since a user thread attached to
+ * it cannot be killed.  Kthreads only temporarily assume a thread's mm,
+ * so they are not considered.
+ *
+ * mm need not be protected by task_lock() since it will not be
+ * dereferened.
+ */
+static bool is_mm_unfreeable(struct mm_struct *mm)
+{
+	struct task_struct *p;
+
+	for_each_process(p)
+		if (p->mm == mm && !(p->flags & PF_KTHREAD) &&
+		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
+			return true;
+	return false;
+}
+
+/*
  * If this is a system OOM (not a memcg OOM) and the task selected to be
  * killed is not already running at high (RT) priorities, speed up the
  * recovery by boosting the dying task to the lowest FIFO priority.
@@ -160,12 +179,7 @@ unsigned int oom_badness(struct task_struct *p, struct mem_cgroup *mem,
 	p = find_lock_task_mm(p);
 	if (!p)
 		return 0;
-
-	/*
-	 * Shortcut check for OOM_SCORE_ADJ_MIN so the entire heuristic doesn't
-	 * need to be executed for something that cannot be killed.
-	 */
-	if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
+	if (is_mm_unfreeable(p->mm)) {
 		task_unlock(p);
 		return 0;
 	}
@@ -675,7 +689,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
 	read_lock(&tasklist_lock);
 	if (sysctl_oom_kill_allocating_task &&
 	    !oom_unkillable_task(current, NULL, nodemask) &&
-	    (current->signal->oom_adj != OOM_DISABLE)) {
+	    !is_mm_unfreeable(current->mm)) {
 		/*
 		 * oom_kill_process() needs tasklist_lock held.  If it returns
 		 * non-zero, current could not be killed so we must fallback to

--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[David Rientjes]

It's necessary to kill all threads that share an oom killed task's mm if
the goal is to lead to future memory freeing.

This patch reintroduces the code removed in 8c5cd6f3 (oom: oom_kill
doesn't kill vfork parent (or child)) since it is obsoleted.

It's now guaranteed that any task passed to oom_kill_task() does not
share an mm with any thread that is unkillable.  Thus, we're safe to
issue a SIGKILL to any thread sharing the same mm.

This is especially necessary to solve an mm->mmap_sem livelock issue
whereas an oom killed thread must acquire the lock in the exit path while
another thread is holding it in the page allocator while trying to
allocate memory itself (and will preempt the oom killer since a task was
already killed).  Since tasks with pending fatal signals are now granted
access to memory reserves, the thread holding the lock may quickly
allocate and release the lock so that the oom killed task may exit.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 v2: kill all threads in other thread groups before killing p to ensure
     it doesn't preemptively exit while still iterating through the
     tasklist and comparing unprotected mm pointers, as suggested by Oleg.

 mm/oom_kill.c |   20 ++++++++++++++++++++
 1 files changed, 20 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
@@ -414,17 +414,37 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
 #define K(x) ((x) << (PAGE_SHIFT-10))
 static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
 {
+	struct task_struct *q;
+	struct mm_struct *mm;
+
 	p = find_lock_task_mm(p);
 	if (!p) {
 		task_unlock(p);
 		return 1;
 	}
+
+	/* mm cannot be safely dereferenced after task_unlock(p) */
+	mm = p->mm;
+
 	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
 		task_pid_nr(p), p->comm, K(p->mm->total_vm),
 		K(get_mm_counter(p->mm, MM_ANONPAGES)),
 		K(get_mm_counter(p->mm, MM_FILEPAGES)));
 	task_unlock(p);
 
+	/*
+	 * Kill all processes sharing p->mm in other thread groups, if any.
+	 * They don't get access to memory reserves or a higher scheduler
+	 * priority, though, to avoid depletion of all memory or task
+	 * starvation.  This prevents mm->mmap_sem livelock when an oom killed
+	 * task cannot exit because it requires the semaphore and its contended
+	 * by another thread trying to allocate memory itself.  That thread will
+	 * now get access to memory reserves since it has a pending fatal
+	 * signal.
+	 */
+	for_each_process(q)
+		if (q->mm == mm && !same_thread_group(q, p))
+			force_sig(SIGKILL, q);
 
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 	force_sig(SIGKILL, p);

--
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 v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[KAMEZAWA Hiroyuki]

On Mon, 16 Aug 2010 18:15:57 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> The oom killer's goal is to kill a memory-hogging task so that it may
> exit, free its memory, and allow the current context to allocate the
> memory that triggered it in the first place.  Thus, killing a task is
> pointless if other threads sharing its mm cannot be killed because of its
> /proc/pid/oom_adj or /proc/pid/oom_score_adj value.
> 
> This patch checks all user threads on the system to determine whether
> oom_badness(p) should return 0 for p, which means it should not be killed.
> If a thread shares p's mm and is unkillable, p is considered to be
> unkillable as well.
> 
> Kthreads are not considered toward this rule since they only temporarily
> assume a task's mm via use_mm().
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

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

Thank you. BTW, do you have good idea for speed-up ?
This code seems terribly slow when a system has many processes.



> ---
>  v2: change do_each_thread() to for_each_process() as suggested by Oleg.
> 
>  It's actually not possible to move this logic to oom_kill_task() because
>  it's racy: oom_badness() is not a constant score and depends on the state 
>  of the VM when it is called.  This leads to unnecessarily panicking the 
>  machine in that case as wel as when the same child to sacrifice is 
>  repeatedly selected in oom_kill_process() based on the parent's badness 
>  score.
> 
>  mm/oom_kill.c |   28 +++++++++++++++++++++-------
>  1 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -83,6 +83,25 @@ static bool has_intersects_mems_allowed(struct task_struct *tsk,
>  #endif /* CONFIG_NUMA */
>  
>  /*
> + * Determines whether an mm is unfreeable since a user thread attached to
> + * it cannot be killed.  Kthreads only temporarily assume a thread's mm,
> + * so they are not considered.
> + *
> + * mm need not be protected by task_lock() since it will not be
> + * dereferened.
> + */
> +static bool is_mm_unfreeable(struct mm_struct *mm)
> +{
> +	struct task_struct *p;
> +
> +	for_each_process(p)
> +		if (p->mm == mm && !(p->flags & PF_KTHREAD) &&
> +		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> +			return true;
> +	return false;
> +}
> +
> +/*
>   * If this is a system OOM (not a memcg OOM) and the task selected to be
>   * killed is not already running at high (RT) priorities, speed up the
>   * recovery by boosting the dying task to the lowest FIFO priority.
> @@ -160,12 +179,7 @@ unsigned int oom_badness(struct task_struct *p, struct mem_cgroup *mem,
>  	p = find_lock_task_mm(p);
>  	if (!p)
>  		return 0;
> -
> -	/*
> -	 * Shortcut check for OOM_SCORE_ADJ_MIN so the entire heuristic doesn't
> -	 * need to be executed for something that cannot be killed.
> -	 */
> -	if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
> +	if (is_mm_unfreeable(p->mm)) {
>  		task_unlock(p);
>  		return 0;
>  	}
> @@ -675,7 +689,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
>  	read_lock(&tasklist_lock);
>  	if (sysctl_oom_kill_allocating_task &&
>  	    !oom_unkillable_task(current, NULL, nodemask) &&
> -	    (current->signal->oom_adj != OOM_DISABLE)) {
> +	    !is_mm_unfreeable(current->mm)) {
>  		/*
>  		 * oom_kill_process() needs tasklist_lock held.  If it returns
>  		 * non-zero, current could not be killed so we must fallback to
> 

--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[KAMEZAWA Hiroyuki]

On Mon, 16 Aug 2010 18:16:08 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> It's necessary to kill all threads that share an oom killed task's mm if
> the goal is to lead to future memory freeing.
> 
> This patch reintroduces the code removed in 8c5cd6f3 (oom: oom_kill
> doesn't kill vfork parent (or child)) since it is obsoleted.
> 
> It's now guaranteed that any task passed to oom_kill_task() does not
> share an mm with any thread that is unkillable.  Thus, we're safe to
> issue a SIGKILL to any thread sharing the same mm.
> 
> This is especially necessary to solve an mm->mmap_sem livelock issue
> whereas an oom killed thread must acquire the lock in the exit path while
> another thread is holding it in the page allocator while trying to
> allocate memory itself (and will preempt the oom killer since a task was
> already killed).  Since tasks with pending fatal signals are now granted
> access to memory reserves, the thread holding the lock may quickly
> allocate and release the lock so that the oom killed task may exit.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Acked-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 v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[David Rientjes]

On Wed, 18 Aug 2010, KAMEZAWA Hiroyuki wrote:

> > The oom killer's goal is to kill a memory-hogging task so that it may
> > exit, free its memory, and allow the current context to allocate the
> > memory that triggered it in the first place.  Thus, killing a task is
> > pointless if other threads sharing its mm cannot be killed because of its
> > /proc/pid/oom_adj or /proc/pid/oom_score_adj value.
> > 
> > This patch checks all user threads on the system to determine whether
> > oom_badness(p) should return 0 for p, which means it should not be killed.
> > If a thread shares p's mm and is unkillable, p is considered to be
> > unkillable as well.
> > 
> > Kthreads are not considered toward this rule since they only temporarily
> > assume a task's mm via use_mm().
> > 
> > Signed-off-by: David Rientjes <rientjes@google.com>
> 
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 

Thanks!

> Thank you. BTW, do you have good idea for speed-up ?
> This code seems terribly slow when a system has many processes.
> 

I was thinking about adding an "unsinged long oom_kill_disable_count" to 
struct mm_struct that would atomically increment anytime a task attached 
to it had a signal->oom_score_adj of OOM_SCORE_ADJ_MIN.

The proc handler when changing /proc/pid/oom_score_adj would inc or dec 
the counter depending on the new value, and exit_mm() would dec the 
counter if current->signal->oom_score_adj is OOM_SCORE_ADJ_MIN.

What do you think?

--
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 v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[KAMEZAWA Hiroyuki]

On Tue, 17 Aug 2010 19:36:02 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> On Wed, 18 Aug 2010, KAMEZAWA Hiroyuki wrote:
> 
> > > The oom killer's goal is to kill a memory-hogging task so that it may
> > > exit, free its memory, and allow the current context to allocate the
> > > memory that triggered it in the first place.  Thus, killing a task is
> > > pointless if other threads sharing its mm cannot be killed because of its
> > > /proc/pid/oom_adj or /proc/pid/oom_score_adj value.
> > > 
> > > This patch checks all user threads on the system to determine whether
> > > oom_badness(p) should return 0 for p, which means it should not be killed.
> > > If a thread shares p's mm and is unkillable, p is considered to be
> > > unkillable as well.
> > > 
> > > Kthreads are not considered toward this rule since they only temporarily
> > > assume a task's mm via use_mm().
> > > 
> > > Signed-off-by: David Rientjes <rientjes@google.com>
> > 
> > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> 
> Thanks!
> 
> > Thank you. BTW, do you have good idea for speed-up ?
> > This code seems terribly slow when a system has many processes.
> > 
> 
> I was thinking about adding an "unsinged long oom_kill_disable_count" to 
> struct mm_struct that would atomically increment anytime a task attached 
> to it had a signal->oom_score_adj of OOM_SCORE_ADJ_MIN.
> 
> The proc handler when changing /proc/pid/oom_score_adj would inc or dec 
> the counter depending on the new value, and exit_mm() would dec the 
> counter if current->signal->oom_score_adj is OOM_SCORE_ADJ_MIN.
> 
> What do you think?
> 

Hmm. I want to make hooks to "exit" small. 

One idea is.

add a new member
		mm->unkiilable_by_oom_jiffies.

And add
> +static bool is_mm_unfreeable(struct mm_struct *mm)
> +{
> +	struct task_struct *p;
> +
	if (mm->unkillable_by_oom_jiffies < jiffies)
		return true;

> +	for_each_process(p)
> +		if (p->mm == mm && !(p->flags & PF_KTHREAD) &&
> +		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) 

			mm->unkillable_by_oom_jiffies = jiffies + HZ;

> +			return true;
> +	return false;
> +}+static bool is_mm_unfreeable(struct mm_struct *mm)


Maybe no new lock is required and this not-accurate one will work enough.

Thanks,
-Kame

--
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 v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[David Rientjes]

On Wed, 18 Aug 2010, KAMEZAWA Hiroyuki wrote:

> > I was thinking about adding an "unsinged long oom_kill_disable_count" to 
> > struct mm_struct that would atomically increment anytime a task attached 
> > to it had a signal->oom_score_adj of OOM_SCORE_ADJ_MIN.
> > 
> > The proc handler when changing /proc/pid/oom_score_adj would inc or dec 
> > the counter depending on the new value, and exit_mm() would dec the 
> > counter if current->signal->oom_score_adj is OOM_SCORE_ADJ_MIN.
> > 
> > What do you think?
> > 
> 
> Hmm. I want to make hooks to "exit" small. 
> 


Is it worth adding

	if (unlikely(current->signal->oom_score_adj == OOM_SCORE_ADJ_MIN))
		atomic_dec(&current->mm->oom_disable_count);

to exit_mm() under task_lock() to avoid the O(n^2) select_bad_process() on 
oom?  Or do you think that's too expensive?

> One idea is.
> 
> add a new member
> 		mm->unkiilable_by_oom_jiffies.
> 
> And add
> > +static bool is_mm_unfreeable(struct mm_struct *mm)
> > +{
> > +	struct task_struct *p;
> > +
> 	if (mm->unkillable_by_oom_jiffies < jiffies)
> 		return true;
> 
> > +	for_each_process(p)
> > +		if (p->mm == mm && !(p->flags & PF_KTHREAD) &&
> > +		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) 
> 
> 			mm->unkillable_by_oom_jiffies = jiffies + HZ;
> 
> > +			return true;
> > +	return false;
> > +}+static bool is_mm_unfreeable(struct mm_struct *mm)
> 

This probably isn't fast enough for the common case, which is when no 
tasks for "mm" have an oom_score_adj of OOM_SCORE_ADJ_MIN, since it still 
iterates through every task.

--
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 v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[KAMEZAWA Hiroyuki]

On Tue, 17 Aug 2010 20:43:10 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> On Wed, 18 Aug 2010, KAMEZAWA Hiroyuki wrote:
> 
> > > I was thinking about adding an "unsinged long oom_kill_disable_count" to 
> > > struct mm_struct that would atomically increment anytime a task attached 
> > > to it had a signal->oom_score_adj of OOM_SCORE_ADJ_MIN.
> > > 
> > > The proc handler when changing /proc/pid/oom_score_adj would inc or dec 
> > > the counter depending on the new value, and exit_mm() would dec the 
> > > counter if current->signal->oom_score_adj is OOM_SCORE_ADJ_MIN.
> > > 
> > > What do you think?
> > > 
> > 
> > Hmm. I want to make hooks to "exit" small. 
> > 
> 
> 
> Is it worth adding
> 
> 	if (unlikely(current->signal->oom_score_adj == OOM_SCORE_ADJ_MIN))
> 		atomic_dec(&current->mm->oom_disable_count);
> 
> to exit_mm() under task_lock() to avoid the O(n^2) select_bad_process() on 
> oom?  Or do you think that's too expensive?
> 

Hmm, if this coutner is changed only under down_write(mmap_sem),
simple 'int' counter is enough quick. 

> > One idea is.
> > 
> > add a new member
> > 		mm->unkiilable_by_oom_jiffies.
> > 
> > And add
> > > +static bool is_mm_unfreeable(struct mm_struct *mm)
> > > +{
> > > +	struct task_struct *p;
> > > +
> > 	if (mm->unkillable_by_oom_jiffies < jiffies)
> > 		return true;
> > 
> > > +	for_each_process(p)
> > > +		if (p->mm == mm && !(p->flags & PF_KTHREAD) &&
> > > +		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) 
> > 
> > 			mm->unkillable_by_oom_jiffies = jiffies + HZ;
> > 
> > > +			return true;
> > > +	return false;
> > > +}+static bool is_mm_unfreeable(struct mm_struct *mm)
> > 
> 
> This probably isn't fast enough for the common case, which is when no 
> tasks for "mm" have an oom_score_adj of OOM_SCORE_ADJ_MIN, since it still 
> iterates through every task.
> 
you're right. 


Thanks,
-Kame

--
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 v2 1/2] oom: avoid killing a task if a thread sharing its mm cannot be killed

[David Rientjes]

On Wed, 18 Aug 2010, KAMEZAWA Hiroyuki wrote:

> > Is it worth adding
> > 
> > 	if (unlikely(current->signal->oom_score_adj == OOM_SCORE_ADJ_MIN))
> > 		atomic_dec(&current->mm->oom_disable_count);
> > 
> > to exit_mm() under task_lock() to avoid the O(n^2) select_bad_process() on 
> > oom?  Or do you think that's too expensive?
> > 
> 
> Hmm, if this coutner is changed only under down_write(mmap_sem),
> simple 'int' counter is enough quick. 
> 

task->mm->oom_disable_count would be protected by task_lock(task) to pin 
the ->mm, which we already take in exit_mm() to set task->mm to NULL.  We 
can take task_lock() in the proc handler, oom killer, and exec() paths 
where we're interested in the accounting.

--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[KOSAKI Motohiro]

> It's necessary to kill all threads that share an oom killed task's mm if
> the goal is to lead to future memory freeing.
> 
> This patch reintroduces the code removed in 8c5cd6f3 (oom: oom_kill
> doesn't kill vfork parent (or child)) since it is obsoleted.
> 
> It's now guaranteed that any task passed to oom_kill_task() does not
> share an mm with any thread that is unkillable.  Thus, we're safe to
> issue a SIGKILL to any thread sharing the same mm.

correct.

> 
> This is especially necessary to solve an mm->mmap_sem livelock issue
> whereas an oom killed thread must acquire the lock in the exit path while
> another thread is holding it in the page allocator while trying to
> allocate memory itself (and will preempt the oom killer since a task was
> already killed).  Since tasks with pending fatal signals are now granted
> access to memory reserves, the thread holding the lock may quickly
> allocate and release the lock so that the oom killed task may exit.

I can't understand this sentence. mm sharing is happen when vfork, That
said, parent process is always sleeping. why do we need to worry that parent
process is holding mmap_sem?

Your change seems to don't change multi threading behavior. it only change
vfork() process behavior.


> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  v2: kill all threads in other thread groups before killing p to ensure
>      it doesn't preemptively exit while still iterating through the
>      tasklist and comparing unprotected mm pointers, as suggested by Oleg.
> 
>  mm/oom_kill.c |   20 ++++++++++++++++++++
>  1 files changed, 20 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
> @@ -414,17 +414,37 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
>  #define K(x) ((x) << (PAGE_SHIFT-10))
>  static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
>  {
> +	struct task_struct *q;
> +	struct mm_struct *mm;
> +
>  	p = find_lock_task_mm(p);
>  	if (!p) {
>  		task_unlock(p);
>  		return 1;
>  	}
> +
> +	/* mm cannot be safely dereferenced after task_unlock(p) */
> +	mm = p->mm;
> +
>  	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
>  		task_pid_nr(p), p->comm, K(p->mm->total_vm),
>  		K(get_mm_counter(p->mm, MM_ANONPAGES)),
>  		K(get_mm_counter(p->mm, MM_FILEPAGES)));
>  	task_unlock(p);
>  
> +	/*
> +	 * Kill all processes sharing p->mm in other thread groups, if any.
> +	 * They don't get access to memory reserves or a higher scheduler
> +	 * priority, though, to avoid depletion of all memory or task
> +	 * starvation.  This prevents mm->mmap_sem livelock when an oom killed
> +	 * task cannot exit because it requires the semaphore and its contended
> +	 * by another thread trying to allocate memory itself.  That thread will
> +	 * now get access to memory reserves since it has a pending fatal
> +	 * signal.
> +	 */
> +	for_each_process(q)
> +		if (q->mm == mm && !same_thread_group(q, p))
> +			force_sig(SIGKILL, q);

This makes silent process kill when vfork() is used. right?
If so, it is wrong idea. instead, can you please write "which process was killed" log
on each process?


--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[David Rientjes]

On Thu, 19 Aug 2010, KOSAKI Motohiro wrote:

> > This is especially necessary to solve an mm->mmap_sem livelock issue
> > whereas an oom killed thread must acquire the lock in the exit path while
> > another thread is holding it in the page allocator while trying to
> > allocate memory itself (and will preempt the oom killer since a task was
> > already killed).  Since tasks with pending fatal signals are now granted
> > access to memory reserves, the thread holding the lock may quickly
> > allocate and release the lock so that the oom killed task may exit.
> 
> I can't understand this sentence. mm sharing is happen when vfork, That
> said, parent process is always sleeping. why do we need to worry that parent
> process is holding mmap_sem?
> 

No, I'm talking about threads with CLONE_VM and not CLONE_THREAD (or 
CLONE_VFORK, in your example).  They share the same address space but are 
in different tgid's and may sit holding mm->mmap_sem looping in the page 
allocator while we know we're oom and there's no chance of freeing any 
more memory since the oom killer doesn't kill will other tasks have yet to 
exit.

> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -414,17 +414,37 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
> >  #define K(x) ((x) << (PAGE_SHIFT-10))
> >  static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
> >  {
> > +	struct task_struct *q;
> > +	struct mm_struct *mm;
> > +
> >  	p = find_lock_task_mm(p);
> >  	if (!p) {
> >  		task_unlock(p);
> >  		return 1;
> >  	}
> > +
> > +	/* mm cannot be safely dereferenced after task_unlock(p) */
> > +	mm = p->mm;
> > +
> >  	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
> >  		task_pid_nr(p), p->comm, K(p->mm->total_vm),
> >  		K(get_mm_counter(p->mm, MM_ANONPAGES)),
> >  		K(get_mm_counter(p->mm, MM_FILEPAGES)));
> >  	task_unlock(p);
> >  
> > +	/*
> > +	 * Kill all processes sharing p->mm in other thread groups, if any.
> > +	 * They don't get access to memory reserves or a higher scheduler
> > +	 * priority, though, to avoid depletion of all memory or task
> > +	 * starvation.  This prevents mm->mmap_sem livelock when an oom killed
> > +	 * task cannot exit because it requires the semaphore and its contended
> > +	 * by another thread trying to allocate memory itself.  That thread will
> > +	 * now get access to memory reserves since it has a pending fatal
> > +	 * signal.
> > +	 */
> > +	for_each_process(q)
> > +		if (q->mm == mm && !same_thread_group(q, p))
> > +			force_sig(SIGKILL, q);
> 
> This makes silent process kill when vfork() is used. right?
> If so, it is wrong idea. instead, can you please write "which process was killed" log
> on each process?
> 

Sure, I'll add a pr_err() for these kills as well.

--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[KOSAKI Motohiro]

> On Thu, 19 Aug 2010, KOSAKI Motohiro wrote:
> 
> > > This is especially necessary to solve an mm->mmap_sem livelock issue
> > > whereas an oom killed thread must acquire the lock in the exit path while
> > > another thread is holding it in the page allocator while trying to
> > > allocate memory itself (and will preempt the oom killer since a task was
> > > already killed).  Since tasks with pending fatal signals are now granted
> > > access to memory reserves, the thread holding the lock may quickly
> > > allocate and release the lock so that the oom killed task may exit.
> > 
> > I can't understand this sentence. mm sharing is happen when vfork, That
> > said, parent process is always sleeping. why do we need to worry that parent
> > process is holding mmap_sem?
> > 
> 
> No, I'm talking about threads with CLONE_VM and not CLONE_THREAD (or 
> CLONE_VFORK, in your example).  They share the same address space but are 
> in different tgid's and may sit holding mm->mmap_sem looping in the page 
> allocator while we know we're oom and there's no chance of freeing any 
> more memory since the oom killer doesn't kill will other tasks have yet to 
> exit.

Why don't you use pthread library? Is there any good reason? That said,
If you are trying to optimize neither thread nor vfork case, I'm not charmed
this because 99.99% user don't use it. but even though every user will get 
performance degression. Can you please consider typical use case optimization?



> 
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -414,17 +414,37 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
> > >  #define K(x) ((x) << (PAGE_SHIFT-10))
> > >  static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
> > >  {
> > > +	struct task_struct *q;
> > > +	struct mm_struct *mm;
> > > +
> > >  	p = find_lock_task_mm(p);
> > >  	if (!p) {
> > >  		task_unlock(p);
> > >  		return 1;
> > >  	}
> > > +
> > > +	/* mm cannot be safely dereferenced after task_unlock(p) */
> > > +	mm = p->mm;
> > > +
> > >  	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
> > >  		task_pid_nr(p), p->comm, K(p->mm->total_vm),
> > >  		K(get_mm_counter(p->mm, MM_ANONPAGES)),
> > >  		K(get_mm_counter(p->mm, MM_FILEPAGES)));
> > >  	task_unlock(p);
> > >  
> > > +	/*
> > > +	 * Kill all processes sharing p->mm in other thread groups, if any.
> > > +	 * They don't get access to memory reserves or a higher scheduler
> > > +	 * priority, though, to avoid depletion of all memory or task
> > > +	 * starvation.  This prevents mm->mmap_sem livelock when an oom killed
> > > +	 * task cannot exit because it requires the semaphore and its contended
> > > +	 * by another thread trying to allocate memory itself.  That thread will
> > > +	 * now get access to memory reserves since it has a pending fatal
> > > +	 * signal.
> > > +	 */
> > > +	for_each_process(q)
> > > +		if (q->mm == mm && !same_thread_group(q, p))
> > > +			force_sig(SIGKILL, q);
> > 
> > This makes silent process kill when vfork() is used. right?
> > If so, it is wrong idea. instead, can you please write "which process was killed" log
> > on each process?
> > 
> 
> Sure, I'll add a pr_err() for these kills as well.

ok, 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>

Re: [patch v2 2/2] oom: kill all threads sharing oom killed task's mm

[KOSAKI Motohiro]

> > On Thu, 19 Aug 2010, KOSAKI Motohiro wrote:
> > 
> > > > This is especially necessary to solve an mm->mmap_sem livelock issue
> > > > whereas an oom killed thread must acquire the lock in the exit path while
> > > > another thread is holding it in the page allocator while trying to
> > > > allocate memory itself (and will preempt the oom killer since a task was
> > > > already killed).  Since tasks with pending fatal signals are now granted
> > > > access to memory reserves, the thread holding the lock may quickly
> > > > allocate and release the lock so that the oom killed task may exit.
> > > 
> > > I can't understand this sentence. mm sharing is happen when vfork, That
> > > said, parent process is always sleeping. why do we need to worry that parent
> > > process is holding mmap_sem?
> > > 
> > 
> > No, I'm talking about threads with CLONE_VM and not CLONE_THREAD (or 
> > CLONE_VFORK, in your example).  They share the same address space but are 
> > in different tgid's and may sit holding mm->mmap_sem looping in the page 
> > allocator while we know we're oom and there's no chance of freeing any 
> > more memory since the oom killer doesn't kill will other tasks have yet to 
> > exit.
> 
> Why don't you use pthread library? Is there any good reason? That said,
> If you are trying to optimize neither thread nor vfork case, I'm not charmed
> this because 99.99% user don't use it. but even though every user will get 
> performance degression. Can you please consider typical use case optimization?

That said, This was NAKed while this patch makes end user unhappy. please
fix it.




--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[David Rientjes]

On Thu, 19 Aug 2010, KOSAKI Motohiro wrote:

> > No, I'm talking about threads with CLONE_VM and not CLONE_THREAD (or 
> > CLONE_VFORK, in your example).  They share the same address space but are 
> > in different tgid's and may sit holding mm->mmap_sem looping in the page 
> > allocator while we know we're oom and there's no chance of freeing any 
> > more memory since the oom killer doesn't kill will other tasks have yet to 
> > exit.
> 
> Why don't you use pthread library? Is there any good reason? That said,
> If you are trying to optimize neither thread nor vfork case, I'm not charmed
> this because 99.99% user don't use it. but even though every user will get 
> performance degression. Can you please consider typical use case optimization?
> 

Non-NPTL threaded applications exist in the wild, and I can't change that.  
This mm->mmap_sem livelock is a problem for them, we've hit it internally 
(we're forced to carry this patch internally), and we aren't the only ones 
running at least some non-NPTL apps.  That's why this code existed in the 
oom killer for over eight years since the 2.4 kernel before you removed it 
based on the mempolicy policy of killing current, which has since been 
obsoleted.  Until CLONE_VM without CLONE_THREAD is prohibited entirely on 
Linux, this livelock can exist.

Users who do not want the tasklist scan here, which only iterates over 
thread group leaders and not threads, can enable 
/proc/sys/vm/oom_kill_allocating_task.  That's its whole purpose.  Other 
than that, the oom killer will never be the most efficient part of the 
kernel and doing for_each_process() is much less expensive than all the 
task_lock()s we take already.

--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[KOSAKI Motohiro]

> On Thu, 19 Aug 2010, KOSAKI Motohiro wrote:
> 
> > > No, I'm talking about threads with CLONE_VM and not CLONE_THREAD (or 
> > > CLONE_VFORK, in your example).  They share the same address space but are 
> > > in different tgid's and may sit holding mm->mmap_sem looping in the page 
> > > allocator while we know we're oom and there's no chance of freeing any 
> > > more memory since the oom killer doesn't kill will other tasks have yet to 
> > > exit.
> > 
> > Why don't you use pthread library? Is there any good reason? That said,
> > If you are trying to optimize neither thread nor vfork case, I'm not charmed
> > this because 99.99% user don't use it. but even though every user will get 
> > performance degression. Can you please consider typical use case optimization?
> > 
> 
> Non-NPTL threaded applications exist in the wild, and I can't change that.  

Which application? If it is major opensource application, I think this one
makes end user happy.


> This mm->mmap_sem livelock is a problem for them, we've hit it internally 
> (we're forced to carry this patch internally), and we aren't the only ones 
> running at least some non-NPTL apps.  That's why this code existed in the 
> oom killer for over eight years since the 2.4 kernel before you removed it 
> based on the mempolicy policy of killing current, which has since been 
> obsoleted.  Until CLONE_VM without CLONE_THREAD is prohibited entirely on 
> Linux, this livelock can exist.

Or, can you eliminate O(n^2) thing? The cost is enough low, I don't oppose 
this.



> Users who do not want the tasklist scan here, which only iterates over 
> thread group leaders and not threads, can enable 
> /proc/sys/vm/oom_kill_allocating_task.  That's its whole purpose.  Other 
> than that, the oom killer will never be the most efficient part of the 
> kernel and doing for_each_process() is much less expensive than all the 
> task_lock()s we take already.

No.
Please don't forget typical end user don't use any kernel knob. kernel knob
is not a way for developer excuse.


--
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 v2 2/2] oom: kill all threads sharing oom killed task's mm

[David Rientjes]

On Fri, 20 Aug 2010, KOSAKI Motohiro wrote:

> > > Why don't you use pthread library? Is there any good reason? That said,
> > > If you are trying to optimize neither thread nor vfork case, I'm not charmed
> > > this because 99.99% user don't use it. but even though every user will get 
> > > performance degression. Can you please consider typical use case optimization?
> > > 
> > 
> > Non-NPTL threaded applications exist in the wild, and I can't change that.  
> 
> Which application? If it is major opensource application, I think this one
> makes end user happy.
> 

Being a "major opensource application" is not the requirement to prevent a 
livelock in the kernel.  The livelock exists, we hit it all the time, and 
your change to remove this code from the oom killer caused a regression.

Unless you can guarantee that a thread holding mm->mmap_sem cannot loop in 
the page allocator, then we need a way for that allocation to succeed.

> > This mm->mmap_sem livelock is a problem for them, we've hit it internally 
> > (we're forced to carry this patch internally), and we aren't the only ones 
> > running at least some non-NPTL apps.  That's why this code existed in the 
> > oom killer for over eight years since the 2.4 kernel before you removed it 
> > based on the mempolicy policy of killing current, which has since been 
> > obsoleted.  Until CLONE_VM without CLONE_THREAD is prohibited entirely on 
> > Linux, this livelock can exist.
> 
> Or, can you eliminate O(n^2) thing? The cost is enough low, I don't oppose 
> this.
> 

Suggestions on improvements are welcome in the form of a patch.

> > Users who do not want the tasklist scan here, which only iterates over 
> > thread group leaders and not threads, can enable 
> > /proc/sys/vm/oom_kill_allocating_task.  That's its whole purpose.  Other 
> > than that, the oom killer will never be the most efficient part of the 
> > kernel and doing for_each_process() is much less expensive than all the 
> > task_lock()s we take already.
> 
> No.
> Please don't forget typical end user don't use any kernel knob. kernel knob
> is not a way for developer excuse.
> 

Users who find tasklist scans with for_each_process() in the oom killer 
are a very, very special case and is typically only SGI.  They requested 
that the sysctl be added years ago to prevent the lengthy scan, so they 
are already protected.

Unless you can propose a different fix for the regression that you 
introduced with 8c5cd6f3 and fix the livelock, this type of check is 
mandatory.

--
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>