Unused check for thread group leader in mem_cgroup_move_task

PATCH NAK Nikanth Karthikesan 5 participants 10 messages Nov 29, 2008 – Dec 3, 2008 4 messages not in the archive

Subsystems (2)

Memory management memcg

lore

Share & tools

lore

Nikanth Karthikesan

author

2008-11-29 07:29 UTC #0 lore raw

PATCH 1/1 Unused check for thread group leader in mem_cgroup_move_task

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 <[email protected]>

---

mm/memcontrol.c +0 -0

index 866dcc7..26bc823 100644
@@ -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 <[email protected]>

---

mm/memcontrol.c +0 -0

index 866dcc7..8e9287d 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

A message that is not in the archive (sent off-list or not yet received): [email protected]

KAMEZAWA Hiroyuki

NAK

2008-12-01 01:12 UTC #2 lore raw

Quoted 4 lines · On Sat, 29 Nov 2008 12:59:27 +0530 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.

Quoted 8 lines

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 <[email protected]>

It's allowed.

Nack.

-Kame

Quoted 86 lines

---

mm/memcontrol.c +12 -0

index 866dcc7..26bc823 100644
@@ -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,

mm/memcontrol.c +1 -8

 				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 <[email protected]>

---

mm/memcontrol.c +0 -9

index 866dcc7..8e9287d 100644
@@ -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 [email protected] For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KAMEZAWA Hiroyuki

NAK

2008-12-01 01:12 UTC #3 lore raw

In reply to Nikanth Karthikesan

Quoted 4 lines · On Sat, 29 Nov 2008 12:59:27 +0530 Nikanth Karthikesan <[email protected]> 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.

Quoted 8 lines

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 <[email protected]>

It's allowed.

Nack.

-Kame

Quoted 81 lines

---

mm/memcontrol.c +12 -0

index 866dcc7..26bc823 100644
@@ -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,

mm/memcontrol.c +1 -8

 				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 <[email protected]>

---

mm/memcontrol.c +0 -9

index 866dcc7..8e9287d 100644
@@ -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);
 }
 
 
--

Quoted 5 lines

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Nikanth Karthikesan

author

2008-12-01 04:21 UTC #4 lore raw

Quoted 10 lines · On Monday 01 December 2008 06:42:08 KAMEZAWA Hiroyuki wrote:

On Sat, 29 Nov 2008 12:59:27 +0530
Quoted 7 lines · 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

Quoted 9 lines

Quoted 4 lines
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 <[email protected]>
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 <[email protected]>

---

mm/memcontrol.c +0 -0

index 866dcc7..8e9287d 100644
@@ -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);
}

A message that is not in the archive (sent off-list or not yet received): [email protected]

KAMEZAWA Hiroyuki

Reviewed-by

2008-12-01 04:30 UTC #6 lore raw

Quoted 6 lines · On Mon, 1 Dec 2008 09:51:35 +0530 Nikanth Karthikesan…

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 <[email protected]>

Anyway we have to visit here, again.

Quoted 28 lines

Remove the unused test for thread group leader in mem_cgroup_move_task.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

mm/memcontrol.c +0 -8

index 866dcc7..8e9287d 100644
@@ -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);
 }

KAMEZAWA Hiroyuki

Reviewed-by

2008-12-01 04:31 UTC #7 lore raw

In reply to Nikanth Karthikesan

Quoted 6 lines · On Mon, 1 Dec 2008 09:51:35 +0530 Nikanth Karthikesan <[email protected]> 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 <[email protected]>

Anyway we have to visit here, again.

Quoted 18 lines

Remove the unused test for thread group leader in mem_cgroup_move_task.
Signed-off-by Nikanth Karthikesan <[email protected]>
---
mm/memcontrol.c +0 -0
index 866dcc7..8e9287d 100644
@@ -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);
 

mm/memcontrol.c static void mem_cgroup_move_task(struct cgroup_subsys · line 1156

Quoted 10 lines

mm/memcontrol.c +0 -8

-	/*
-	 * 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);
 }

Balbir Singh

2008-12-03 13:40 UTC #8 lore raw

Quoted 9 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · On Mon, 1 Dec 2008 09:51:35 +0530 Nikanth Karthikesan…
Ok. Then should we remove the unused code which simply checks for thread group leader but does nothing?
Thanks
Nikanth

Quoted 6 lines

Hmm, it seem that code is obsolete. thanks.
Balbir, how do you think ?
Reviewed-by KAMEZAWA Hiroyuki <[email protected]>
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.

Signature · 1 line

	Balbir

A message that is not in the archive (sent off-list or not yet received): 20081203134024.GD17701-SINUvgVNF2CyUtPGxGje5AC/[email protected]

KAMEZAWA Hiroyuki

2008-12-03 16:08 UTC #10 lore raw

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

Quoted 13 lines · Balbir Singh

Quoted 13 lines
Quoted 1 line · On Mon, 1 Dec 2008 09:51:35 +0530 Nikanth Karthikesan…
Ok. Then should we remove the unused code which simply checks for
thread group
Quoted 5 lines
leader but does nothing?
Thanks
Nikanth
Hmm, it seem that code is obsolete. thanks.
Balbir, how do you think ?

Quoted 9 lines

Quoted 3 lines
Reviewed-by KAMEZAWA Hiroyuki <[email protected]>
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

KAMEZAWA Hiroyuki

2008-12-03 16:08 UTC #11 lore raw

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

In reply to Balbir Singh

Quoted 13 lines · Balbir Singh

Quoted 13 lines
Quoted 1 line · On Mon, 1 Dec 2008 09:51:35 +0530 Nikanth Karthikesan <[email protected]> wrote:
Ok. Then should we remove the unused code which simply checks for
thread group
Quoted 5 lines
leader but does nothing?
Thanks
Nikanth
Hmm, it seem that code is obsolete. thanks.
Balbir, how do you think ?

Quoted 9 lines

Quoted 3 lines
Reviewed-by KAMEZAWA Hiroyuki <[email protected]>
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

A message that is not in the archive (sent off-list or not yet received): 20081201101208.08e0aa98.kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org

Nikanth Karthikesan

2008-12-01 04:21 UTC #13 lore raw

Quoted 10 lines · On Monday 01 December 2008 06:42:08 KAMEZAWA Hiroyuki wrote:

On Sat, 29 Nov 2008 12:59:27 +0530
Quoted 7 lines · 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

Quoted 9 lines

Quoted 4 lines
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 <[email protected]>
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 <[email protected]>

---

mm/memcontrol.c +0 -0

index 866dcc7..8e9287d 100644
@@ -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);
}