Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1235152 > unrolled thread

[PATCH -mm 0/3] mm/oom_kill: ensure we actually kill all tasks sharing the same mm

Back to article view | Back to linux.kernel

Contents

  [PATCH -mm 0/3] mm/oom_kill: ensure we actually kill all tasks  sharing the same mm Oleg Nesterov <oleg@redhat.com> - 2015-09-29 16:30 +0200
    [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in Oleg Nesterov <oleg@redhat.com> - 2015-09-29 16:30 +0200
      Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks  in David Rientjes <rientjes@google.com> - 2015-09-30 00:50 +0200
        Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm  checks in Oleg Nesterov <oleg@redhat.com> - 2015-09-30 16:00 +0200
      Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-09-30 04:20 +0200
        Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm  checks in Oleg Nesterov <oleg@redhat.com> - 2015-09-30 16:10 +0200
    [PATCH -mm 2/3] mm/oom_kill: cleanup the "kill sharing same memory" Oleg Nesterov <oleg@redhat.com> - 2015-09-29 16:30 +0200
      Re: [PATCH -mm 2/3] mm/oom_kill: cleanup the "kill sharing same  memory" David Rientjes <rientjes@google.com> - 2015-09-30 00:40 +0200
        Re: [PATCH -mm 2/3] mm/oom_kill: cleanup the "kill sharing same  memory" Oleg Nesterov <oleg@redhat.com> - 2015-09-30 16:00 +0200
    [PATCH -mm v2 0/3] mm/oom_kill: ensure we actually kill all tasks  sharing the same mm Oleg Nesterov <oleg@redhat.com> - 2015-09-30 20:30 +0200

#1235152 — [PATCH -mm 0/3] mm/oom_kill: ensure we actually kill all tasks sharing the same mm

Michal, Tetsuo, David, sorry for delay. I'll try to read and answer
your emails in "can't oom-kill zap the victim's memory" thread later.

Let me send some initial changes which imo makes sense regardless,
but if we want to zap the victim's memory we need to ensure that all
tasks which share this ->mm were actually killed (see 3/3).

Please review, this series is simple but only compile tested.

Andrew, this is on top of linux-mmotm.git + the recent mm-oom-remove-
task_lock-protecting-comm-printing.patch from David.

Oleg.

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

[toc] | [next] | [standalone]

#1235153 — [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in

Both "child->mm == mm" and "p->mm != mm" checks in oom_kill_process()
are wrong. ->mm can be if task is the exited group leader. This means
in particular that "kill sharing same memory" loop can miss a process
with a zombie leader which uses the same ->mm.

Note: the process_has_mm(child, p->mm) check is still not 100% correct,
p->mm can be NULL too. This is minor, but probably deserves a fix or a
comment anyway.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/oom_kill.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 8e7bed2..8ecac2ef 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -483,6 +483,17 @@ void oom_killer_enable(void)
 	oom_killer_disabled = false;
 }
 
