Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Michal Hocko]

On Thu 04-08-16 14:46:49, 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.
> 
> : 		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