* [PATCH] Unused check for thread group leader in mem_cgroup_move_task
@ 2008-11-29 7:29 Nikanth Karthikesan
2008-12-01 1:12 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 6+ messages in thread
From: Nikanth Karthikesan @ 2008-11-29 7:29 UTC (permalink / raw)
To: balbir; +Cc: containers, xemul, kamezawa.hiroyu, linux-mm, nikanth
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task
2008-11-29 7:29 [PATCH] Unused check for thread group leader in mem_cgroup_move_task Nikanth Karthikesan
@ 2008-12-01 1:12 ` KAMEZAWA Hiroyuki
2008-12-01 4:21 ` Nikanth Karthikesan
0 siblings, 1 reply; 6+ messages in thread
From: KAMEZAWA Hiroyuki @ 2008-12-01 1:12 UTC (permalink / raw)
To: Nikanth Karthikesan; +Cc: balbir, containers, xemul, linux-mm, nikanth
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task
2008-12-01 1:12 ` KAMEZAWA Hiroyuki
@ 2008-12-01 4:21 ` Nikanth Karthikesan
2008-12-01 4:30 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 6+ messages in thread
From: Nikanth Karthikesan @ 2008-12-01 4:21 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: balbir, containers, xemul, linux-mm, nikanth
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task
2008-12-01 4:21 ` Nikanth Karthikesan
@ 2008-12-01 4:30 ` KAMEZAWA Hiroyuki
2008-12-03 13:40 ` Balbir Singh
0 siblings, 1 reply; 6+ messages in thread
From: KAMEZAWA Hiroyuki @ 2008-12-01 4:30 UTC (permalink / raw)
To: Nikanth Karthikesan, balbir; +Cc: containers, xemul, linux-mm, nikanth
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unused check for thread group leader in mem_cgroup_move_task
2008-12-01 4:30 ` KAMEZAWA Hiroyuki
@ 2008-12-03 13:40 ` Balbir Singh
2008-12-03 16:08 ` [PATCH] Unused check for thread group leader inmem_cgroup_move_task KAMEZAWA Hiroyuki
0 siblings, 1 reply; 6+ messages in thread
From: Balbir Singh @ 2008-12-03 13:40 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Nikanth Karthikesan, containers, xemul, linux-mm, nikanth
* 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unused check for thread group leader inmem_cgroup_move_task
2008-12-03 13:40 ` Balbir Singh
@ 2008-12-03 16:08 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 6+ messages in thread
From: KAMEZAWA Hiroyuki @ 2008-12-03 16:08 UTC (permalink / raw)
To: balbir
Cc: KAMEZAWA Hiroyuki, Nikanth Karthikesan, containers, xemul,
linux-mm, nikanth
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-12-03 16:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-11-29 7:29 [PATCH] Unused check for thread group leader in mem_cgroup_move_task Nikanth Karthikesan
2008-12-01 1:12 ` KAMEZAWA Hiroyuki
2008-12-01 4:21 ` Nikanth Karthikesan
2008-12-01 4:30 ` KAMEZAWA Hiroyuki
2008-12-03 13:40 ` Balbir Singh
2008-12-03 16:08 ` [PATCH] Unused check for thread group leader inmem_cgroup_move_task KAMEZAWA Hiroyuki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox