[PATCH 0/5] cgroup: some cleanups of cgroup_attach_proc

[PATCH 0/5] cgroup: some cleanups of cgroup_attach_proc

[Mandeep Singh Baines]

Hello,

This series is a series of cleanups to cgroup_attach_proc. The
series will apply cleanly against cgroup/for-3.3.

There may be some collisions once Frederic's series is applied:

https://lkml.org/lkml/2011/12/20/260

So I may need to resend.

[1 V3] cgroup: remove redundate get/put of old css_set from migrate
[2] cgroup: remove redundant get/put of task struct
[3] cgroup: only need to check oldcgrp==newgrp once
[4] cgroup: remove extra calls to find_existing_css_set
[5] cgroup: separate out cgroup_attach_proc error handling code

Regards,
Mandeep

[PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Mandeep Singh Baines]

Changes in V3:
* Fixed to put error in retval (thanks Frederic Weisbecker)
  * https://lkml.org/lkml/2011/12/20/255
Changes in V2:
* Updated commit message as per Tejun's feedback:
  * https://lkml.org/lkml/2011/12/19/289

-- >8 -- (snip)

We can now assume that the css_set reference held by the task
will not go away for an exiting task. PF_EXITING state can be
trusted throughout migration by checking it after locking
threadgroup.

While at it, renamed css_set_check_fetched to css_set_fetched.
!css_set_fetched() seems to read better than
!css_set_check_fetched().

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index bc3caff..4166066 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1856,7 +1856,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 	 */
 	task_lock(tsk);
 	oldcg = tsk->cgroups;
-	get_css_set(oldcg);
 	task_unlock(tsk);
 
 	/* locate or allocate a new css_set for this task. */
@@ -1872,12 +1871,9 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 		might_sleep();
 		/* find_css_set will give us newcg already referenced. */
 		newcg = find_css_set(oldcg, cgrp);
-		if (!newcg) {
-			put_css_set(oldcg);
+		if (!newcg)
 			return -ENOMEM;
-		}
 	}
-	put_css_set(oldcg);
 
 	/* @tsk can't exit as its threadgroup is locked */
 	task_lock(tsk);
@@ -2015,9 +2011,8 @@ struct cg_list_entry {
 	struct list_head links;
 };
 
-static bool css_set_check_fetched(struct cgroup *cgrp,
-				  struct task_struct *tsk, struct css_set *cg,
-				  struct list_head *newcg_list)
+static bool css_set_fetched(struct cgroup *cgrp, struct task_struct *tsk,
+			    struct css_set *cg, struct list_head *newcg_list)
 {
 	struct css_set *newcg;
 	struct cg_list_entry *cg_entry;
@@ -2185,19 +2180,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
-		get_css_set(oldcg);
 		task_unlock(tc->task);
-		/* see if the new one for us is already in the list? */
-		if (css_set_check_fetched(cgrp, tc->task, oldcg, &newcg_list)) {
-			/* was already there, nothing to do. */
-			put_css_set(oldcg);
-		} else {
-			/* we don't already have it. get new one. */
-			retval = css_set_prefetch(cgrp, oldcg, &newcg_list);
-			put_css_set(oldcg);
-			if (retval)
+		/* if we don't already have it in the list get a new one */
+		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
+			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
 				goto out_list_teardown;
-		}
 	}
 
 	/*
-- 
1.7.3.1

[PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Mandeep Singh Baines]

Changes in V3:
* Fixed to put error in retval (thanks Frederic Weisbecker)
  * https://lkml.org/lkml/2011/12/20/255
Changes in V2:
* Updated commit message as per Tejun's feedback:
  * https://lkml.org/lkml/2011/12/19/289

-- >8 -- (snip)

We can now assume that the css_set reference held by the task
will not go away for an exiting task. PF_EXITING state can be
trusted throughout migration by checking it after locking
threadgroup.

While at it, renamed css_set_check_fetched to css_set_fetched.
!css_set_fetched() seems to read better than
!css_set_check_fetched().

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: containers@lists.linux-foundation.org
Cc: cgroups@vger.kernel.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Paul Menage <paul@paulmenage.org>
---
 kernel/cgroup.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index bc3caff..4166066 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1856,7 +1856,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 	 */
 	task_lock(tsk);
 	oldcg = tsk->cgroups;
-	get_css_set(oldcg);
 	task_unlock(tsk);
 
 	/* locate or allocate a new css_set for this task. */
@@ -1872,12 +1871,9 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 		might_sleep();
 		/* find_css_set will give us newcg already referenced. */
 		newcg = find_css_set(oldcg, cgrp);
-		if (!newcg) {
-			put_css_set(oldcg);
+		if (!newcg)
 			return -ENOMEM;
-		}
 	}
-	put_css_set(oldcg);
 
 	/* @tsk can't exit as its threadgroup is locked */
 	task_lock(tsk);
@@ -2015,9 +2011,8 @@ struct cg_list_entry {
 	struct list_head links;
 };
 
-static bool css_set_check_fetched(struct cgroup *cgrp,
-				  struct task_struct *tsk, struct css_set *cg,
-				  struct list_head *newcg_list)
+static bool css_set_fetched(struct cgroup *cgrp, struct task_struct *tsk,
+			    struct css_set *cg, struct list_head *newcg_list)
 {
 	struct css_set *newcg;
 	struct cg_list_entry *cg_entry;
@@ -2185,19 +2180,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
-		get_css_set(oldcg);
 		task_unlock(tc->task);
-		/* see if the new one for us is already in the list? */
-		if (css_set_check_fetched(cgrp, tc->task, oldcg, &newcg_list)) {
-			/* was already there, nothing to do. */
-			put_css_set(oldcg);
-		} else {
-			/* we don't already have it. get new one. */
-			retval = css_set_prefetch(cgrp, oldcg, &newcg_list);
-			put_css_set(oldcg);
-			if (retval)
+		/* if we don't already have it in the list get a new one */
+		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
+			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
 				goto out_list_teardown;
-		}
 	}
 
 	/*
-- 
1.7.3.1

[PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Mandeep Singh Baines]

Changes in V3:
* Fixed to put error in retval (thanks Frederic Weisbecker)
  * https://lkml.org/lkml/2011/12/20/255
Changes in V2:
* Updated commit message as per Tejun's feedback:
  * https://lkml.org/lkml/2011/12/19/289

-- >8 -- (snip)

We can now assume that the css_set reference held by the task
will not go away for an exiting task. PF_EXITING state can be
trusted throughout migration by checking it after locking
threadgroup.

While at it, renamed css_set_check_fetched to css_set_fetched.
!css_set_fetched() seems to read better than
!css_set_check_fetched().

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index bc3caff..4166066 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1856,7 +1856,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 	 */
 	task_lock(tsk);
 	oldcg = tsk->cgroups;
-	get_css_set(oldcg);
 	task_unlock(tsk);
 
 	/* locate or allocate a new css_set for this task. */
@@ -1872,12 +1871,9 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 		might_sleep();
 		/* find_css_set will give us newcg already referenced. */
 		newcg = find_css_set(oldcg, cgrp);
-		if (!newcg) {
-			put_css_set(oldcg);
+		if (!newcg)
 			return -ENOMEM;
-		}
 	}
-	put_css_set(oldcg);
 
 	/* @tsk can't exit as its threadgroup is locked */
 	task_lock(tsk);
@@ -2015,9 +2011,8 @@ struct cg_list_entry {
 	struct list_head links;
 };
 
-static bool css_set_check_fetched(struct cgroup *cgrp,
-				  struct task_struct *tsk, struct css_set *cg,
-				  struct list_head *newcg_list)
+static bool css_set_fetched(struct cgroup *cgrp, struct task_struct *tsk,
+			    struct css_set *cg, struct list_head *newcg_list)
 {
 	struct css_set *newcg;
 	struct cg_list_entry *cg_entry;
@@ -2185,19 +2180,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
-		get_css_set(oldcg);
 		task_unlock(tc->task);
-		/* see if the new one for us is already in the list? */
-		if (css_set_check_fetched(cgrp, tc->task, oldcg, &newcg_list)) {
-			/* was already there, nothing to do. */
-			put_css_set(oldcg);
-		} else {
-			/* we don't already have it. get new one. */
-			retval = css_set_prefetch(cgrp, oldcg, &newcg_list);
-			put_css_set(oldcg);
-			if (retval)
+		/* if we don't already have it in the list get a new one */
+		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
+			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
 				goto out_list_teardown;
-		}
 	}
 
 	/*
-- 
1.7.3.1

[PATCH 2/5] cgroup: remove redundant get/put of task struct

[Mandeep Singh Baines]

threadgroup_lock() guarantees that the target threadgroup will
remain stable - no new task will be added, no new PF_EXITING
will be set and exec won't happen.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4166066..6649529 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2130,7 +2130,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 
 		/* as per above, nr_threads may decrease, but not increase. */
 		BUG_ON(i >= group_size);
-		get_task_struct(tsk);
 		/*
 		 * saying GFP_ATOMIC has no effect here because we did prealloc
 		 * earlier, but it's good form to communicate our expectations.
@@ -2152,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	/* methods shouldn't be called if no task is actually migrating */
 	retval = 0;
 	if (!nr_migrating_tasks)
-		goto out_put_tasks;
+		goto out_free_group_list;
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
@@ -2233,12 +2232,6 @@ out_cancel_attach:
 				ss->cancel_attach(ss, cgrp, &tset);
 		}
 	}
-out_put_tasks:
-	/* clean up the array of referenced threads in the group. */
-	for (i = 0; i < group_size; i++) {
-		tc = flex_array_get(group, i);
-		put_task_struct(tc->task);
-	}
 out_free_group_list:
 	flex_array_free(group);
 	return retval;
@@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
 			cgroup_unlock();
 			return -EACCES;
 		}
-		get_task_struct(tsk);
 		rcu_read_unlock();
 	} else {
 		if (threadgroup)
 			tsk = current->group_leader;
 		else
 			tsk = current;
-		get_task_struct(tsk);
 	}
 
 	threadgroup_lock(tsk);
@@ -2306,7 +2297,6 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
 
 	threadgroup_unlock(tsk);
 
-	put_task_struct(tsk);
 	cgroup_unlock();
 	return ret;
 }
-- 
1.7.3.1

[PATCH 2/5] cgroup: remove redundant get/put of task struct

[Mandeep Singh Baines]

