From: Oleg Nesterov <oleg@redhat.com> To: Michal Hocko <mhocko@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org>, David Rientjes <rientjes@google.com>, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, Michal Hocko <mhocko@suse.com> Subject: Re: [PATCH] oom: consider multi-threaded tasks in task_will_free_mem Date: Tue, 17 May 2016 20:42:25 +0200 [thread overview] Message-ID: <20160517184225.GB32068@redhat.com> (raw) In-Reply-To: <1460452756-15491-1-git-send-email-mhocko@kernel.org> On 04/12, Michal Hocko wrote: > > We shouldn't consider the task > unless the whole thread group is going down. Yes, agreed. I'd even say that oom-killer should never look at individual task/threads, it should work with mm's. And one of the big mistakes (imo) was the s/for_each_process/for_each_thread/ change in select_bad_process() a while ago. Michal, I won't even try to actually review this patch, I lost any hope to understand OOM-killer a long ago ;) But I do agree with this change, we obviously should not rely on PF_EXITING. > static inline bool task_will_free_mem(struct task_struct *task) > { > + struct signal_struct *sig = task->signal; > + > /* > * A coredumping process may sleep for an extended period in exit_mm(), > * so the oom killer cannot assume that the process will promptly exit > * and release memory. > */ > - return (task->flags & PF_EXITING) && > - !(task->signal->flags & SIGNAL_GROUP_COREDUMP); > + if (sig->flags & SIGNAL_GROUP_COREDUMP) > + return false; > + > + if (!(task->flags & PF_EXITING)) > + return false; > + > + /* Make sure that the whole thread group is going down */ > + if (!thread_group_empty(task) && !(sig->flags & SIGNAL_GROUP_EXIT)) > + return false; > + > + return true; So this looks certainly better to me, but perhaps it should do if (SIGNAL_GROUP_COREDUMP) return false; if (SIGNAL_GROUP_EXIT) return true; if (thread_group_empty() && PF_EXITING) return true; return false; ? I won't insist, I do not even know if this would be better or not. But if SIGNAL_GROUP_EXIT is set all sub-threads should go away even if PF_EXITING is not set yet because this task didn't dequeue SIGKILL yet. Up to you in any case. Oleg.
WARNING: multiple messages have this Message-ID (diff)
From: Oleg Nesterov <oleg@redhat.com> To: Michal Hocko <mhocko@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org>, David Rientjes <rientjes@google.com>, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, Michal Hocko <mhocko@suse.com> Subject: Re: [PATCH] oom: consider multi-threaded tasks in task_will_free_mem Date: Tue, 17 May 2016 20:42:25 +0200 [thread overview] Message-ID: <20160517184225.GB32068@redhat.com> (raw) In-Reply-To: <1460452756-15491-1-git-send-email-mhocko@kernel.org> On 04/12, Michal Hocko wrote: > > We shouldn't consider the task > unless the whole thread group is going down. Yes, agreed. I'd even say that oom-killer should never look at individual task/threads, it should work with mm's. And one of the big mistakes (imo) was the s/for_each_process/for_each_thread/ change in select_bad_process() a while ago. Michal, I won't even try to actually review this patch, I lost any hope to understand OOM-killer a long ago ;) But I do agree with this change, we obviously should not rely on PF_EXITING. > static inline bool task_will_free_mem(struct task_struct *task) > { > + struct signal_struct *sig = task->signal; > + > /* > * A coredumping process may sleep for an extended period in exit_mm(), > * so the oom killer cannot assume that the process will promptly exit > * and release memory. > */ > - return (task->flags & PF_EXITING) && > - !(task->signal->flags & SIGNAL_GROUP_COREDUMP); > + if (sig->flags & SIGNAL_GROUP_COREDUMP) > + return false; > + > + if (!(task->flags & PF_EXITING)) > + return false; > + > + /* Make sure that the whole thread group is going down */ > + if (!thread_group_empty(task) && !(sig->flags & SIGNAL_GROUP_EXIT)) > + return false; > + > + return true; So this looks certainly better to me, but perhaps it should do if (SIGNAL_GROUP_COREDUMP) return false; if (SIGNAL_GROUP_EXIT) return true; if (thread_group_empty() && PF_EXITING) return true; return false; ? I won't insist, I do not even know if this would be better or not. But if SIGNAL_GROUP_EXIT is set all sub-threads should go away even if PF_EXITING is not set yet because this task didn't dequeue SIGKILL yet. Up to you in any case. Oleg. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-05-17 18:42 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-04-12 9:19 [PATCH] oom: consider multi-threaded tasks in task_will_free_mem Michal Hocko 2016-04-12 9:19 ` Michal Hocko 2016-04-13 11:04 ` Tetsuo Handa 2016-04-13 11:04 ` Tetsuo Handa 2016-04-13 13:08 ` Michal Hocko 2016-04-13 13:08 ` Michal Hocko 2016-04-13 13:27 ` Tetsuo Handa 2016-04-13 13:27 ` Tetsuo Handa 2016-04-13 13:45 ` Michal Hocko 2016-04-13 13:45 ` Michal Hocko 2016-05-17 18:06 ` Oleg Nesterov 2016-05-17 18:06 ` Oleg Nesterov 2016-04-26 13:57 ` Michal Hocko 2016-04-26 13:57 ` Michal Hocko 2016-05-17 20:28 ` Michal Hocko 2016-05-17 20:28 ` Michal Hocko 2016-05-17 22:21 ` Andrew Morton 2016-05-17 22:21 ` Andrew Morton 2016-05-18 7:16 ` Michal Hocko 2016-05-18 7:16 ` Michal Hocko 2016-05-17 18:42 ` Oleg Nesterov [this message] 2016-05-17 18:42 ` Oleg Nesterov 2016-05-17 20:25 ` Michal Hocko 2016-05-17 20:25 ` Michal Hocko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160517184225.GB32068@redhat.com \ --to=oleg@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=mhocko@suse.com \ --cc=rientjes@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.