Andrew Morton wrote:
> On Thu, 4 Aug 2016 21:28:13 +0900 Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > >
> > > Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> > > Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > ---
> > > 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.
Correct.
>
> : ret = __task_will_free_mem(p);
>
> And here we check to see if killing `p' would free up memory.
Not correct. Basic idea of __task_will_free_mem() is "check whether
the given task is already killed or exiting" in order to avoid sending
SIGKILL to tasks more than needed, and task_will_free_mem() is "check
whether all of the given mm users are already killed or exiting" in
order to avoid sending SIGKILL to tasks more than needed.
__task_will_free_mem(p) == true means p is already killed or exiting
and therefore the OOM killer does not need to send SIGKILL to `p'.
>
> : 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.
If `p' is not already killed or exiting, the OOM reaper cannot reap
p->mm because p will crash if p->mm suddenly disappears. Therefore,
the OOM killer needs to send SIGKILL to somebody.
>
> : }
>
> 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.
`A' is already killed or exiting, for it passed
if (!__task_will_free_mem(task))
return false;
test before the for_each_process(p) loop.
Although
if (atomic_read(&mm->mm_users) <= 1)
return true;
test was false as of atomic_read(), it is possible that `p'
releases its mm before reaching
if (!process_shares_mm(p, mm))
continue;
test. Therefore, it is possible that __task_will_free_mem(p) is
never called inside the for_each_process(p) loop. In that case,
task_will_free_mem(task) should return true, for it passed
if (!__task_will_free_mem(task))
return false;
test before the for_each_process(p) loop.
It is possible that `p' and `A' are the same thread group because
`A' (which can be "current") is not always a thread group leader.
If there is no external process sharing A's mm,
if (!process_shares_mm(p, mm))
continue;
test is true for all processes except the process for `A', and
if (same_thread_group(task, p))
continue;
test is true for the process for `A'. Therefore, it is possible that
__task_will_free_mem(p) is never called inside the for_each_process(p)
loop. In that case, task_will_free_mem(task) should return true.
>
> Correctish? Maybe. Can we please get some comments in there to
> demystify the decision-making?
>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>