threadgroup_lock() guarantees that the target threadgroup will
remain stable - no new task will be added, no new PF_EXITING
will be set and exec won't happen.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: containers@lists.linux-foundation.org
Cc: cgroups@vger.kernel.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Paul Menage <paul@paulmenage.org>
---
 kernel/cgroup.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4166066..6649529 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2130,7 +2130,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 
 		/* as per above, nr_threads may decrease, but not increase. */
 		BUG_ON(i >= group_size);
-		get_task_struct(tsk);
 		/*
 		 * saying GFP_ATOMIC has no effect here because we did prealloc
 		 * earlier, but it's good form to communicate our expectations.
@@ -2152,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	/* methods shouldn't be called if no task is actually migrating */
 	retval = 0;
 	if (!nr_migrating_tasks)
-		goto out_put_tasks;
+		goto out_free_group_list;
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
@@ -2233,12 +2232,6 @@ out_cancel_attach:
 				ss->cancel_attach(ss, cgrp, &tset);
 		}
 	}
-out_put_tasks:
-	/* clean up the array of referenced threads in the group. */
-	for (i = 0; i < group_size; i++) {
-		tc = flex_array_get(group, i);
-		put_task_struct(tc->task);
-	}
 out_free_group_list:
 	flex_array_free(group);
 	return retval;
@@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
 			cgroup_unlock();
 			return -EACCES;
 		}
-		get_task_struct(tsk);
 		rcu_read_unlock();
 	} else {
 		if (threadgroup)
 			tsk = current->group_leader;
 		else
 			tsk = current;
-		get_task_struct(tsk);
 	}
 
 	threadgroup_lock(tsk);
@@ -2306,7 +2297,6 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
 
 	threadgroup_unlock(tsk);
 
-	put_task_struct(tsk);
 	cgroup_unlock();
 	return ret;
 }
-- 
1.7.3.1

[PATCH 2/5] cgroup: remove redundant get/put of task struct

[Mandeep Singh Baines]

threadgroup_lock() guarantees that the target threadgroup will
remain stable - no new task will be added, no new PF_EXITING
will be set and exec won't happen.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4166066..6649529 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2130,7 +2130,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 
 		/* as per above, nr_threads may decrease, but not increase. */
 		BUG_ON(i >= group_size);
-		get_task_struct(tsk);
 		/*
 		 * saying GFP_ATOMIC has no effect here because we did prealloc
 		 * earlier, but it's good form to communicate our expectations.
@@ -2152,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	/* methods shouldn't be called if no task is actually migrating */
 	retval = 0;
 	if (!nr_migrating_tasks)
-		goto out_put_tasks;
+		goto out_free_group_list;
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
@@ -2233,12 +2232,6 @@ out_cancel_attach:
 				ss->cancel_attach(ss, cgrp, &tset);
 		}
 	}
-out_put_tasks:
-	/* clean up the array of referenced threads in the group. */
-	for (i = 0; i < group_size; i++) {
-		tc = flex_array_get(group, i);
-		put_task_struct(tc->task);
-	}
 out_free_group_list:
 	flex_array_free(group);
 	return retval;
@@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
 			cgroup_unlock();
 			return -EACCES;
 		}
-		get_task_struct(tsk);
 		rcu_read_unlock();
 	} else {
 		if (threadgroup)
 			tsk = current->group_leader;
 		else
 			tsk = current;
-		get_task_struct(tsk);
 	}
 
 	threadgroup_lock(tsk);
@@ -2306,7 +2297,6 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
 
 	threadgroup_unlock(tsk);
 
-	put_task_struct(tsk);
 	cgroup_unlock();
 	return ret;
 }
-- 
1.7.3.1

[PATCH 3/5] cgroup: only need to check oldcgrp==newgrp once

[Mandeep Singh Baines]

In cgroup_attach_proc it is now sufficient to only check that
oldcgrp==newcgrp once. Now that we are using threadgroup_lock()
during the migrations, oldcgrp will not change.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 6649529..4668a3f 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2069,7 +2069,7 @@ static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
  */
 int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
-	int retval, i, group_size, nr_migrating_tasks;
+	int retval, i, group_size;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
@@ -2120,7 +2120,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	}
 	/* take a reference on each task in the group to go in the array. */
 	tsk = leader;
-	i = nr_migrating_tasks = 0;
+	i = 0;
 	do {
 		struct task_and_cgroup ent;
 
@@ -2136,11 +2136,12 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		 */
 		ent.task = tsk;
 		ent.cgrp = task_cgroup_from_root(tsk, root);
+		/* nothing to do if this task is already in the cgroup */
+		if (ent.cgrp == cgrp)
+			continue;
 		retval = flex_array_put(group, i, &ent, GFP_ATOMIC);
 		BUG_ON(retval != 0);
 		i++;
-		if (ent.cgrp != cgrp)
-			nr_migrating_tasks++;
 	} while_each_thread(leader, tsk);
 	/* remember the number of threads in the array for later. */
 	group_size = i;
@@ -2150,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 
 	/* methods shouldn't be called if no task is actually migrating */
 	retval = 0;
-	if (!nr_migrating_tasks)
+	if (!group_size)
 		goto out_free_group_list;
 
 	/*
@@ -2173,9 +2174,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	INIT_LIST_HEAD(&newcg_list);
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		/* nothing to do if this task is already in the cgroup */
-		if (tc->cgrp == cgrp)
-			continue;
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
@@ -2193,9 +2191,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		/* leave current thread as it is if it's already there */
-		if (tc->cgrp == cgrp)
-			continue;
 		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
 		BUG_ON(retval);
 	}
-- 
1.7.3.1

[PATCH 3/5] cgroup: only need to check oldcgrp==newgrp once

[Mandeep Singh Baines]

In cgroup_attach_proc it is now sufficient to only check that
oldcgrp==newcgrp once. Now that we are using threadgroup_lock()
during the migrations, oldcgrp will not change.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: containers@lists.linux-foundation.org
Cc: cgroups@vger.kernel.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Paul Menage <paul@paulmenage.org>
---
 kernel/cgroup.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 6649529..4668a3f 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2069,7 +2069,7 @@ static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
  */
 int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
-	int retval, i, group_size, nr_migrating_tasks;
+	int retval, i, group_size;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
@@ -2120,7 +2120,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	}
 	/* take a reference on each task in the group to go in the array. */
 	tsk = leader;
-	i = nr_migrating_tasks = 0;
+	i = 0;
 	do {
 		struct task_and_cgroup ent;
 
@@ -2136,11 +2136,12 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		 */
 		ent.task = tsk;
 		ent.cgrp = task_cgroup_from_root(tsk, root);
+		/* nothing to do if this task is already in the cgroup */
+		if (ent.cgrp == cgrp)
+			continue;
 		retval = flex_array_put(group, i, &ent, GFP_ATOMIC);
 		BUG_ON(retval != 0);
 		i++;
-		if (ent.cgrp != cgrp)
-			nr_migrating_tasks++;
 	} while_each_thread(leader, tsk);
 	/* remember the number of threads in the array for later. */
 	group_size = i;
@@ -2150,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 
 	/* methods shouldn't be called if no task is actually migrating */
 	retval = 0;
-	if (!nr_migrating_tasks)
+	if (!group_size)
 		goto out_free_group_list;
 
 	/*
@@ -2173,9 +2174,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	INIT_LIST_HEAD(&newcg_list);
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		/* nothing to do if this task is already in the cgroup */
-		if (tc->cgrp == cgrp)
-			continue;
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
@@ -2193,9 +2191,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		/* leave current thread as it is if it's already there */
-		if (tc->cgrp == cgrp)
-			continue;
 		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
 		BUG_ON(retval);
 	}
-- 
1.7.3.1

[PATCH 3/5] cgroup: only need to check oldcgrp==newgrp once

[Mandeep Singh Baines]

In cgroup_attach_proc it is now sufficient to only check that
oldcgrp==newcgrp once. Now that we are using threadgroup_lock()
during the migrations, oldcgrp will not change.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 6649529..4668a3f 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2069,7 +2069,7 @@ static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
  */
 int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
-	int retval, i, group_size, nr_migrating_tasks;
+	int retval, i, group_size;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
@@ -2120,7 +2120,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	}
 	/* take a reference on each task in the group to go in the array. */
 	tsk = leader;
-	i = nr_migrating_tasks = 0;
+	i = 0;
 	do {
 		struct task_and_cgroup ent;
 
@@ -2136,11 +2136,12 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		 */
 		ent.task = tsk;
 		ent.cgrp = task_cgroup_from_root(tsk, root);
+		/* nothing to do if this task is already in the cgroup */
+		if (ent.cgrp == cgrp)
+			continue;
 		retval = flex_array_put(group, i, &ent, GFP_ATOMIC);
 		BUG_ON(retval != 0);
 		i++;
-		if (ent.cgrp != cgrp)
-			nr_migrating_tasks++;
 	} while_each_thread(leader, tsk);
 	/* remember the number of threads in the array for later. */
 	group_size = i;
@@ -2150,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 
 	/* methods shouldn't be called if no task is actually migrating */
 	retval = 0;
-	if (!nr_migrating_tasks)
+	if (!group_size)
 		goto out_free_group_list;
 
 	/*
@@ -2173,9 +2174,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	INIT_LIST_HEAD(&newcg_list);
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		/* nothing to do if this task is already in the cgroup */
-		if (tc->cgrp == cgrp)
-			continue;
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
@@ -2193,9 +2191,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		/* leave current thread as it is if it's already there */
-		if (tc->cgrp == cgrp)
-			continue;
 		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
 		BUG_ON(retval);
 	}
-- 
1.7.3.1

[PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Mandeep Singh Baines]

In cgroup_attach_proc, we indirectly call find_existing_css_set 3
times. It is an expensive call so we want to call it a minimum
of times. This patch only calls it once and stores the result so
that it can be used later on when we call cgroup_task_migrate.

