[PATCH] Unused check for thread group leader in mem_cgroup_move_task

[PATCH] Unused check for thread group leader in mem_cgroup_move_task

[Nikanth Karthikesan]

Currently we just check for thread group leader in attach() handler but do 
nothing!  Either (1) move it to can_attach handler or (2) remove the test 
itself. I am attaching patches for both below.

Thanks
Nikanth Karthikesan

Move thread group leader check to can_attach handler, but this may prevent non 
thread group leaders to be moved at all! 

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 866dcc7..26bc823 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1136,6 +1136,18 @@ static int mem_cgroup_populate(struct cgroup_subsys 
*ss,
 					ARRAY_SIZE(mem_cgroup_files));
 }
 
+static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
+                          struct cgroup *cgrp, struct task_struct *tsk)
+{
+	/*
+	 * Only thread group leaders are allowed to migrate, the mm_struct is
+	 * in effect owned by the leader
+	 */
+	if (!thread_group_leader(tsk))
+		return -EINVAL;
+	return 0;
+}
+
 static void mem_cgroup_move_task(struct cgroup_subsys *ss,
 				struct cgroup *cont,
 				struct cgroup *old_cont,
@@ -1151,14 +1163,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys 
*ss,
 	mem = mem_cgroup_from_cont(cont);
 	old_mem = mem_cgroup_from_cont(old_cont);
 
-	/*
-	 * Only thread group leaders are allowed to migrate, the mm_struct is
-	 * in effect owned by the leader
-	 */
-	if (!thread_group_leader(p))
-		goto out;
-
-out:
 	mmput(mm);
 }
 
@@ -1169,6 +1173,7 @@ 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,
 	.attach = mem_cgroup_move_task,
 	.early_init = 0,
 };



The patch to remove unused code follows.

Remove the unused test for thread group leader.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 866dcc7..8e9287d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1151,14 +1151,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys 
*ss,
 	mem = mem_cgroup_from_cont(cont);
 	old_mem = mem_cgroup_from_cont(old_cont);
 
-	/*
-	 * Only thread group leaders are allowed to migrate, the mm_struct is
-	 * in effect owned by the leader
-	 */
-	if (!thread_group_leader(p))
-		goto out;
-
-out:
 	mmput(mm);
 }
 

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

Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task

[KAMEZAWA Hiroyuki]

On Sat, 29 Nov 2008 12:59:27 +0530
Nikanth Karthikesan <knikanth@suse.de> wrote:

> Currently we just check for thread group leader in attach() handler but do 
> nothing!  Either (1) move it to can_attach handler or (2) remove the test 
> itself. I am attaching patches for both below.
> 
> Thanks
> Nikanth Karthikesan
> 
> Move thread group leader check to can_attach handler, but this may prevent non 
> thread group leaders to be moved at all! 
> 
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
> 
It's allowed.

Nack.

-Kame



> ---
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 866dcc7..26bc823 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1136,6 +1136,18 @@ static int mem_cgroup_populate(struct cgroup_subsys 
> *ss,
>  					ARRAY_SIZE(mem_cgroup_files));
>  }
>  
> +static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
> +                          struct cgroup *cgrp, struct task_struct *tsk)
> +{
> +	/*
> +	 * Only thread group leaders are allowed to migrate, the mm_struct is
> +	 * in effect owned by the leader
> +	 */
> +	if (!thread_group_leader(tsk))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  static void mem_cgroup_move_task(struct cgroup_subsys *ss,
>  				struct cgroup *cont,
>  				struct cgroup *old_cont,
> @@ -1151,14 +1163,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys 
> *ss,
>  	mem = mem_cgroup_from_cont(cont);
>  	old_mem = mem_cgroup_from_cont(old_cont);
>  
> -	/*
> -	 * Only thread group leaders are allowed to migrate, the mm_struct is
> -	 * in effect owned by the leader
> -	 */
> -	if (!thread_group_leader(p))
> -		goto out;
> -
> -out:
>  	mmput(mm);
>  }
>  
> @@ -1169,6 +1173,7 @@ 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,
>  	.attach = mem_cgroup_move_task,
>  	.early_init = 0,
>  };
> 
> 
> 
> The patch to remove unused code follows.
> 
> Remove the unused test for thread group leader.
> 
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
> 
> ---
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 866dcc7..8e9287d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1151,14 +1151,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys 
> *ss,
>  	mem = mem_cgroup_from_cont(cont);
>  	old_mem = mem_cgroup_from_cont(old_cont);
>  
> -	/*
> -	 * Only thread group leaders are allowed to migrate, the mm_struct is
> -	 * in effect owned by the leader
> -	 */
> -	if (!thread_group_leader(p))
> -		goto out;
> -
> -out:
>  	mmput(mm);
>  }
>  
> 
> --
> 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>
> 

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

Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task

[Nikanth Karthikesan]

