[PATCH rh6] mm: skip zombie in OOM-killer

[Andrey Vagin]

A parent may wait a memory and zombie will prevent killing another task.

Signed-off-by: Andrey Vagin <ava...@openvz.org>
---
mm/oom_kill.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7dcca55..2fc554e 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -311,7 +311,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
* blocked waiting for another task which itself is waiting
* for memory. Is there a better alternative?
*/
- if (test_tsk_thread_flag(p, TIF_MEMDIE))
+ if (test_tsk_thread_flag(p, TIF_MEMDIE) && p->mm)
return ERR_PTR(-1UL);

/*
--
1.7.1

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

[David Rientjes]

On Sat, 5 Mar 2011, Andrey Vagin wrote:

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7dcca55..2fc554e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -311,7 +311,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
> * blocked waiting for another task which itself is waiting
> * for memory. Is there a better alternative?
> */
> - if (test_tsk_thread_flag(p, TIF_MEMDIE))
> + if (test_tsk_thread_flag(p, TIF_MEMDIE) && p->mm)
> return ERR_PTR(-1UL);
>
> /*

I think it would be better to just do

if (!p->mm)
continue;

after the check for oom_unkillable_task() because everything that follows
this really depends on p->mm being non-NULL to actually do anything
useful.

[Andrey Vagin]

When we check that task has flag TIF_MEMDIE, we forgot check that
it has mm. A task may be zombie and a parent may wait a memor.

v2: Check that task doesn't have mm one time and skip it immediately

Signed-off-by: Andrey Vagin <ava...@openvz.org>
---

mm/oom_kill.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7dcca55..b1ce3ba 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -299,6 +299,9 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
for_each_process(p) {
unsigned int points;

+ if (!p->mm)
+ continue;
+
if (oom_unkillable_task(p, mem, nodemask))
continue;

@@ -324,7 +327,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
* the process of exiting and releasing its resources.
* Otherwise we could get an easy OOM deadlock.
*/
- if (thread_group_empty(p) && (p->flags & PF_EXITING) && p->mm) {
+ if (thread_group_empty(p) && (p->flags & PF_EXITING)) {
if (p != current)
return ERR_PTR(-1UL);

--
1.7.1

[ava...@gmail.com]

On 03/05/2011 02:41 AM, David Rientjes wrote:
> On Sat, 5 Mar 2011, Andrey Vagin wrote:
>
>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> index 7dcca55..2fc554e 100644
>> --- a/mm/oom_kill.c
>> +++ b/mm/oom_kill.c
>> @@ -311,7 +311,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
>> * blocked waiting for another task which itself is waiting
>> * for memory. Is there a better alternative?
>> */
>> - if (test_tsk_thread_flag(p, TIF_MEMDIE))

>> + if (test_tsk_thread_flag(p, TIF_MEMDIE)&& p->mm)

>> return ERR_PTR(-1UL);
>>
>> /*
>
> I think it would be better to just do
>
> if (!p->mm)
> continue;
>
> after the check for oom_unkillable_task() because everything that follows
> this really depends on p->mm being non-NULL to actually do anything
> useful.

Yes. You are right. Thank you.

[David Rientjes]

On Sat, 5 Mar 2011, Andrey Vagin wrote:

> When we check that task has flag TIF_MEMDIE, we forgot check that
> it has mm. A task may be zombie and a parent may wait a memor.
>
> v2: Check that task doesn't have mm one time and skip it immediately
>
> Signed-off-by: Andrey Vagin <ava...@openvz.org>

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

[KOSAKI Motohiro]

> When we check that task has flag TIF_MEMDIE, we forgot check that
> it has mm. A task may be zombie and a parent may wait a memor.
>
> v2: Check that task doesn't have mm one time and skip it immediately
>
> Signed-off-by: Andrey Vagin <ava...@openvz.org>

This seems incorrect. Do you have a reprodusable testcasae?
Your patch only care thread group leader state, but current code
care all thread in the process. Please look at oom_badness() and
find_lock_task_mm().

[KOSAKI Motohiro]

> On Sat, 5 Mar 2011, Andrey Vagin wrote:
>
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 7dcca55..2fc554e 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -311,7 +311,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
> > * blocked waiting for another task which itself is waiting
> > * for memory. Is there a better alternative?
> > */
> > - if (test_tsk_thread_flag(p, TIF_MEMDIE))
> > + if (test_tsk_thread_flag(p, TIF_MEMDIE) && p->mm)
> > return ERR_PTR(-1UL);
> >
> > /*
>
> I think it would be better to just do
>
> if (!p->mm)
> continue;
>
> after the check for oom_unkillable_task() because everything that follows
> this really depends on p->mm being non-NULL to actually do anything
> useful.

I'm glad you join to review MM patches. It is worth effort for making
solid kernel. But, please look at a current code at first.

[David Rientjes]

On Sun, 6 Mar 2011, KOSAKI Motohiro wrote:

> > When we check that task has flag TIF_MEMDIE, we forgot check that
> > it has mm. A task may be zombie and a parent may wait a memor.
> >
> > v2: Check that task doesn't have mm one time and skip it immediately
> >
> > Signed-off-by: Andrey Vagin <ava...@openvz.org>
>
> This seems incorrect. Do you have a reprodusable testcasae?
> Your patch only care thread group leader state, but current code
> care all thread in the process. Please look at oom_badness() and
> find_lock_task_mm().
>

That's all irrelevant, the test for TIF_MEMDIE specifically makes the oom
killer a complete no-op when an eligible task is found to have been oom
killed to prevent needlessly killing additional tasks. oom_badness() and
find_lock_task_mm() have nothing to do with that check to return
ERR_PTR(-1UL) from select_bad_process().

Andrey is patching the case where an eligible TIF_MEMDIE process is found
but it has already detached its ->mm. In combination with the patch
posted to linux-mm, oom: prevent unnecessary oom kills or kernel panics,
which makes select_bad_process() iterate over all threads, it is an
effective solution.

Thanks.

[Andrew Vagin]

2011/3/7 David Rientjes <rien...@google.com>:

> On Sun, 6 Mar 2011, KOSAKI Motohiro wrote:
>
>> > When we check that task has flag TIF_MEMDIE, we forgot check that
>> > it has mm. A task may be zombie and a parent may wait a memor.
>> >
>> > v2: Check that task doesn't have mm one time and skip it immediately
>> >
>> > Signed-off-by: Andrey Vagin <ava...@openvz.org>
>>
>> This seems incorrect. Do you have a reprodusable testcasae?
>> Your patch only care thread group leader state, but current code
>> care all thread in the process. Please look at oom_badness() and
>> find_lock_task_mm().
>>
>
> That's all irrelevant, the test for TIF_MEMDIE specifically makes the oom
> killer a complete no-op when an eligible task is found to have been oom
> killed to prevent needlessly killing additional tasks.  oom_badness() and
> find_lock_task_mm() have nothing to do with that check to return
> ERR_PTR(-1UL) from select_bad_process().
>
> Andrey is patching the case where an eligible TIF_MEMDIE process is found
> but it has already detached its ->mm.  In combination with the patch
> posted to linux-mm, oom: prevent unnecessary oom kills or kernel panics,
> which makes select_bad_process() iterate over all threads, it is an
> effective solution.

Probably you said about the first version of my patch.
This version is incorrect because of
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=dd8e8f405ca386c7ce7cbb996ccd985d283b0e03

but my first patch is correct and it has a simple reproducer(I
attached it). You can execute it and your kernel hangs up, because the
parent doesn't wait children, but the one child (zombie) will have
flag TIF_MEMDIE, oom_killer will kill nobody

The link on the first patch:
http://groups.google.com/group/linux.kernel/browse_thread/thread/b9c6ddf34d1671ab/2941e1877ca4f626?lnk=raot&pli=1

[David Rientjes]

On Mon, 7 Mar 2011, Andrew Vagin wrote:

> > Andrey is patching the case where an eligible TIF_MEMDIE process is found
> > but it has already detached its ->mm.  In combination with the patch
> > posted to linux-mm, oom: prevent unnecessary oom kills or kernel panics,
> > which makes select_bad_process() iterate over all threads, it is an
> > effective solution.
>
> Probably you said about the first version of my patch.
> This version is incorrect because of
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=dd8e8f405ca386c7ce7cbb996ccd985d283b0e03
>
> but my first patch is correct and it has a simple reproducer(I
> attached it). You can execute it and your kernel hangs up, because the
> parent doesn't wait children, but the one child (zombie) will have
> flag TIF_MEMDIE, oom_killer will kill nobody
>

The second version of your patch works fine in combination with the
pending "oom: prevent unnecessary oom kills or kernel panics" patch from
linux-mm (included below). Try your test case with both this patch and
the second version of your patch.

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -292,11 +292,11 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
unsigned long totalpages, struct mem_cgroup *mem,
const nodemask_t *nodemask)
{
- struct task_struct *p;
+ struct task_struct *g, *p;
struct task_struct *chosen = NULL;
*ppoints = 0;

- for_each_process(p) {
+ do_each_thread(g, p) {
unsigned int points;

if (oom_unkillable_task(p, mem, nodemask))
@@ -324,7 +324,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,

* the process of exiting and releasing its resources.
* Otherwise we could get an easy OOM deadlock.
*/
- if (thread_group_empty(p) && (p->flags & PF_EXITING) && p->mm) {

+ if ((p->flags & PF_EXITING) && p->mm) {

if (p != current)
return ERR_PTR(-1UL);

@@ -337,7 +337,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
chosen = p;
*ppoints = points;
}
- }
+ } while_each_thread(g, p);

return chosen;
}