This required modifying cgroup_task_migrate to take the new css_set
(which we obtained from find_css_set) as a parameter. The nice side
effect of this is that cgroup_task_migrate is now identical for
cgroup_attach_task and cgroup_attach_proc. It also now returns a
void since it can never fail.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |  138 ++++++++++--------------------------------------------
 1 files changed, 26 insertions(+), 112 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4668a3f..2ac9eee 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1763,6 +1763,7 @@ EXPORT_SYMBOL_GPL(cgroup_path);
 struct task_and_cgroup {
 	struct task_struct	*task;
 	struct cgroup		*cgrp;
+	struct css_set		*cg;
 };
 
 struct cgroup_taskset {
@@ -1843,40 +1844,14 @@ EXPORT_SYMBOL_GPL(cgroup_taskset_size);
  * will already exist. If not set, this function might sleep, and can fail with
  * -ENOMEM. Must be called with cgroup_mutex and threadgroup locked.
  */
-static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
-			       struct task_struct *tsk, bool guarantee)
+static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
+				struct task_struct *tsk, struct css_set *newcg)
 {
 	struct css_set *oldcg;
-	struct css_set *newcg;
-
-	/*
-	 * get old css_set. we need to take task_lock and refcount it, because
-	 * an exiting task can change its css_set to init_css_set and drop its
-	 * old one without taking cgroup_mutex.
-	 */
-	task_lock(tsk);
-	oldcg = tsk->cgroups;
-	task_unlock(tsk);
-
-	/* locate or allocate a new css_set for this task. */
-	if (guarantee) {
-		/* we know the css_set we want already exists. */
-		struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
-		read_lock(&css_set_lock);
-		newcg = find_existing_css_set(oldcg, cgrp, template);
-		BUG_ON(!newcg);
-		get_css_set(newcg);
-		read_unlock(&css_set_lock);
-	} else {
-		might_sleep();
-		/* find_css_set will give us newcg already referenced. */
-		newcg = find_css_set(oldcg, cgrp);
-		if (!newcg)
-			return -ENOMEM;
-	}
 
 	/* @tsk can't exit as its threadgroup is locked */
 	task_lock(tsk);
+	oldcg = tsk->cgroups;
 	WARN_ON_ONCE(tsk->flags & PF_EXITING);
 	rcu_assign_pointer(tsk->cgroups, newcg);
 	task_unlock(tsk);
@@ -1895,7 +1870,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 	put_css_set(oldcg);
 
 	set_bit(CGRP_RELEASABLE, &oldcgrp->flags);
-	return 0;
 }
 
 /**
@@ -1913,6 +1887,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 	struct cgroup *oldcgrp;
 	struct cgroupfs_root *root = cgrp->root;
 	struct cgroup_taskset tset = { };
+	struct css_set *newcg, *oldcg;
 
 	/* @tsk either already exited or can't exit until the end */
 	if (tsk->flags & PF_EXITING)
@@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		}
 	}
 
-	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
-	if (retval)
+	task_lock(tsk);
+	oldcg = tsk->cgroups;
+	task_unlock(tsk);
+
+	newcg = find_css_set(oldcg, cgrp);
+	if (!newcg) {
+		retval = -ENOMEM;
 		goto out;
+	}
+
+	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
 
 	for_each_subsys(root, ss) {
 		if (ss->attach)
@@ -2000,65 +1983,6 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
 }
 EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
 
-/*
- * cgroup_attach_proc works in two stages, the first of which prefetches all
- * new css_sets needed (to make sure we have enough memory before committing
- * to the move) and stores them in a list of entries of the following type.
- * TODO: possible optimization: use css_set->rcu_head for chaining instead
- */
-struct cg_list_entry {
-	struct css_set *cg;
-	struct list_head links;
-};
-
-static bool css_set_fetched(struct cgroup *cgrp, struct task_struct *tsk,
-			    struct css_set *cg, struct list_head *newcg_list)
-{
-	struct css_set *newcg;
-	struct cg_list_entry *cg_entry;
-	struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
-
-	read_lock(&css_set_lock);
-	newcg = find_existing_css_set(cg, cgrp, template);
-	read_unlock(&css_set_lock);
-
-	/* doesn't exist at all? */
-	if (!newcg)
-		return false;
-	/* see if it's already in the list */
-	list_for_each_entry(cg_entry, newcg_list, links)
-		if (cg_entry->cg == newcg)
-			return true;
-
-	/* not found */
-	return false;
-}
-
-/*
- * Find the new css_set and store it in the list in preparation for moving the
- * given task to the given cgroup. Returns 0 or -ENOMEM.
- */
-static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
-			    struct list_head *newcg_list)
-{
-	struct css_set *newcg;
-	struct cg_list_entry *cg_entry;
-
-	/* ensure a new css_set will exist for this thread */
-	newcg = find_css_set(cg, cgrp);
-	if (!newcg)
-		return -ENOMEM;
-	/* add it to the list */
-	cg_entry = kmalloc(sizeof(struct cg_list_entry), GFP_KERNEL);
-	if (!cg_entry) {
-		put_css_set(newcg);
-		return -ENOMEM;
-	}
-	cg_entry->cg = newcg;
-	list_add(&cg_entry->links, newcg_list);
-	return 0;
-}
-
 /**
  * cgroup_attach_proc - attach all threads in a threadgroup to a cgroup
  * @cgrp: the cgroup to attach to
@@ -2069,7 +1993,7 @@ static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
  */
 int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
-	int retval, i, group_size;
+	int retval, i, group_size, css_set_refs = 0;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
@@ -2079,13 +2003,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	struct task_and_cgroup *tc;
 	struct flex_array *group;
 	struct cgroup_taskset tset = { };
-	/*
-	 * we need to make sure we have css_sets for all the tasks we're
-	 * going to move -before- we actually start moving them, so that in
-	 * case we get an ENOMEM we can bail out before making any changes.
-	 */
-	struct list_head newcg_list;
-	struct cg_list_entry *cg_entry, *temp_nobe;
 
 	/*
 	 * step 0: in order to do expensive, possibly blocking operations for
@@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 * step 2: make sure css_sets exist for all threads to be migrated.
 	 * we use find_css_set, which allocates a new one if necessary.
 	 */
-	INIT_LIST_HEAD(&newcg_list);
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
 		task_unlock(tc->task);
-		/* if we don't already have it in the list get a new one */
-		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
-			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
-				goto out_list_teardown;
+		tc->cg = find_css_set(oldcg, cgrp);
+		if (!tc->cg) {
+			retval = -ENOMEM;
+			css_set_refs = i + 1;
+			goto out_list_teardown;
+		}
 	}
 
 	/*
@@ -2191,8 +2109,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
-		BUG_ON(retval);
+		cgroup_task_migrate(cgrp, tc->cgrp, tc->task, tc->cg);
 	}
 	/* nothing is sensitive to fork() after this point. */
 
@@ -2211,14 +2128,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	cgroup_wakeup_rmdir_waiter(cgrp);
 	retval = 0;
 out_list_teardown:
-	/* clean up the list of prefetched css_sets. */
-	list_for_each_entry_safe(cg_entry, temp_nobe, &newcg_list, links) {
-		list_del(&cg_entry->links);
-		put_css_set(cg_entry->cg);
-		kfree(cg_entry);
+	for (i = 0; i < css_set_refs; i++) {
+		tc = flex_array_get(group, i);
+		put_css_set(tc->cg);
 	}
 out_cancel_attach:
-	/* same deal as in cgroup_attach_task */
 	if (retval) {
 		for_each_subsys(root, ss) {
 			if (ss == failed_ss)
-- 
1.7.3.1

[PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Mandeep Singh Baines]

In cgroup_attach_proc, we indirectly call find_existing_css_set 3
times. It is an expensive call so we want to call it a minimum
of times. This patch only calls it once and stores the result so
that it can be used later on when we call cgroup_task_migrate.

This required modifying cgroup_task_migrate to take the new css_set
(which we obtained from find_css_set) as a parameter. The nice side
effect of this is that cgroup_task_migrate is now identical for
cgroup_attach_task and cgroup_attach_proc. It also now returns a
void since it can never fail.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: containers@lists.linux-foundation.org
Cc: cgroups@vger.kernel.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Paul Menage <paul@paulmenage.org>
---
 kernel/cgroup.c |  138 ++++++++++--------------------------------------------
 1 files changed, 26 insertions(+), 112 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4668a3f..2ac9eee 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1763,6 +1763,7 @@ EXPORT_SYMBOL_GPL(cgroup_path);
 struct task_and_cgroup {
 	struct task_struct	*task;
 	struct cgroup		*cgrp;
+	struct css_set		*cg;
 };
 
 struct cgroup_taskset {
@@ -1843,40 +1844,14 @@ EXPORT_SYMBOL_GPL(cgroup_taskset_size);
  * will already exist. If not set, this function might sleep, and can fail with
  * -ENOMEM. Must be called with cgroup_mutex and threadgroup locked.
  */
-static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
-			       struct task_struct *tsk, bool guarantee)
+static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
+				struct task_struct *tsk, struct css_set *newcg)
 {
 	struct css_set *oldcg;
-	struct css_set *newcg;
-
-	/*
-	 * get old css_set. we need to take task_lock and refcount it, because
-	 * an exiting task can change its css_set to init_css_set and drop its
-	 * old one without taking cgroup_mutex.
-	 */
-	task_lock(tsk);
-	oldcg = tsk->cgroups;
-	task_unlock(tsk);
-
-	/* locate or allocate a new css_set for this task. */
-	if (guarantee) {
-		/* we know the css_set we want already exists. */
-		struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
-		read_lock(&css_set_lock);
-		newcg = find_existing_css_set(oldcg, cgrp, template);
-		BUG_ON(!newcg);
-		get_css_set(newcg);
-		read_unlock(&css_set_lock);
-	} else {
-		might_sleep();
-		/* find_css_set will give us newcg already referenced. */
-		newcg = find_css_set(oldcg, cgrp);
-		if (!newcg)
-			return -ENOMEM;
-	}
 
 	/* @tsk can't exit as its threadgroup is locked */
 	task_lock(tsk);
+	oldcg = tsk->cgroups;
 	WARN_ON_ONCE(tsk->flags & PF_EXITING);
 	rcu_assign_pointer(tsk->cgroups, newcg);
 	task_unlock(tsk);
@@ -1895,7 +1870,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 	put_css_set(oldcg);
 
 	set_bit(CGRP_RELEASABLE, &oldcgrp->flags);
-	return 0;
 }
 
 /**
@@ -1913,6 +1887,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 	struct cgroup *oldcgrp;
 	struct cgroupfs_root *root = cgrp->root;
 	struct cgroup_taskset tset = { };
+	struct css_set *newcg, *oldcg;
 
 	/* @tsk either already exited or can't exit until the end */
 	if (tsk->flags & PF_EXITING)
@@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		}
 	}
 