On Monday 01 December 2008 06:42:08 KAMEZAWA Hiroyuki wrote:
> On Sat, 29 Nov 2008 12:59:27 +0530
>
> Nikanth Karthikesan <knikanth@suse.de> wrote:
> > Currently we just check for thread group leader in attach() handler but
> > do nothing!  Either (1) move it to can_attach handler or (2) remove the
> > test itself. I am attaching patches for both below.
> >
> > Thanks
> > Nikanth Karthikesan
> >
> > Move thread group leader check to can_attach handler, but this may
> > prevent non thread group leaders to be moved at all!
> >
> > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> It's allowed.
>
> Nack.
>

Ok. Then should we remove the unused code which simply checks for thread group 
leader but does nothing?
 
Thanks
Nikanth

Remove the unused test for thread group leader in mem_cgroup_move_task.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 866dcc7..8e9287d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1151,14 +1151,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys 
*ss,
 	mem = mem_cgroup_from_cont(cont);
 	old_mem = mem_cgroup_from_cont(old_cont);
 
-	/*
-	 * Only thread group leaders are allowed to migrate, the mm_struct is
-	 * in effect owned by the leader
-	 */
-	if (!thread_group_leader(p))
-		goto out;
-
-out:
 	mmput(mm);
 }
 

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

Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task

[KAMEZAWA Hiroyuki]

On Mon, 1 Dec 2008 09:51:35 +0530
Nikanth Karthikesan <knikanth@suse.de> wrote:

> Ok. Then should we remove the unused code which simply checks for thread group 
> leader but does nothing?
>  
> Thanks
> Nikanth
> 
Hmm, it seem that code is obsolete. thanks.
Balbir, how do you think ?

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Anyway we have to visit here, again.


> Remove the unused test for thread group leader in mem_cgroup_move_task.
> 
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
> 
> ---
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 866dcc7..8e9287d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1151,14 +1151,6 @@ static void mem_cgroup_move_task(struct cgroup_subsys 
> *ss,
>  	mem = mem_cgroup_from_cont(cont);
>  	old_mem = mem_cgroup_from_cont(old_cont);
>  
> -	/*
> -	 * Only thread group leaders are allowed to migrate, the mm_struct is
> -	 * in effect owned by the leader
> -	 */
> -	if (!thread_group_leader(p))
> -		goto out;
> -
> -out:
>  	mmput(mm);
>  }
>  
> 
> 

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

Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2008-12-01 13:30:30]:

> On Mon, 1 Dec 2008 09:51:35 +0530
> Nikanth Karthikesan <knikanth@suse.de> wrote:
> 
> > Ok. Then should we remove the unused code which simply checks for thread group 
> > leader but does nothing?
> >  
> > Thanks
> > Nikanth
> > 
> Hmm, it seem that code is obsolete. thanks.
> Balbir, how do you think ?
> 
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Anyway we have to visit here, again.

Sorry, I did not review this patch. The correct thing was nikanth did
at first, move this to can_attach(). Why would we allow threads to
exist in different groups, but still mark them as being accounted to
the thread group leader.

It can be a bit confusing for end users, it can be helpful when all
controllers are mounted together. I agree we did not do anything
useful in move_task(). The correct check now, should be for mm->owner.

If the common case is going to be that memory and cpu are mounted
together, then this patch is correct, but it can be confusing to users
who look at tasks/threads, but as the threads consume memory, the
accounting will happen with mm->owner.

-- 
	Balbir

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

Re: [PATCH] Unused check for thread group leader inmem_cgroup_move_task

[KAMEZAWA Hiroyuki]

Balbir Singh said:
>> On Mon, 1 Dec 2008 09:51:35 +0530
>> Nikanth Karthikesan <knikanth@suse.de> wrote:
>>
>> > Ok. Then should we remove the unused code which simply checks for
>> thread group
>> > leader but does nothing?
>> >
>> > Thanks
>> > Nikanth
>> >
>> Hmm, it seem that code is obsolete. thanks.
>> Balbir, how do you think ?
>>
>> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>
>> Anyway we have to visit here, again.
>
> Sorry, I did not review this patch. The correct thing was nikanth did
> at first, move this to can_attach(). Why would we allow threads to
> exist in different groups, but still mark them as being accounted to
> the thread group leader.
>
Considering following case,

  # mount -t cgroup none /cgroup -t memory,cpuset
  # mkdir /cgroup/grpA/
  # echo 1 > /cgroup/grpA/memory.use_hierarchy
  # echo 1G > /cgroup/grpA/memory.limit_in_bytes
  # mkdir /cgroup/grpA/01
  # mkdir /cgroup/grpA/02
  limit grpA/01's cpu to 0
  limit grpA/02's cpu to 1

  Run multithread program under grpA and move threads to grpA/01, grpA/02
  if necessary to bind cpu.

Your "hierarchy" added this kind of flexibility and this is very useful.
And, of course, cgroup generic interface allows per-thread group attaching.

This is why I changed my mind and agreed to handle hierarchy management
under the kernel.

This can be an answer for use case to explain why thread-leader-check is
bad ? If we add limitation as "memcgroup should be mounted without
others",

And, if we only allows attaching thread-group-leader, how to migrate
multi-threaded program's all thread ?

I'm sorry I misunderstand something.

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