+static bool process_has_mm(struct task_struct *p, struct mm_struct *mm)
+{
+	struct task_struct *t;
+
+	for_each_thread(p, t)
+		if (t->mm)
+			return t->mm == mm;
+
+	return false;
+}
+
 #define K(x) ((x) << (PAGE_SHIFT-10))
 /*
  * Must be called while holding a reference to p, which will be released upon
@@ -530,7 +541,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 		list_for_each_entry(child, &t->children, sibling) {
 			unsigned int child_points;
 
-			if (child->mm == p->mm)
+			if (process_has_mm(child, p->mm))
 				continue;
 			/*
 			 * oom_badness() returns 0 if the thread is unkillable
@@ -588,7 +599,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 			continue;
 		if (same_thread_group(p, victim))
 			continue;
-		if (p->mm != mm)
+		if (!process_has_mm(p, mm))
 			continue;
 		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 			continue;
-- 
2.4.3

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

[toc] | [prev] | [next] | [standalone]

#1235609 — Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in

On Tue, 29 Sep 2015, Oleg Nesterov wrote:

> Both "child->mm == mm" and "p->mm != mm" checks in oom_kill_process()
> are wrong. ->mm can be if task is the exited group leader. This means
> in particular that "kill sharing same memory" loop can miss a process
> with a zombie leader which uses the same ->mm.
> 
> Note: the process_has_mm(child, p->mm) check is still not 100% correct,
> p->mm can be NULL too. This is minor, but probably deserves a fix or a
> comment anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: David Rientjes <rientjes@google.com>

I like this and I don't want to hold up a fix for a personal preference, 
but I find process_has_mm() to simply imply the process has a non-NULL mm.  
Maybe process_shares_mm()?  Something to consider.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236321 — Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in

On 09/29, David Rientjes wrote:
>
> On Tue, 29 Sep 2015, Oleg Nesterov wrote:
>
> > Both "child->mm == mm" and "p->mm != mm" checks in oom_kill_process()
> > are wrong. ->mm can be if task is the exited group leader. This means
> > in particular that "kill sharing same memory" loop can miss a process
> > with a zombie leader which uses the same ->mm.
> >
> > Note: the process_has_mm(child, p->mm) check is still not 100% correct,
> > p->mm can be NULL too. This is minor, but probably deserves a fix or a
> > comment anyway.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Acked-by: David Rientjes <rientjes@google.com>
>
> I like this and I don't want to hold up a fix for a personal preference,
> but I find process_has_mm() to simply imply the process has a non-NULL mm.

Hmm, yes ;)

> Maybe process_shares_mm()?  Something to consider.

Agreed, will rename in v2.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1235712 — Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in

Oleg Nesterov wrote:
> Both "child->mm == mm" and "p->mm != mm" checks in oom_kill_process()
> are wrong. ->mm can be if task is the exited group leader. This means

can be [missing word here?] if task



> +static bool process_has_mm(struct task_struct *p, struct mm_struct *mm)
> +{
> +	struct task_struct *t;
> +
> +	for_each_thread(p, t)
> +		if (t->mm)

Can t->mm change between pevious line and next line?

> +			return t->mm == mm;
> +
> +	return false;
> +}
> +
>  #define K(x) ((x) << (PAGE_SHIFT-10))
>  /*
>   * Must be called while holding a reference to p, which will be released upon
> @@ -530,7 +541,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  		list_for_each_entry(child, &t->children, sibling) {
>  			unsigned int child_points;
>  
> -			if (child->mm == p->mm)
> +			if (process_has_mm(child, p->mm))
>  				continue;

We hold read_lock(&tasklist_lock) but not rcu_read_lock().
Is for_each_thread() safe without rcu_read_lock()?

>  			/*
>  			 * oom_badness() returns 0 if the thread is unkillable
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236330 — Re: [PATCH -mm 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in

On 09/30, Tetsuo Handa wrote:
>
> Oleg Nesterov wrote:
> > Both "child->mm == mm" and "p->mm != mm" checks in oom_kill_process()
> > are wrong. ->mm can be if task is the exited group leader. This means
>
> can be [missing word here?] if task

Yes thanks. Will fix in v2.

Hmm. And I just noticed that the subjects were corrupted... need to fix
my script.

> > +static bool process_has_mm(struct task_struct *p, struct mm_struct *mm)
> > +{
> > +	struct task_struct *t;
> > +
> > +	for_each_thread(p, t)
> > +		if (t->mm)
>
> Can t->mm change between pevious line and next line?

Good point, thanks. I'll add READ_ONCE() to ensure gcc won't re-load
t->mm again.

> > @@ -530,7 +541,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  		list_for_each_entry(child, &t->children, sibling) {
> >  			unsigned int child_points;
> >
> > -			if (child->mm == p->mm)
> > +			if (process_has_mm(child, p->mm))
> >  				continue;
>
> We hold read_lock(&tasklist_lock) but not rcu_read_lock().
> Is for_each_thread() safe without rcu_read_lock()?

Yes, for_each_thread() is rcu and/or tasklist_lock safe.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1235160 — [PATCH -mm 2/3] mm/oom_kill: cleanup the "kill sharing same memory"

Purely cosmetic, but the complex "if" condition looks annoying to me.
Especially because it is not consistent with OOM_SCORE_ADJ_MIN check
which adds another if/continue.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/oom_kill.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0d581c6..8e7bed2 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -583,16 +583,20 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	 * pending fatal signal.
 	 */
 	rcu_read_lock();