-	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
-	if (retval)
+	task_lock(tsk);
+	oldcg = tsk->cgroups;
+	task_unlock(tsk);
+
+	newcg = find_css_set(oldcg, cgrp);
+	if (!newcg) {
+		retval = -ENOMEM;
 		goto out;
+	}
+
+	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
 
 	for_each_subsys(root, ss) {
 		if (ss->attach)
@@ -2000,65 +1983,6 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
 }
 EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
 
-/*
- * cgroup_attach_proc works in two stages, the first of which prefetches all
- * new css_sets needed (to make sure we have enough memory before committing
- * to the move) and stores them in a list of entries of the following type.
- * TODO: possible optimization: use css_set->rcu_head for chaining instead
- */
-struct cg_list_entry {
-	struct css_set *cg;
-	struct list_head links;
-};
-
-static bool css_set_fetched(struct cgroup *cgrp, struct task_struct *tsk,
-			    struct css_set *cg, struct list_head *newcg_list)
-{
-	struct css_set *newcg;
-	struct cg_list_entry *cg_entry;
-	struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
-
-	read_lock(&css_set_lock);
-	newcg = find_existing_css_set(cg, cgrp, template);
-	read_unlock(&css_set_lock);
-
-	/* doesn't exist at all? */
-	if (!newcg)
-		return false;
-	/* see if it's already in the list */
-	list_for_each_entry(cg_entry, newcg_list, links)
-		if (cg_entry->cg == newcg)
-			return true;
-
-	/* not found */
-	return false;
-}
-
-/*
- * Find the new css_set and store it in the list in preparation for moving the
- * given task to the given cgroup. Returns 0 or -ENOMEM.
- */
-static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
-			    struct list_head *newcg_list)
-{
-	struct css_set *newcg;
-	struct cg_list_entry *cg_entry;
-
-	/* ensure a new css_set will exist for this thread */
-	newcg = find_css_set(cg, cgrp);
-	if (!newcg)
-		return -ENOMEM;
-	/* add it to the list */
-	cg_entry = kmalloc(sizeof(struct cg_list_entry), GFP_KERNEL);
-	if (!cg_entry) {
-		put_css_set(newcg);
-		return -ENOMEM;
-	}
-	cg_entry->cg = newcg;
-	list_add(&cg_entry->links, newcg_list);
-	return 0;
-}
-
 /**
  * cgroup_attach_proc - attach all threads in a threadgroup to a cgroup
  * @cgrp: the cgroup to attach to
@@ -2069,7 +1993,7 @@ static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
  */
 int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
-	int retval, i, group_size;
+	int retval, i, group_size, css_set_refs = 0;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
@@ -2079,13 +2003,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	struct task_and_cgroup *tc;
 	struct flex_array *group;
 	struct cgroup_taskset tset = { };
-	/*
-	 * we need to make sure we have css_sets for all the tasks we're
-	 * going to move -before- we actually start moving them, so that in
-	 * case we get an ENOMEM we can bail out before making any changes.
-	 */
-	struct list_head newcg_list;
-	struct cg_list_entry *cg_entry, *temp_nobe;
 
 	/*
 	 * step 0: in order to do expensive, possibly blocking operations for
@@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 * step 2: make sure css_sets exist for all threads to be migrated.
 	 * we use find_css_set, which allocates a new one if necessary.
 	 */
-	INIT_LIST_HEAD(&newcg_list);
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
 		task_unlock(tc->task);
-		/* if we don't already have it in the list get a new one */
-		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
-			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
-				goto out_list_teardown;
+		tc->cg = find_css_set(oldcg, cgrp);
+		if (!tc->cg) {
+			retval = -ENOMEM;
+			css_set_refs = i + 1;
+			goto out_list_teardown;
+		}
 	}
 
 	/*
@@ -2191,8 +2109,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
-		BUG_ON(retval);
+		cgroup_task_migrate(cgrp, tc->cgrp, tc->task, tc->cg);
 	}
 	/* nothing is sensitive to fork() after this point. */
 
@@ -2211,14 +2128,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	cgroup_wakeup_rmdir_waiter(cgrp);
 	retval = 0;
 out_list_teardown:
-	/* clean up the list of prefetched css_sets. */
-	list_for_each_entry_safe(cg_entry, temp_nobe, &newcg_list, links) {
-		list_del(&cg_entry->links);
-		put_css_set(cg_entry->cg);
-		kfree(cg_entry);
+	for (i = 0; i < css_set_refs; i++) {
+		tc = flex_array_get(group, i);
+		put_css_set(tc->cg);
 	}
 out_cancel_attach:
-	/* same deal as in cgroup_attach_task */
 	if (retval) {
 		for_each_subsys(root, ss) {
 			if (ss == failed_ss)
-- 
1.7.3.1

[PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Mandeep Singh Baines]

In cgroup_attach_proc, we indirectly call find_existing_css_set 3
times. It is an expensive call so we want to call it a minimum
of times. This patch only calls it once and stores the result so
that it can be used later on when we call cgroup_task_migrate.

This required modifying cgroup_task_migrate to take the new css_set
(which we obtained from find_css_set) as a parameter. The nice side
effect of this is that cgroup_task_migrate is now identical for
cgroup_attach_task and cgroup_attach_proc. It also now returns a
void since it can never fail.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |  138 ++++++++++--------------------------------------------
 1 files changed, 26 insertions(+), 112 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4668a3f..2ac9eee 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1763,6 +1763,7 @@ EXPORT_SYMBOL_GPL(cgroup_path);
 struct task_and_cgroup {
 	struct task_struct	*task;
 	struct cgroup		*cgrp;
+	struct css_set		*cg;
 };
 
 struct cgroup_taskset {
@@ -1843,40 +1844,14 @@ EXPORT_SYMBOL_GPL(cgroup_taskset_size);
  * will already exist. If not set, this function might sleep, and can fail with
  * -ENOMEM. Must be called with cgroup_mutex and threadgroup locked.
  */
-static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
-			       struct task_struct *tsk, bool guarantee)
+static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
+				struct task_struct *tsk, struct css_set *newcg)
 {
 	struct css_set *oldcg;
-	struct css_set *newcg;
-
-	/*
-	 * get old css_set. we need to take task_lock and refcount it, because
-	 * an exiting task can change its css_set to init_css_set and drop its
-	 * old one without taking cgroup_mutex.
-	 */
-	task_lock(tsk);
-	oldcg = tsk->cgroups;
-	task_unlock(tsk);
-
-	/* locate or allocate a new css_set for this task. */
-	if (guarantee) {
-		/* we know the css_set we want already exists. */
-		struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
-		read_lock(&css_set_lock);
-		newcg = find_existing_css_set(oldcg, cgrp, template);
-		BUG_ON(!newcg);
-		get_css_set(newcg);
-		read_unlock(&css_set_lock);
-	} else {
-		might_sleep();
-		/* find_css_set will give us newcg already referenced. */
-		newcg = find_css_set(oldcg, cgrp);
-		if (!newcg)
-			return -ENOMEM;
-	}
 
 	/* @tsk can't exit as its threadgroup is locked */
 	task_lock(tsk);
+	oldcg = tsk->cgroups;
 	WARN_ON_ONCE(tsk->flags & PF_EXITING);
 	rcu_assign_pointer(tsk->cgroups, newcg);
 	task_unlock(tsk);
@@ -1895,7 +1870,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 	put_css_set(oldcg);
 
 	set_bit(CGRP_RELEASABLE, &oldcgrp->flags);
-	return 0;
 }
 
 /**
@@ -1913,6 +1887,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 	struct cgroup *oldcgrp;
 	struct cgroupfs_root *root = cgrp->root;
 	struct cgroup_taskset tset = { };
+	struct css_set *newcg, *oldcg;
 
 	/* @tsk either already exited or can't exit until the end */
 	if (tsk->flags & PF_EXITING)
@@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		}
 	}
 
-	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
-	if (retval)
+	task_lock(tsk);
+	oldcg = tsk->cgroups;
+	task_unlock(tsk);
+
+	newcg = find_css_set(oldcg, cgrp);
+	if (!newcg) {
+		retval = -ENOMEM;
 		goto out;
+	}
+
+	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
 
 	for_each_subsys(root, ss) {
 		if (ss->attach)
@@ -2000,65 +1983,6 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
 }
 EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
 
-/*
- * cgroup_attach_proc works in two stages, the first of which prefetches all
- * new css_sets needed (to make sure we have enough memory before committing
- * to the move) and stores them in a list of entries of the following type.
- * TODO: possible optimization: use css_set->rcu_head for chaining instead
- */
-struct cg_list_entry {
-	struct css_set *cg;
-	struct list_head links;
-};
-
-static bool css_set_fetched(struct cgroup *cgrp, struct task_struct *tsk,
-			    struct css_set *cg, struct list_head *newcg_list)
-{
-	struct css_set *newcg;
-	struct cg_list_entry *cg_entry;
-	struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
-
-	read_lock(&css_set_lock);
-	newcg = find_existing_css_set(cg, cgrp, template);
-	read_unlock(&css_set_lock);
-
-	/* doesn't exist at all? */
-	if (!newcg)
-		return false;
-	/* see if it's already in the list */
-	list_for_each_entry(cg_entry, newcg_list, links)
-		if (cg_entry->cg == newcg)
-			return true;
-
-	/* not found */
-	return false;
-}
-
-/*
- * Find the new css_set and store it in the list in preparation for moving the
- * given task to the given cgroup. Returns 0 or -ENOMEM.
- */
-static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
-			    struct list_head *newcg_list)
-{
-	struct css_set *newcg;
-	struct cg_list_entry *cg_entry;
-
-	/* ensure a new css_set will exist for this thread */
-	newcg = find_css_set(cg, cgrp);
-	if (!newcg)
-		return -ENOMEM;
-	/* add it to the list */
-	cg_entry = kmalloc(sizeof(struct cg_list_entry), GFP_KERNEL);
-	if (!cg_entry) {
-		put_css_set(newcg);
-		return -ENOMEM;
-	}
-	cg_entry->cg = newcg;
-	list_add(&cg_entry->links, newcg_list);
-	return 0;
-}
-
 /**
  * cgroup_attach_proc - attach all threads in a threadgroup to a cgroup
  * @cgrp: the cgroup to attach to
@@ -2069,7 +1993,7 @@ static int css_set_prefetch(struct cgroup *cgrp, struct css_set *cg,
  */
 int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
