Re: + mm-oom-killer-kills-more-than-needed.patch added to -mm tree

[Chad Zanonie]

On Tue, Sep 23, 2008 at 5:40 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 09/22, Andrew Morton wrote:
>>
>> ------------------------------------------------------
>> Subject: mm: oom-killer kills more than needed
>> From: "Chad Zanonie" <chad.zanonie@xxxxxxxxx>
>>
>> Possibility exists for an exiting application to be in between marking its
>> mm NULL and calling mmput when out_of_memory is invoked.
>> select_bad_process() will continue past this process as opposed to
>> returning -1UL due to its mm being NULL.  This causes the oom killer in
>> certain scenarios to not only kill the memory culprit, but also kill the
>> runner up.
>>
>> EXIT_DEAD seems to be the only flag that guarantees that mmput() has
>> finished.
>
> I don't think this is right.
>
> Let's suppose we have a single zombie. Now select_bad_process() always
> returns -1 ? IOW, doesn't this means that, say,
>
>        $ perl -e 'fork && sleep'
>
> disables oom-kill completely and forever?

Ugh, looks like my description was slightly incorrect. I don't mean
for it to return -1 upon noticing a null mm. I mean for
select_bad_process to not skip (continue) over processes that haven't
provably finished unmapping their memory (which EXIT_DEAD infers).

>
> Hmm. But please see below. This doesn't happen because the usage
> of EXIT_DEAD is not right.
>
>> Checking for PF_KTHREAD should replace p->mm regardless.
>
> Yes, almost every check for ->mm in oom_kill.c is not right.
>
>> Adding EXIT_DEAD to the check seems to prevent unnecessary kills in local
>> testing.
>
> This is strange, could you re-test? Because
>
>> @@ -216,7 +216,7 @@ static struct task_struct *select_bad_pr
>>                * skip kernel threads and tasks which have already released
>>                * their mm.
>>                */
>> -             if (!p->mm)
>> +             if (p->flags & PF_KTHREAD || p->flags & EXIT_DEAD)
>                                                ^^^^^^^^^^^^^^^^^
>
> this is not possible. EXIT_DEAD lives in ->exit_state, not in ->flags.

Good catch. I had actually done the testing in an older kernel that
used PF_DEAD and while noticing the change to only EXIT_DEAD forgot to
use the new appropriate flag in the patch.

>
> Oleg.
>
>

I'm new to this endeavor. Should I propose a new patch, or, will
things be fixed from this omission? I still support the EXIT_DEAD
inclusion, as it'll allow the OOM killer to make the similar progress
that !p->mm provided.

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