On Thu 04-08-16 14:46:49, Andrew Morton wrote:
> On Thu, 4 Aug 2016 21:28:13 +0900 Tetsuo Handa
> <penguin-ker...@i-love.sakura.ne.jp> wrote:
>
> > >
> > > Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> > > Signed-off-by: Geert Uytterhoeven <ge...@linux-m68k.org>
> > > ---
> > > Untested. I'm not familiar with the code, hence the default value of
> > > true was deducted from the logic in the loop (return false as soon as
> > > __task_will_free_mem() has returned false).
> >
> > I think ret = true is correct. Andrew, please send to linux.git.
>
> task_will_free_mem() is too hard to understand.
>
> We're examining task "A":
>
> : for_each_process(p) {
> : if (!process_shares_mm(p, mm))
> : continue;
> : if (same_thread_group(task, p))
> : continue;
>
> So here, we've found a process `p' which shares A's mm and which does
> not share A's thread group.
>
> : ret = __task_will_free_mem(p);
>
> And here we check to see if killing `p' would free up memory.
>
> : if (!ret)
> : break;
>
> If killing `p' will not free memory then give up the scan of all
> processes because <reasons>, and we decide that killing `A' will
> not free memory either, because some other task is holding onto
> A's memory anyway.
>
> : }
>
> And if no task is found to be sharing A's mm while not sharing A's
> thread group then fall through and decide to kill A. In which case the
> patch to return `true' is correct.
>
> Correctish?
Yes this is more or less correct. task_will_free_mem is a bit misnomer
but I couldn't come up with something better when reworking it and so
I kept the original name. task_will_free_mem basically says that the
task is dying and we hope it will free some memory so it doesn't make
much sense to send it SIGKILL.
> Maybe. Can we please get some comments in there to
> demystify the decision-making?
Does this help?
---
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 908c097c8b47..ce02db7f8661 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -803,8 +803,9 @@ static bool task_will_free_mem(struct task_struct *task)
return true;
/*
- * This is really pessimistic but we do not have any reliable way
- * to check that external processes share with our mm
+ * Make sure that all tasks which share the mm with the given tasks
+ * are dying as well to make sure that a) nobody pins its mm and
+ * b) the task is also reapable by the oom reaper.
*/
rcu_read_lock();
for_each_process(p) {
--
Michal Hocko
SUSE Labs