-	int retval, i, group_size;
+	int retval, i, group_size, css_set_refs = 0;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
@@ -2079,13 +2003,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	struct task_and_cgroup *tc;
 	struct flex_array *group;
 	struct cgroup_taskset tset = { };
-	/*
-	 * we need to make sure we have css_sets for all the tasks we're
-	 * going to move -before- we actually start moving them, so that in
-	 * case we get an ENOMEM we can bail out before making any changes.
-	 */
-	struct list_head newcg_list;
-	struct cg_list_entry *cg_entry, *temp_nobe;
 
 	/*
 	 * step 0: in order to do expensive, possibly blocking operations for
@@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 * step 2: make sure css_sets exist for all threads to be migrated.
 	 * we use find_css_set, which allocates a new one if necessary.
 	 */
-	INIT_LIST_HEAD(&newcg_list);
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
 		/* get old css_set pointer */
 		task_lock(tc->task);
 		oldcg = tc->task->cgroups;
 		task_unlock(tc->task);
-		/* if we don't already have it in the list get a new one */
-		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
-			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
-				goto out_list_teardown;
+		tc->cg = find_css_set(oldcg, cgrp);
+		if (!tc->cg) {
+			retval = -ENOMEM;
+			css_set_refs = i + 1;
+			goto out_list_teardown;
+		}
 	}
 
 	/*
@@ -2191,8 +2109,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
-		BUG_ON(retval);
+		cgroup_task_migrate(cgrp, tc->cgrp, tc->task, tc->cg);
 	}
 	/* nothing is sensitive to fork() after this point. */
 
@@ -2211,14 +2128,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	cgroup_wakeup_rmdir_waiter(cgrp);
 	retval = 0;
 out_list_teardown:
