Re: [BUGFIX][PATCH] memcg: fix oom killer kills a task in other cgroup v2

[David Rientjes <rientjes@google.com>]

On Tue, 9 Feb 2010, KAMEZAWA Hiroyuki wrote:

> > > > > -	task_lock(task);
> > > > > +	/*
> > > > > + 	 * The task's task->mm pointer is guarded by task_lock() but it's
> > > > > + 	 * risky to take task_lock in oom kill situaion. Oom-killer may
> > > > > + 	 * kill a task which is in unknown status and cause siginificant delay
> > > > > + 	 * or deadlock.
> > > > > + 	 * So, we use some loose way. Because we're under taslist lock, "task"
> > > > > + 	 * pointer is always safe and we can access it. So, accessing mem_cgroup
> > > > > + 	 * via task struct is safe. To check the task is mm owner, we do loose
> > > > > + 	 * check. And this is enough.
> > > > > + 	 * There is small race at updating mm->onwer but we can ignore it.
> > > > > + 	 * A problematic race here means that oom-selection logic by walking
> > > > > + 	 * task list itself is racy. We can't make any strict guarantee between
> > > > > + 	 * task's cgroup status and oom-killer selection, anyway. And, in real
> > > > > + 	 * world, this will be no problem.
> > > > > + 	 */
> > > > > +	mm = task->mm;
> > > > > +	if (!mm || mm->owner != task)
> > > > > +		return 0;
> > > > 
> > > > You can't dereference task->mm->owner without holding task_lock(task), but 
> > > > I don't see why you need to even deal with task->mm.  All callers to this 
> > > > function will check for !task->mm either during their iterations or with 
> > > > oom_kill_task() returning 0.
> > > > 
> > > Just for being careful. We don't hold task_lock(), which guards task->mm in
> > > callers.
> > > 
> > 
> > The callers don't care if it disappears out from under us since we never 
> > dereference it, it's just a sanity check to ensure we don't pick a 
> > kthread or an exiting task that won't free any memory. 
> 
> But we need the guarantee that it's safe to access mm->owner in this code.
> It's possible task->mm is set to be NULL while we come here.
> Hmm. taking task_lock() is better, finally ?
> 

That was my original point when I said you can't dereference 
task->mm->owner without task_lock(task), but I don't see why you need that 
check to begin with.

> But I don't like taking such a lock here to do easy checks..
> *maybe* I'll postpone this updates and just post original fix again.
> 

Feel free to add my

	Acked-by: David Rientjes <rientjes@google.com>

since it's a much-needed fix for memcg both in mainline and in -stable.

> There are task_lock() and task_unlock() but task_trylock() is not implemented.
> I think I shouldn't add a new trylock.

task_trylock() isn't appropriate for this usecase because it would exclude 
tasks from the iteration in select_bad_process() if its contended, i.e. we 
could panic the machine unnecessary simply because the lock is taken.

--
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>