Messages in this thread | | | Date | Tue, 23 Sep 2008 10:37:52 -0700 | From | "Chad Zanonie" <> | Subject | Re: + mm-oom-killer-kills-more-than-needed.patch added to -mm tree |
| |
Before I propagate this blunder anymore, I've found the root of my mistake.
I really mean TASK_DEAD, not EXIT_DEAD.
(p->state & TASK_DEAD)
-Chad
On Tue, Sep 23, 2008 at 10:25 AM, Chad Zanonie <chad.zanonie@gmail.com> wrote: > On Tue, Sep 23, 2008 at 5:40 AM, Oleg Nesterov <oleg@tv-sign.ru> wrote: >> On 09/22, Andrew Morton wrote: >>> >>> ------------------------------------------------------ >>> Subject: mm: oom-killer kills more than needed >>> From: "Chad Zanonie" <chad.zanonie@gmail.com> >>> >>> 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 >
| |