-	/* clean up the list of prefetched css_sets. */
-	list_for_each_entry_safe(cg_entry, temp_nobe, &newcg_list, links) {
-		list_del(&cg_entry->links);
-		put_css_set(cg_entry->cg);
-		kfree(cg_entry);
+	for (i = 0; i < css_set_refs; i++) {
+		tc = flex_array_get(group, i);
+		put_css_set(tc->cg);
 	}
 out_cancel_attach:
-	/* same deal as in cgroup_attach_task */
 	if (retval) {
 		for_each_subsys(root, ss) {
 			if (ss == failed_ss)
-- 
1.7.3.1

[PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Mandeep Singh Baines]

Makes it easier to read the non-error path.

While at it, move the retval=0 assignment inside the !group_size
check. This way the comment describe this check is directly
above it(we also avoid an extra assignment not needed in the
normal path). Also return 0 directly instead of using retval.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2ac9eee..9ce0872 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	read_unlock(&tasklist_lock);
 
 	/* methods shouldn't be called if no task is actually migrating */
-	retval = 0;
-	if (!group_size)
+	if (!group_size) {
+		retval = 0;
 		goto out_free_group_list;
+	}
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
@@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	synchronize_rcu();
 	cgroup_wakeup_rmdir_waiter(cgrp);
-	retval = 0;
+	flex_array_free(group);
+	return 0;
+
 out_list_teardown:
 	for (i = 0; i < css_set_refs; i++) {
 		tc = flex_array_get(group, i);
 		put_css_set(tc->cg);
 	}
 out_cancel_attach:
-	if (retval) {
-		for_each_subsys(root, ss) {
-			if (ss == failed_ss)
-				break;
-			if (ss->cancel_attach)
-				ss->cancel_attach(ss, cgrp, &tset);
-		}
+	for_each_subsys(root, ss) {
+		if (ss == failed_ss)
+			break;
+		if (ss->cancel_attach)
+			ss->cancel_attach(ss, cgrp, &tset);
 	}
 out_free_group_list:
 	flex_array_free(group);
-- 
1.7.3.1

[PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Mandeep Singh Baines]

Makes it easier to read the non-error path.

While at it, move the retval=0 assignment inside the !group_size
check. This way the comment describe this check is directly
above it(we also avoid an extra assignment not needed in the
normal path). Also return 0 directly instead of using retval.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: containers@lists.linux-foundation.org
Cc: cgroups@vger.kernel.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Paul Menage <paul@paulmenage.org>
---
 kernel/cgroup.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2ac9eee..9ce0872 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	read_unlock(&tasklist_lock);
 
 	/* methods shouldn't be called if no task is actually migrating */
-	retval = 0;
-	if (!group_size)
+	if (!group_size) {
+		retval = 0;
 		goto out_free_group_list;
+	}
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
@@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	synchronize_rcu();
 	cgroup_wakeup_rmdir_waiter(cgrp);
-	retval = 0;
+	flex_array_free(group);
+	return 0;
+
 out_list_teardown:
 	for (i = 0; i < css_set_refs; i++) {
 		tc = flex_array_get(group, i);
 		put_css_set(tc->cg);
 	}
 out_cancel_attach:
-	if (retval) {
-		for_each_subsys(root, ss) {
-			if (ss == failed_ss)
-				break;
-			if (ss->cancel_attach)
-				ss->cancel_attach(ss, cgrp, &tset);
-		}
+	for_each_subsys(root, ss) {
+		if (ss == failed_ss)
+			break;
+		if (ss->cancel_attach)
+			ss->cancel_attach(ss, cgrp, &tset);
 	}
 out_free_group_list:
 	flex_array_free(group);
-- 
1.7.3.1

[PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Mandeep Singh Baines]

Makes it easier to read the non-error path.

While at it, move the retval=0 assignment inside the !group_size
check. This way the comment describe this check is directly
above it(we also avoid an extra assignment not needed in the
normal path). Also return 0 directly instead of using retval.

Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
---
 kernel/cgroup.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2ac9eee..9ce0872 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	read_unlock(&tasklist_lock);
 
 	/* methods shouldn't be called if no task is actually migrating */
-	retval = 0;
-	if (!group_size)
+	if (!group_size) {
+		retval = 0;
 		goto out_free_group_list;
+	}
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
@@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 */
 	synchronize_rcu();
 	cgroup_wakeup_rmdir_waiter(cgrp);
-	retval = 0;
+	flex_array_free(group);
+	return 0;
+
 out_list_teardown:
 	for (i = 0; i < css_set_refs; i++) {
 		tc = flex_array_get(group, i);
 		put_css_set(tc->cg);
 	}
 out_cancel_attach:
-	if (retval) {
-		for_each_subsys(root, ss) {
-			if (ss == failed_ss)
-				break;
-			if (ss->cancel_attach)
-				ss->cancel_attach(ss, cgrp, &tset);
-		}
+	for_each_subsys(root, ss) {
+		if (ss == failed_ss)
+			break;
+		if (ss->cancel_attach)
+			ss->cancel_attach(ss, cgrp, &tset);
 	}
 out_free_group_list:
 	flex_array_free(group);
-- 
1.7.3.1

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

Hello,

s/redundate/redundant/ in $SUBJ, right?

On Tue, Dec 20, 2011 at 03:14:29PM -0800, Mandeep Singh Baines wrote:
> Changes in V3:
> * Fixed to put error in retval (thanks Frederic Weisbecker)
>   * https://lkml.org/lkml/2011/12/20/255
> Changes in V2:
> * Updated commit message as per Tejun's feedback:
>   * https://lkml.org/lkml/2011/12/19/289
> 
> -- >8 -- (snip)

Either put this as part of patch description or if you don't want it
to be part of commit message, put it between --- and diffstat.
Unfortunately, no tool understands "-- >8 --".

> We can now assume that the css_set reference held by the task
> will not go away for an exiting task. PF_EXITING state can be
> trusted throughout migration by checking it after locking
> threadgroup.
> 
> While at it, renamed css_set_check_fetched to css_set_fetched.
> !css_set_fetched() seems to read better than
> !css_set_check_fetched().

I don't feel too hot about the renaming.  Can you please drop it for
now?

> Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>

Other than that, looks good to me.  I'll commit this once Li acks it.

Thanks.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

Hello,

s/redundate/redundant/ in $SUBJ, right?

On Tue, Dec 20, 2011 at 03:14:29PM -0800, Mandeep Singh Baines wrote:
> Changes in V3:
> * Fixed to put error in retval (thanks Frederic Weisbecker)
>   * https://lkml.org/lkml/2011/12/20/255
> Changes in V2:
> * Updated commit message as per Tejun's feedback:
>   * https://lkml.org/lkml/2011/12/19/289
> 
> -- >8 -- (snip)

Either put this as part of patch description or if you don't want it
to be part of commit message, put it between --- and diffstat.
Unfortunately, no tool understands "-- >8 --".

> We can now assume that the css_set reference held by the task
> will not go away for an exiting task. PF_EXITING state can be
> trusted throughout migration by checking it after locking
> threadgroup.
> 
> While at it, renamed css_set_check_fetched to css_set_fetched.
> !css_set_fetched() seems to read better than
> !css_set_check_fetched().

I don't feel too hot about the renaming.  Can you please drop it for
now?

> Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: containers@lists.linux-foundation.org
> Cc: cgroups@vger.kernel.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Paul Menage <paul@paulmenage.org>

Other than that, looks good to me.  I'll commit this once Li acks it.

Thanks.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

Hello,

s/redundate/redundant/ in $SUBJ, right?

On Tue, Dec 20, 2011 at 03:14:29PM -0800, Mandeep Singh Baines wrote:
> Changes in V3:
> * Fixed to put error in retval (thanks Frederic Weisbecker)
>   * https://lkml.org/lkml/2011/12/20/255
> Changes in V2:
> * Updated commit message as per Tejun's feedback:
>   * https://lkml.org/lkml/2011/12/19/289
> 
> -- >8 -- (snip)

Either put this as part of patch description or if you don't want it
to be part of commit message, put it between --- and diffstat.
Unfortunately, no tool understands "-- >8 --".

> We can now assume that the css_set reference held by the task
> will not go away for an exiting task. PF_EXITING state can be
> trusted throughout migration by checking it after locking
> threadgroup.
> 
> While at it, renamed css_set_check_fetched to css_set_fetched.
> !css_set_fetched() seems to read better than
> !css_set_check_fetched().

I don't feel too hot about the renaming.  Can you please drop it for
now?

> Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>

Other than that, looks good to me.  I'll commit this once Li acks it.

Thanks.

-- 
tejun

Re: [PATCH 2/5] cgroup: remove redundant get/put of task struct

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:30PM -0800, Mandeep Singh Baines wrote:
> @@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
>  			cgroup_unlock();
>  			return -EACCES;
>  		}
> -		get_task_struct(tsk);
>  		rcu_read_unlock();

This is before threadgroup is locked.  What guarantees @tsk doesn't go
away once rcu is unlocked?

Thanks.

-- 
tejun

Re: [PATCH 2/5] cgroup: remove redundant get/put of task struct

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:30PM -0800, Mandeep Singh Baines wrote:
> @@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
>  			cgroup_unlock();
>  			return -EACCES;
>  		}
> -		get_task_struct(tsk);
>  		rcu_read_unlock();

This is before threadgroup is locked.  What guarantees @tsk doesn't go
away once rcu is unlocked?

Thanks.

-- 
tejun

Re: [PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:32PM -0800, Mandeep Singh Baines wrote:
> @@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  		}
>  	}
>  
> -	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
> -	if (retval)
> +	task_lock(tsk);
> +	oldcg = tsk->cgroups;
> +	task_unlock(tsk);

Probably this is patch order issue w/ Frederic's patch but we don't
need task_lock here, right?

> @@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	 * step 2: make sure css_sets exist for all threads to be migrated.
>  	 * we use find_css_set, which allocates a new one if necessary.
>  	 */
> -	INIT_LIST_HEAD(&newcg_list);
>  	for (i = 0; i < group_size; i++) {
>  		tc = flex_array_get(group, i);
>  		/* get old css_set pointer */
>  		task_lock(tc->task);
>  		oldcg = tc->task->cgroups;
>  		task_unlock(tc->task);
> -		/* if we don't already have it in the list get a new one */
> -		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
> -			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
> -				goto out_list_teardown;
> +		tc->cg = find_css_set(oldcg, cgrp);
> +		if (!tc->cg) {
> +			retval = -ENOMEM;
> +			css_set_refs = i + 1;
> +			goto out_list_teardown;
> +		}

Nice cleanup, but can't the above be merged into the loop which builds
the flex array?

Thanks.

-- 
tejun

Re: [PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:32PM -0800, Mandeep Singh Baines wrote:
> @@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  		}
>  	}
>  
> -	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
> -	if (retval)
> +	task_lock(tsk);
> +	oldcg = tsk->cgroups;
> +	task_unlock(tsk);

Probably this is patch order issue w/ Frederic's patch but we don't
need task_lock here, right?

> @@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	 * step 2: make sure css_sets exist for all threads to be migrated.
>  	 * we use find_css_set, which allocates a new one if necessary.
>  	 */
> -	INIT_LIST_HEAD(&newcg_list);
>  	for (i = 0; i < group_size; i++) {
>  		tc = flex_array_get(group, i);
>  		/* get old css_set pointer */
>  		task_lock(tc->task);
>  		oldcg = tc->task->cgroups;
>  		task_unlock(tc->task);
> -		/* if we don't already have it in the list get a new one */
> -		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
> -			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
> -				goto out_list_teardown;
> +		tc->cg = find_css_set(oldcg, cgrp);
> +		if (!tc->cg) {
> +			retval = -ENOMEM;
> +			css_set_refs = i + 1;
> +			goto out_list_teardown;
> +		}

Nice cleanup, but can't the above be merged into the loop which builds
the flex array?

Thanks.

-- 
tejun

Re: [PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:32PM -0800, Mandeep Singh Baines wrote:
> @@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  		}
>  	}
>  
> -	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
> -	if (retval)
> +	task_lock(tsk);
> +	oldcg = tsk->cgroups;
> +	task_unlock(tsk);

Probably this is patch order issue w/ Frederic's patch but we don't
need task_lock here, right?

> @@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	 * step 2: make sure css_sets exist for all threads to be migrated.
>  	 * we use find_css_set, which allocates a new one if necessary.
>  	 */
> -	INIT_LIST_HEAD(&newcg_list);
>  	for (i = 0; i < group_size; i++) {
>  		tc = flex_array_get(group, i);
>  		/* get old css_set pointer */
>  		task_lock(tc->task);
>  		oldcg = tc->task->cgroups;
>  		task_unlock(tc->task);
> -		/* if we don't already have it in the list get a new one */
> -		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
> -			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
> -				goto out_list_teardown;
> +		tc->cg = find_css_set(oldcg, cgrp);
> +		if (!tc->cg) {
> +			retval = -ENOMEM;
> +			css_set_refs = i + 1;
> +			goto out_list_teardown;
> +		}

Nice cleanup, but can't the above be merged into the loop which builds
the flex array?

Thanks.

-- 
tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:33PM -0800, Mandeep Singh Baines wrote:
> @@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	read_unlock(&tasklist_lock);
>  
>  	/* methods shouldn't be called if no task is actually migrating */
> -	retval = 0;
> -	if (!group_size)
> +	if (!group_size) {
> +		retval = 0;
>  		goto out_free_group_list;
> +	}

Eh... I don't think this is an improvement.  It's just different.

> @@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	 */
>  	synchronize_rcu();
>  	cgroup_wakeup_rmdir_waiter(cgrp);
> -	retval = 0;
> +	flex_array_free(group);
> +	return 0;

Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
and failure paths can lead future updaters forget one of them.  The
exit path in this function isn't pretty but I don't think the proposed
patch improves it either.

Thanks.

-- 
tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:33PM -0800, Mandeep Singh Baines wrote:
> @@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	read_unlock(&tasklist_lock);
>  
>  	/* methods shouldn't be called if no task is actually migrating */
> -	retval = 0;
> -	if (!group_size)
> +	if (!group_size) {
> +		retval = 0;
>  		goto out_free_group_list;
> +	}

Eh... I don't think this is an improvement.  It's just different.

> @@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	 */
>  	synchronize_rcu();
>  	cgroup_wakeup_rmdir_waiter(cgrp);
> -	retval = 0;
> +	flex_array_free(group);
> +	return 0;

Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
and failure paths can lead future updaters forget one of them.  The
exit path in this function isn't pretty but I don't think the proposed
patch improves it either.

Thanks.

-- 
tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:14:33PM -0800, Mandeep Singh Baines wrote:
> @@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	read_unlock(&tasklist_lock);
>  
>  	/* methods shouldn't be called if no task is actually migrating */
> -	retval = 0;
> -	if (!group_size)
> +	if (!group_size) {
> +		retval = 0;
>  		goto out_free_group_list;
> +	}

Eh... I don't think this is an improvement.  It's just different.

> @@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	 */
>  	synchronize_rcu();
>  	cgroup_wakeup_rmdir_waiter(cgrp);
> -	retval = 0;
> +	flex_array_free(group);
> +	return 0;

Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
and failure paths can lead future updaters forget one of them.  The
exit path in this function isn't pretty but I don't think the proposed
patch improves it either.

Thanks.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> s/redundate/redundant/ in $SUBJ, right?
> 
> On Tue, Dec 20, 2011 at 03:14:29PM -0800, Mandeep Singh Baines wrote:
> > Changes in V3:
> > * Fixed to put error in retval (thanks Frederic Weisbecker)
> >   * https://lkml.org/lkml/2011/12/20/255
> > Changes in V2:
> > * Updated commit message as per Tejun's feedback:
> >   * https://lkml.org/lkml/2011/12/19/289
> > 
> > -- >8 -- (snip)
> 
> Either put this as part of patch description or if you don't want it
> to be part of commit message, put it between --- and diffstat.
> Unfortunately, no tool understands "-- >8 --".
> 

git-am should. From "man git-am":

       -c, --scissors
           Remove everything in body before a scissors line (see git-
           mailinfo(1)).

From "man git-mailinfo":

       --scissors
           Remove everything in body before a scissors line. A line that
           mainly consists of scissors (either ">8" or "8<") and perforation
           (dash "-") marks is called a scissors line, and is used to request
           the reader to cut the message at that line. If such a line appears
           in the body of the message before the patch, everything before it
           (including the scissors line itself) is ignored when this option is
           used.

           This is useful if you want to begin your message in a discussion
           thread with comments and suggestions on the message you are
           responding to, and to conclude it with a patch submission,
           separating the discussion and the beginning of the proposed commit
           log message with a scissors line.

           This can enabled by default with the configuration option
           mailinfo.scissors.

But I can stop using scissors if that is what you prefer.

> > We can now assume that the css_set reference held by the task
> > will not go away for an exiting task. PF_EXITING state can be
> > trusted throughout migration by checking it after locking
> > threadgroup.
> > 
> > While at it, renamed css_set_check_fetched to css_set_fetched.
> > !css_set_fetched() seems to read better than
> > !css_set_check_fetched().
> 
> I don't feel too hot about the renaming.  Can you please drop it for
> now?
> 

Sure. No problem.

> > Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> > Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> 
> Other than that, looks good to me.  I'll commit this once Li acks it.
> 
> Thanks.
> 
> -- 
> tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Mandeep Singh Baines]

Tejun Heo (tj@kernel.org) wrote:
> Hello,
> 
> s/redundate/redundant/ in $SUBJ, right?
> 
> On Tue, Dec 20, 2011 at 03:14:29PM -0800, Mandeep Singh Baines wrote:
> > Changes in V3:
> > * Fixed to put error in retval (thanks Frederic Weisbecker)
> >   * https://lkml.org/lkml/2011/12/20/255
> > Changes in V2:
> > * Updated commit message as per Tejun's feedback:
> >   * https://lkml.org/lkml/2011/12/19/289
> > 
> > -- >8 -- (snip)
> 
> Either put this as part of patch description or if you don't want it
> to be part of commit message, put it between --- and diffstat.
> Unfortunately, no tool understands "-- >8 --".
> 

git-am should. From "man git-am":

       -c, --scissors
           Remove everything in body before a scissors line (see git-
           mailinfo(1)).

>From "man git-mailinfo":

       --scissors
           Remove everything in body before a scissors line. A line that
           mainly consists of scissors (either ">8" or "8<") and perforation
           (dash "-") marks is called a scissors line, and is used to request
           the reader to cut the message at that line. If such a line appears
           in the body of the message before the patch, everything before it
           (including the scissors line itself) is ignored when this option is
           used.

           This is useful if you want to begin your message in a discussion
           thread with comments and suggestions on the message you are
           responding to, and to conclude it with a patch submission,
           separating the discussion and the beginning of the proposed commit
           log message with a scissors line.

           This can enabled by default with the configuration option
           mailinfo.scissors.

But I can stop using scissors if that is what you prefer.

> > We can now assume that the css_set reference held by the task
> > will not go away for an exiting task. PF_EXITING state can be
> > trusted throughout migration by checking it after locking
> > threadgroup.
> > 
> > While at it, renamed css_set_check_fetched to css_set_fetched.
> > !css_set_fetched() seems to read better than
> > !css_set_check_fetched().
> 
> I don't feel too hot about the renaming.  Can you please drop it for
> now?
> 

Sure. No problem.

> > Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Li Zefan <lizf@cn.fujitsu.com>
> > Cc: containers@lists.linux-foundation.org
> > Cc: cgroups@vger.kernel.org
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Paul Menage <paul@paulmenage.org>
> 
> Other than that, looks good to me.  I'll commit this once Li acks it.
> 
> Thanks.
> 
> -- 
> tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:41:31PM -0800, Mandeep Singh Baines wrote:
> > Either put this as part of patch description or if you don't want it
> > to be part of commit message, put it between --- and diffstat.
> > Unfortunately, no tool understands "-- >8 --".
> > 
> 
> git-am should. From "man git-am":
> 
>        -c, --scissors
>            Remove everything in body before a scissors line (see git-
>            mailinfo(1)).
> 
> From "man git-mailinfo":
> 
>        --scissors
>            Remove everything in body before a scissors line. A line that
>            mainly consists of scissors (either ">8" or "8<") and perforation
>            (dash "-") marks is called a scissors line, and is used to request
>            the reader to cut the message at that line. If such a line appears
>            in the body of the message before the patch, everything before it
>            (including the scissors line itself) is ignored when this option is
>            used.
> 
>            This is useful if you want to begin your message in a discussion
>            thread with comments and suggestions on the message you are
>            responding to, and to conclude it with a patch submission,
>            separating the discussion and the beginning of the proposed commit
>            log message with a scissors line.
> 
>            This can enabled by default with the configuration option
>            mailinfo.scissors.
> 
> But I can stop using scissors if that is what you prefer.

Heh, that's cute.  If git-am is happy, I'm happy too.  Thanks for
letting me know.

Thanks.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

Hello,

On Tue, Dec 20, 2011 at 03:41:31PM -0800, Mandeep Singh Baines wrote:
> > Either put this as part of patch description or if you don't want it
> > to be part of commit message, put it between --- and diffstat.
> > Unfortunately, no tool understands "-- >8 --".
> > 
> 
> git-am should. From "man git-am":
> 
>        -c, --scissors
>            Remove everything in body before a scissors line (see git-
>            mailinfo(1)).
> 
> From "man git-mailinfo":
> 
>        --scissors
>            Remove everything in body before a scissors line. A line that
>            mainly consists of scissors (either ">8" or "8<") and perforation
>            (dash "-") marks is called a scissors line, and is used to request
>            the reader to cut the message at that line. If such a line appears
>            in the body of the message before the patch, everything before it
>            (including the scissors line itself) is ignored when this option is
>            used.
> 
>            This is useful if you want to begin your message in a discussion
>            thread with comments and suggestions on the message you are
>            responding to, and to conclude it with a patch submission,
>            separating the discussion and the beginning of the proposed commit
>            log message with a scissors line.
> 
>            This can enabled by default with the configuration option
>            mailinfo.scissors.
> 
> But I can stop using scissors if that is what you prefer.

Heh, that's cute.  If git-am is happy, I'm happy too.  Thanks for
letting me know.

Thanks.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

On Tue, Dec 20, 2011 at 03:42:59PM -0800, Tejun Heo wrote:
> > But I can stop using scissors if that is what you prefer.
> 
> Heh, that's cute.  If git-am is happy, I'm happy too.  Thanks for
> letting me know.

Hmm... on the second thought, it's not enabled by default and I
suspect git-quiltimport would cut it out.  Let's stick to below --- or
in the commit message.

Thank you.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

On Tue, Dec 20, 2011 at 03:42:59PM -0800, Tejun Heo wrote:
> > But I can stop using scissors if that is what you prefer.
> 
> Heh, that's cute.  If git-am is happy, I'm happy too.  Thanks for
> letting me know.

Hmm... on the second thought, it's not enabled by default and I
suspect git-quiltimport would cut it out.  Let's stick to below --- or
in the commit message.

Thank you.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Tejun Heo]

On Tue, Dec 20, 2011 at 03:42:59PM -0800, Tejun Heo wrote:
> > But I can stop using scissors if that is what you prefer.
> 
> Heh, that's cute.  If git-am is happy, I'm happy too.  Thanks for
> letting me know.

Hmm... on the second thought, it's not enabled by default and I
suspect git-quiltimport would cut it out.  Let's stick to below --- or
in the commit message.

Thank you.

-- 
tejun

Re: [PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:32PM -0800, Mandeep Singh Baines wrote:
> > @@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> >  		}
> >  	}
> >  
> > -	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
> > -	if (retval)
> > +	task_lock(tsk);
> > +	oldcg = tsk->cgroups;
> > +	task_unlock(tsk);
> 
> Probably this is patch order issue w/ Frederic's patch but we don't
> need task_lock here, right?
> 

Yeah, avoided removing the locks because Frederic's patch already did that.
Once his changes get in, I'll rebase and resend.

> > @@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	 * step 2: make sure css_sets exist for all threads to be migrated.
> >  	 * we use find_css_set, which allocates a new one if necessary.
> >  	 */
> > -	INIT_LIST_HEAD(&newcg_list);
> >  	for (i = 0; i < group_size; i++) {
> >  		tc = flex_array_get(group, i);
> >  		/* get old css_set pointer */
> >  		task_lock(tc->task);
> >  		oldcg = tc->task->cgroups;
> >  		task_unlock(tc->task);
> > -		/* if we don't already have it in the list get a new one */
> > -		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
> > -			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
> > -				goto out_list_teardown;
> > +		tc->cg = find_css_set(oldcg, cgrp);
> > +		if (!tc->cg) {
> > +			retval = -ENOMEM;
> > +			css_set_refs = i + 1;
> > +			goto out_list_teardown;
> > +		}
> 
> Nice cleanup, but can't the above be merged into the loop which builds
> the flex array?
> 

Good idea. That would remove a for loop. Will fix in next version.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Mandeep Singh Baines]

Tejun Heo (tj@kernel.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:32PM -0800, Mandeep Singh Baines wrote:
> > @@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> >  		}
> >  	}
> >  
> > -	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
> > -	if (retval)
> > +	task_lock(tsk);
> > +	oldcg = tsk->cgroups;
> > +	task_unlock(tsk);
> 
> Probably this is patch order issue w/ Frederic's patch but we don't
> need task_lock here, right?
> 