[Andrew Morton]

On Mon, 7 Mar 2011 12:36:49 -0800 (PST)
David Rientjes <rien...@google.com> wrote:

> On Mon, 7 Mar 2011, Andrew Vagin wrote:
>
> > > Andrey is patching the case where an eligible TIF_MEMDIE process is found

> > > but it has already detached its ->mm. __In combination with the patch

> > > posted to linux-mm, oom: prevent unnecessary oom kills or kernel panics,
> > > which makes select_bad_process() iterate over all threads, it is an
> > > effective solution.
> >
> > Probably you said about the first version of my patch.
> > This version is incorrect because of
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=dd8e8f405ca386c7ce7cbb996ccd985d283b0e03
> >
> > but my first patch is correct and it has a simple reproducer(I
> > attached it). You can execute it and your kernel hangs up, because the
> > parent doesn't wait children, but the one child (zombie) will have
> > flag TIF_MEMDIE, oom_killer will kill nobody
> >
>
> The second version of your patch works fine in combination with the
> pending "oom: prevent unnecessary oom kills or kernel panics" patch from
> linux-mm (included below).

Andrew's v2 doesn't apply on top of
oom-prevent-unnecessary-oom-kills-or-kernel-panics.patch and I'm
disinclined to fix that up and merge some untested patch combination.