-	for_each_process(p)
-		if (p->mm == mm && !same_thread_group(p, victim) &&
-		    !(p->flags & PF_KTHREAD)) {
-			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
-				continue;
+	for_each_process(p) {
+		if (unlikely(p->flags & PF_KTHREAD))
+			continue;
+		if (same_thread_group(p, victim))
+			continue;
+		if (p->mm != mm)
+			continue;
+		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
+			continue;
 
-			pr_info("Kill process %d (%s) sharing same memory\n",
-				task_pid_nr(p), p->comm);
-			do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
-		}
+		pr_info("Kill process %d (%s) sharing same memory\n",
+			task_pid_nr(p), p->comm);
+		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
+	}
 	rcu_read_unlock();
 
 	mmput(mm);
-- 
2.4.3

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

[toc] | [prev] | [next] | [standalone]

#1235608 — Re: [PATCH -mm 2/3] mm/oom_kill: cleanup the "kill sharing same memory"

On Tue, 29 Sep 2015, Oleg Nesterov wrote:

> Purely cosmetic, but the complex "if" condition looks annoying to me.
> Especially because it is not consistent with OOM_SCORE_ADJ_MIN check
> which adds another if/continue.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  mm/oom_kill.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 0d581c6..8e7bed2 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -583,16 +583,20 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	 * pending fatal signal.
>  	 */
>  	rcu_read_lock();
> -	for_each_process(p)
> -		if (p->mm == mm && !same_thread_group(p, victim) &&
> -		    !(p->flags & PF_KTHREAD)) {
> -			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> -				continue;
> +	for_each_process(p) {
> +		if (unlikely(p->flags & PF_KTHREAD))
> +			continue;
> +		if (same_thread_group(p, victim))
> +			continue;
> +		if (p->mm != mm)
> +			continue;

This ordering is a little weird to me, I think we would eliminate the 
majority of processes by checking for p->mm != mm first.  There are 
certainly pathological cases where that can be defeated, but in practice 
it seems to happen more often than not.

Unless you object, I think the ordering should be p->mm != mm, 
same_thread_group(), unlikely(PF_KTHREAD) as it originally was (thanks for 
adding the unlikely).

I agree your cleanup looks much better than the nested conditional.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236315 — Re: [PATCH -mm 2/3] mm/oom_kill: cleanup the "kill sharing same memory"

On 09/29, David Rientjes wrote:
>
> On Tue, 29 Sep 2015, Oleg Nesterov wrote:
>
> > Purely cosmetic, but the complex "if" condition looks annoying to me.
> > Especially because it is not consistent with OOM_SCORE_ADJ_MIN check
> > which adds another if/continue.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  mm/oom_kill.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 0d581c6..8e7bed2 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -583,16 +583,20 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  	 * pending fatal signal.
> >  	 */
> >  	rcu_read_lock();
> > -	for_each_process(p)
> > -		if (p->mm == mm && !same_thread_group(p, victim) &&
> > -		    !(p->flags & PF_KTHREAD)) {
> > -			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > -				continue;
> > +	for_each_process(p) {
> > +		if (unlikely(p->flags & PF_KTHREAD))
> > +			continue;
> > +		if (same_thread_group(p, victim))
> > +			continue;
> > +		if (p->mm != mm)
> > +			continue;
>
> This ordering is a little weird to me, I think we would eliminate the
> majority of processes by checking for p->mm != mm first.  There are
> certainly pathological cases where that can be defeated, but in practice
> it seems to happen more often than not.
>
> Unless you object, I think the ordering should be p->mm != mm,
> same_thread_group(), unlikely(PF_KTHREAD) as it originally was (thanks for
> adding the unlikely).

OK, agreed, will send v2.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1236632 — [PATCH -mm v2 0/3] mm/oom_kill: ensure we actually kill all tasks sharing the same mm

On 09/29, Oleg Nesterov wrote:
>
> Let me send some initial changes which imo makes sense regardless,
> but if we want to zap the victim's memory we need to ensure that all
> tasks which share this ->mm were actually killed (see 3/3).
>
> Please review, this series is simple but only compile tested.
>
> Andrew, this is on top of linux-mmotm.git + the recent mm-oom-remove-
> task_lock-protecting-comm-printing.patch from David.

Please consider v2 based on the comments from Tetsuo and David (thanks
a lot!).

Oleg.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web