Yeah, avoided removing the locks because Frederic's patch already did that.
Once his changes get in, I'll rebase and resend.

> > @@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	 * step 2: make sure css_sets exist for all threads to be migrated.
> >  	 * we use find_css_set, which allocates a new one if necessary.
> >  	 */
> > -	INIT_LIST_HEAD(&newcg_list);
> >  	for (i = 0; i < group_size; i++) {
> >  		tc = flex_array_get(group, i);
> >  		/* get old css_set pointer */
> >  		task_lock(tc->task);
> >  		oldcg = tc->task->cgroups;
> >  		task_unlock(tc->task);
> > -		/* if we don't already have it in the list get a new one */
> > -		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
> > -			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
> > -				goto out_list_teardown;
> > +		tc->cg = find_css_set(oldcg, cgrp);
> > +		if (!tc->cg) {
> > +			retval = -ENOMEM;
> > +			css_set_refs = i + 1;
> > +			goto out_list_teardown;
> > +		}
> 
> Nice cleanup, but can't the above be merged into the loop which builds
> the flex array?
> 

Good idea. That would remove a for loop. Will fix in next version.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 4/5] cgroup: remove extra calls to find_existing_css_set

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:32PM -0800, Mandeep Singh Baines wrote:
> > @@ -1942,9 +1917,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> >  		}
> >  	}
> >  
> > -	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
> > -	if (retval)
> > +	task_lock(tsk);
> > +	oldcg = tsk->cgroups;
> > +	task_unlock(tsk);
> 
> Probably this is patch order issue w/ Frederic's patch but we don't
> need task_lock here, right?
> 

Yeah, avoided removing the locks because Frederic's patch already did that.
Once his changes get in, I'll rebase and resend.

> > @@ -2171,17 +2088,18 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	 * step 2: make sure css_sets exist for all threads to be migrated.
> >  	 * we use find_css_set, which allocates a new one if necessary.
> >  	 */
> > -	INIT_LIST_HEAD(&newcg_list);
> >  	for (i = 0; i < group_size; i++) {
> >  		tc = flex_array_get(group, i);
> >  		/* get old css_set pointer */
> >  		task_lock(tc->task);
> >  		oldcg = tc->task->cgroups;
> >  		task_unlock(tc->task);
> > -		/* if we don't already have it in the list get a new one */
> > -		if (!css_set_fetched(cgrp, tc->task, oldcg, &newcg_list))
> > -			if (retval = css_set_prefetch(cgrp, oldcg, &newcg_list))
> > -				goto out_list_teardown;
> > +		tc->cg = find_css_set(oldcg, cgrp);
> > +		if (!tc->cg) {
> > +			retval = -ENOMEM;
> > +			css_set_refs = i + 1;
> > +			goto out_list_teardown;
> > +		}
> 
> Nice cleanup, but can't the above be merged into the loop which builds
> the flex array?
> 

Good idea. That would remove a for loop. Will fix in next version.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:33PM -0800, Mandeep Singh Baines wrote:
> > @@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	read_unlock(&tasklist_lock);
> >  
> >  	/* methods shouldn't be called if no task is actually migrating */
> > -	retval = 0;
> > -	if (!group_size)
> > +	if (!group_size) {
> > +		retval = 0;
> >  		goto out_free_group_list;
> > +	}
> 
> Eh... I don't think this is an improvement.  It's just different.
> 

The main benefit is that the comment is directly above the code its
describing but I can drop this part of the change.

> > @@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	 */
> >  	synchronize_rcu();
> >  	cgroup_wakeup_rmdir_waiter(cgrp);
> > -	retval = 0;
> > +	flex_array_free(group);
> > +	return 0;
> 
> Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
> and failure paths can lead future updaters forget one of them.  The
> exit path in this function isn't pretty but I don't think the proposed
> patch improves it either.
> 

Should I drop the patch or add the goto? Its 5/5 so easy enough to drop
since nothing else depends on it.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Mandeep Singh Baines]