[David Rientjes]

On Mon, 7 Mar 2011, Andrew Morton wrote:

> Andrew's v2 doesn't apply on top of
> oom-prevent-unnecessary-oom-kills-or-kernel-panics.patch and I'm
> disinclined to fix that up and merge some untested patch combination.
>

Ok. Andrey, I rebased your patch on top of the latest -mm tree
(mmotm-2011-03-02-16-52 with
oom-prevent-unnecessary-oom-kills-or-kernel-panics.patch from
http://marc.info/?l=linux-mm-commits&m=129953480527038&q=raw) and rewrote
the changelog. They'll both apply on top of Linus' -git even without
mmotm. Could you try this out on your testcase?

Thanks!

oom: skip zombies when iterating tasklist

From: Andrey Vagin <ava...@openvz.org>

We shouldn't defer oom killing if a thread has already detached its ->mm
and still has TIF_MEMDIE set. Memory needs to be freed, so find kill
other threads that pin the same ->mm or find another task to kill.

Signed-off-by: Andrey Vagin <ava...@openvz.org>
Signed-off-by: David Rientjes <rien...@google.com>
---
mm/oom_kill.c | 4 +++-
1 files changed, 3 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

@@ -299,6 +299,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,

do_each_thread(g, p) {
unsigned int points;

+ if (!p->mm)
+ continue;

if (oom_unkillable_task(p, mem, nodemask))
continue;

@@ -324,7 +326,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,

* the process of exiting and releasing its resources.
* Otherwise we could get an easy OOM deadlock.
*/

- if ((p->flags & PF_EXITING) && p->mm) {
+ if (p->flags & PF_EXITING) {

if (p != current)
return ERR_PTR(-1UL);

[KOSAKI Motohiro]

> A parent may wait a memory and zombie will prevent killing another task.
>
> Signed-off-by: Andrey Vagin <ava...@openvz.org>
> ---
> mm/oom_kill.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7dcca55..2fc554e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -311,7 +311,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
> * blocked waiting for another task which itself is waiting
> * for memory. Is there a better alternative?
> */
> - if (test_tsk_thread_flag(p, TIF_MEMDIE))
> + if (test_tsk_thread_flag(p, TIF_MEMDIE) && p->mm)
> return ERR_PTR(-1UL);

OK. Good catch.

Reviewed-by: KOSAKI Motohiro <kosaki....@jp.fujitsu.com>

[KOSAKI Motohiro]

OK. I can ack this.
TIF_MEMDIE mean the process have been receive SIGKILL therefore we can assume it
as per process flag.

[KOSAKI Motohiro]

> On Sun, 6 Mar 2011, KOSAKI Motohiro wrote:
>
> > > When we check that task has flag TIF_MEMDIE, we forgot check that
> > > it has mm. A task may be zombie and a parent may wait a memor.
> > >
> > > v2: Check that task doesn't have mm one time and skip it immediately
> > >
> > > Signed-off-by: Andrey Vagin <ava...@openvz.org>
> >
> > This seems incorrect. Do you have a reprodusable testcasae?
> > Your patch only care thread group leader state, but current code
> > care all thread in the process. Please look at oom_badness() and
> > find_lock_task_mm().
> >
>
> That's all irrelevant, the test for TIF_MEMDIE specifically makes the oom
> killer a complete no-op when an eligible task is found to have been oom
> killed to prevent needlessly killing additional tasks. oom_badness() and
> find_lock_task_mm() have nothing to do with that check to return
> ERR_PTR(-1UL) from select_bad_process().

I don't understand you think which task is eligible and unnecessary.
But, Look! Andrey is not talking about zombie process case. But, this v2
patch have factored out other tasks too. This IS the problem. No need
unrelated talk.

>
> Andrey is patching the case where an eligible TIF_MEMDIE process is found
> but it has already detached its ->mm. In combination with the patch
> posted to linux-mm, oom: prevent unnecessary oom kills or kernel panics,
> which makes select_bad_process() iterate over all threads, it is an
> effective solution.

Guys, It was alread NAKed. I've already talk kind explanation. Why do
you bother to look actual code. Why do you continue to talk funny your
dream?

[KOSAKI Motohiro]

> I don't understand you think which task is eligible and unnecessary.
> But, Look! Andrey is not talking about zombie process case. But, this v2
> patch have factored out other tasks too. This IS the problem. No need

^^^^^^^^
filter.

I need to rest.