Re: [PATCH -mmotm 2/8] memcg: move memcg_tasklist mutex

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Fri, 6 Nov 2009 14:11:49 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> memcg_tasklist was introduced to serialize mem_cgroup_out_of_memory() and
> mem_cgroup_move_task() to ensure tasks cannot be moved to another cgroup
> during select_bad_process().
> 
> task_in_mem_cgroup(), which can be called by select_bad_process(), will check
> whether a task is in the mem_cgroup or not by dereferencing task->cgroups
> ->subsys[]. So, it would be desirable to change task->cgroups
> (rcu_assign_pointer() in cgroup_attach_task() does it) with memcg_tasklist held.
> 
> Now that we can define cancel_attach(), we can safely release memcg_tasklist
> on fail path even if we hold memcg_tasklist in can_attach(). So let's move
> mutex_lock/unlock() of memcg_tasklist.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> ---
>  mm/memcontrol.c |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4bd3451..d3b2ac0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3395,18 +3395,34 @@ static int mem_cgroup_populate(struct cgroup_subsys *ss,
>  	return ret;
>  }
>  
> +static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
> +				struct cgroup *cgroup,
> +				struct task_struct *p,
> +				bool threadgroup)
> +{
> +	mutex_lock(&memcg_tasklist);
> +	return 0;
> +}

Hmm...Is this lock really necessary ?
IOW, can't we just remove memcg_tasklist mutex ?
What kind of bad race happens when we remove this ?

Thanks,
-Kame

> +
> +static void mem_cgroup_cancel_attach(struct cgroup_subsys *ss,
> +				struct cgroup *cgroup,
> +				struct task_struct *p,
> +				bool threadgroup)
> +{
> +	mutex_unlock(&memcg_tasklist);
> +}
> +
>  static void mem_cgroup_move_task(struct cgroup_subsys *ss,
>  				struct cgroup *cont,
>  				struct cgroup *old_cont,
>  				struct task_struct *p,
>  				bool threadgroup)
>  {
> -	mutex_lock(&memcg_tasklist);
> +	mutex_unlock(&memcg_tasklist);
>  	/*
>  	 * FIXME: It's better to move charges of this process from old
>  	 * memcg to new memcg. But it's just on TODO-List now.
>  	 */
> -	mutex_unlock(&memcg_tasklist);
>  }
>  
>  struct cgroup_subsys mem_cgroup_subsys = {
> @@ -3416,6 +3432,8 @@ struct cgroup_subsys mem_cgroup_subsys = {
>  	.pre_destroy = mem_cgroup_pre_destroy,
>  	.destroy = mem_cgroup_destroy,
>  	.populate = mem_cgroup_populate,
> +	.can_attach = mem_cgroup_can_attach,
> +	.cancel_attach = mem_cgroup_cancel_attach,
>  	.attach = mem_cgroup_move_task,
>  	.early_init = 0,
>  	.use_id = 1,


Thanks,
-Kame




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