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:

> > This is only called from the oom killer, so I'm not sure this needs to 
> > be renamed.  
> Why I renamed this is "be careful when a new user calls this".
> 

It would still be good to document the function as requiring a readlock on 
tasklist_lock.

> > > Index: mmotm-2.6.33-Feb06/mm/memcontrol.c
> > > ===================================================================
> > > --- mmotm-2.6.33-Feb06.orig/mm/memcontrol.c
> > > +++ mmotm-2.6.33-Feb06/mm/memcontrol.c
> > > @@ -781,16 +781,40 @@ void mem_cgroup_move_lists(struct page *
> > >  	mem_cgroup_add_lru_list(page, to);
> > >  }
> > >  
> > > -int task_in_mem_cgroup(struct task_struct *task, const struct mem_cgroup *mem)
> > > +/*
> > > + * This function is called from OOM Killer. This checks the task is mm_owner
> > > + * and checks it's mem_cgroup is under oom.
> > > + */
> > > +int task_in_oom_mem_cgroup(struct task_struct *task,
> > > +		const struct mem_cgroup *mem)
> > >  {
> > > +	struct mm_struct *mm;
> > >  	int ret;
> > >  	struct mem_cgroup *curr = NULL;
> > >  
> > > -	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.  One of my patches 
to do the oom killer rewrite that I'll propose tomorrow actually removes a 
lot of that redundancy.

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