Tejun Heo (tj@kernel.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:33PM -0800, Mandeep Singh Baines wrote:
> > @@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	read_unlock(&tasklist_lock);
> >  
> >  	/* methods shouldn't be called if no task is actually migrating */
> > -	retval = 0;
> > -	if (!group_size)
> > +	if (!group_size) {
> > +		retval = 0;
> >  		goto out_free_group_list;
> > +	}
> 
> Eh... I don't think this is an improvement.  It's just different.
> 

The main benefit is that the comment is directly above the code its
describing but I can drop this part of the change.

> > @@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	 */
> >  	synchronize_rcu();
> >  	cgroup_wakeup_rmdir_waiter(cgrp);
> > -	retval = 0;
> > +	flex_array_free(group);
> > +	return 0;
> 
> Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
> and failure paths can lead future updaters forget one of them.  The
> exit path in this function isn't pretty but I don't think the proposed
> patch improves it either.
> 

Should I drop the patch or add the goto? Its 5/5 so easy enough to drop
since nothing else depends on it.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:33PM -0800, Mandeep Singh Baines wrote:
> > @@ -2067,9 +2067,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	read_unlock(&tasklist_lock);
> >  
> >  	/* methods shouldn't be called if no task is actually migrating */
> > -	retval = 0;
> > -	if (!group_size)
> > +	if (!group_size) {
> > +		retval = 0;
> >  		goto out_free_group_list;
> > +	}
> 
> Eh... I don't think this is an improvement.  It's just different.
> 

The main benefit is that the comment is directly above the code its
describing but I can drop this part of the change.

> > @@ -2126,20 +2127,20 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> >  	 */
> >  	synchronize_rcu();
> >  	cgroup_wakeup_rmdir_waiter(cgrp);
> > -	retval = 0;
> > +	flex_array_free(group);
> > +	return 0;
> 
> Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
> and failure paths can lead future updaters forget one of them.  The
> exit path in this function isn't pretty but I don't think the proposed
> patch improves it either.
> 

Should I drop the patch or add the goto? Its 5/5 so easy enough to drop
since nothing else depends on it.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 2/5] cgroup: remove redundant get/put of task struct

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:30PM -0800, Mandeep Singh Baines wrote:
> > @@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
> >  			cgroup_unlock();
> >  			return -EACCES;
> >  		}
> > -		get_task_struct(tsk);
> >  		rcu_read_unlock();
> 
> This is before threadgroup is locked.  What guarantees @tsk doesn't go
> away once rcu is unlocked?
> 

Nothing. D'oh. I'll remove the changes to attach_task_by_id and leave
the others.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 2/5] cgroup: remove redundant get/put of task struct

[Mandeep Singh Baines]

Tejun Heo (tj@kernel.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:30PM -0800, Mandeep Singh Baines wrote:
> > @@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
> >  			cgroup_unlock();
> >  			return -EACCES;
> >  		}
> > -		get_task_struct(tsk);
> >  		rcu_read_unlock();
> 
> This is before threadgroup is locked.  What guarantees @tsk doesn't go
> away once rcu is unlocked?
> 

Nothing. D'oh. I'll remove the changes to attach_task_by_id and leave
the others.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 2/5] cgroup: remove redundant get/put of task struct

[Mandeep Singh Baines]

Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> Hello,
> 
> On Tue, Dec 20, 2011 at 03:14:30PM -0800, Mandeep Singh Baines wrote:
> > @@ -2287,14 +2280,12 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
> >  			cgroup_unlock();
> >  			return -EACCES;
> >  		}
> > -		get_task_struct(tsk);
> >  		rcu_read_unlock();
> 
> This is before threadgroup is locked.  What guarantees @tsk doesn't go
> away once rcu is unlocked?
> 

Nothing. D'oh. I'll remove the changes to attach_task_by_id and leave
the others.

> Thanks.
> 
> -- 
> tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Tejun Heo]

On Tue, Dec 20, 2011 at 04:02:31PM -0800, Mandeep Singh Baines wrote:
> > Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
> > and failure paths can lead future updaters forget one of them.  The
> > exit path in this function isn't pretty but I don't think the proposed
> > patch improves it either.
> > 
> 
> Should I drop the patch or add the goto? Its 5/5 so easy enough to drop
> since nothing else depends on it.

Yeah, let's drop it for now.  Thanks.

-- 
tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Tejun Heo]

On Tue, Dec 20, 2011 at 04:02:31PM -0800, Mandeep Singh Baines wrote:
> > Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
> > and failure paths can lead future updaters forget one of them.  The
> > exit path in this function isn't pretty but I don't think the proposed
> > patch improves it either.
> > 
> 
> Should I drop the patch or add the goto? Its 5/5 so easy enough to drop
> since nothing else depends on it.

Yeah, let's drop it for now.  Thanks.

-- 
tejun

Re: [PATCH 5/5] cgroup: separate out cgroup_attach_proc error handling code

[Tejun Heo]

On Tue, Dec 20, 2011 at 04:02:31PM -0800, Mandeep Singh Baines wrote:
> > Hmm... maybe goto out_free_group_list?  Duplicating cleanup on success
> > and failure paths can lead future updaters forget one of them.  The
> > exit path in this function isn't pretty but I don't think the proposed
> > patch improves it either.
> > 
> 
> Should I drop the patch or add the goto? Its 5/5 so easy enough to drop
> since nothing else depends on it.

Yeah, let's drop it for now.  Thanks.

-- 
tejun

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Li Zefan]

Mandeep Singh Baines wrote:
> Changes in V3:
> * Fixed to put error in retval (thanks Frederic Weisbecker)
>   * https://lkml.org/lkml/2011/12/20/255
> Changes in V2:
> * Updated commit message as per Tejun's feedback:
>   * https://lkml.org/lkml/2011/12/19/289
> 
> -- >8 -- (snip)
> 
> We can now assume that the css_set reference held by the task
> will not go away for an exiting task. PF_EXITING state can be
> trusted throughout migration by checking it after locking
> threadgroup.
> 
> While at it, renamed css_set_check_fetched to css_set_fetched.
> !css_set_fetched() seems to read better than
> !css_set_check_fetched().
> 
> Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> ---
>  kernel/cgroup.c |   25 ++++++-------------------
>  1 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index bc3caff..4166066 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1856,7 +1856,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
>  	 */
>  	task_lock(tsk);

The comments above this line of code should be updated accordingly.

>  	oldcg = tsk->cgroups;
> -	get_css_set(oldcg);
>  	task_unlock(tsk);
>  
>  	/* locate or allocate a new css_set for this task. */

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Li Zefan]

Mandeep Singh Baines wrote:
> Changes in V3:
> * Fixed to put error in retval (thanks Frederic Weisbecker)
>   * https://lkml.org/lkml/2011/12/20/255
> Changes in V2:
> * Updated commit message as per Tejun's feedback:
>   * https://lkml.org/lkml/2011/12/19/289
> 
> -- >8 -- (snip)
> 
> We can now assume that the css_set reference held by the task
> will not go away for an exiting task. PF_EXITING state can be
> trusted throughout migration by checking it after locking
> threadgroup.
> 
> While at it, renamed css_set_check_fetched to css_set_fetched.
> !css_set_fetched() seems to read better than
> !css_set_check_fetched().
> 
> Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: containers@lists.linux-foundation.org
> Cc: cgroups@vger.kernel.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Paul Menage <paul@paulmenage.org>
> ---
>  kernel/cgroup.c |   25 ++++++-------------------
>  1 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index bc3caff..4166066 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1856,7 +1856,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
>  	 */
>  	task_lock(tsk);

The comments above this line of code should be updated accordingly.

>  	oldcg = tsk->cgroups;
> -	get_css_set(oldcg);
>  	task_unlock(tsk);
>  
>  	/* locate or allocate a new css_set for this task. */

Re: [PATCH 1/5 V3] cgroup: remove redundate get/put of old css_set from migrate

[Li Zefan]

Mandeep Singh Baines wrote:
> Changes in V3:
> * Fixed to put error in retval (thanks Frederic Weisbecker)
>   * https://lkml.org/lkml/2011/12/20/255
> Changes in V2:
> * Updated commit message as per Tejun's feedback:
>   * https://lkml.org/lkml/2011/12/19/289
> 
> -- >8 -- (snip)
> 
> We can now assume that the css_set reference held by the task
> will not go away for an exiting task. PF_EXITING state can be
> trusted throughout migration by checking it after locking
> threadgroup.
> 
> While at it, renamed css_set_check_fetched to css_set_fetched.
> !css_set_fetched() seems to read better than
> !css_set_check_fetched().
> 
> Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> ---
>  kernel/cgroup.c |   25 ++++++-------------------
>  1 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index bc3caff..4166066 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1856,7 +1856,6 @@ static int cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
>  	 */
>  	task_lock(tsk);

The comments above this line of code should be updated accordingly.

>  	oldcg = tsk->cgroups;
> -	get_css_set(oldcg);
>  	task_unlock(tsk);
>  
>  	/* locate or allocate a new css_set for this task. */

Re: [PATCH 3/5] cgroup: only need to check oldcgrp==newgrp once

[Li Zefan]

Mandeep Singh Baines wrote:
> In cgroup_attach_proc it is now sufficient to only check that
> oldcgrp==newcgrp once. Now that we are using threadgroup_lock()
> during the migrations, oldcgrp will not change.
> 
> Signed-off-by: Mandeep Singh Baines <msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Reviewed-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> ---
>  kernel/cgroup.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)

Re: [PATCH 3/5] cgroup: only need to check oldcgrp==newgrp once

[Li Zefan]

Mandeep Singh Baines wrote:
> In cgroup_attach_proc it is now sufficient to only check that
> oldcgrp==newcgrp once. Now that we are using threadgroup_lock()
> during the migrations, oldcgrp will not change.
> 
> Signed-off-by: Mandeep Singh Baines <msb@chromium.org>

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: containers@lists.linux-foundation.org
> Cc: cgroups@vger.kernel.org
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Paul Menage <paul@paulmenage.org>
> ---
>  kernel/cgroup.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)