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

[Oleg Nesterov]

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, >children, sibling) {
> > unsigned int child_points;
> >
> > -   if (child->mm == p->mm)
> > +   if (process_has_mm(child, p->mm))
> > continue;
>
> We hold read_lock(_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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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 
>
> Acked-by: David Rientjes 
>
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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 
>
> Acked-by: David Rientjes 
>
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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, >children, sibling) {
> > unsigned int child_points;
> >
> > -   if (child->mm == p->mm)
> > +   if (process_has_mm(child, p->mm))
> > continue;
>
> We hold read_lock(_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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tetsuo Handa]

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, >children, sibling) {
>   unsigned int child_points;
>  
> - if (child->mm == p->mm)
> + if (process_has_mm(child, p->mm))
>   continue;

We hold read_lock(_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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[David Rientjes]

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 

Acked-by: David Rientjes 

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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 
---
 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, >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oleg Nesterov]

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 
---
 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, >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[David Rientjes]

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 

Acked-by: David Rientjes 

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tetsuo Handa]

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, >children, sibling) {
>   unsigned int child_points;
>  
> - if (child->mm == p->mm)
> + if (process_has_mm(child, p->mm))
>   continue;

We hold read_lock(_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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/