[PATCH 0/8] cgroup: a bunch of cleanups

[PATCH 0/8] cgroup: a bunch of cleanups

[Li Zefan]

Hi Tejun,

If you're busy with other stuff, just take your time to go through those
patches.

0001-cgroup-remove-cgroup_is_descentant.patch
0002-cgroup-remove-unused-variables-in-cgroup_destroy_loc.patch
0003-cgroup-hold-cgroup_mutex-before-calling-css_offline.patch
0004-cgroup-don-t-bother-to-resize-pid-array.patch
0005-cgroup-remove-useless-code-in-cgroup_write_event_con.patch
0006-cgroup-remove-unneeded-includes-from-cgroup.h.patch
0007-cgroup-fix-an-almost-harmless-off-by-one-bug.patch
0008-cgroup-consolidate-cgroup_attach_task-and-cgroup_att.patch

---
 Documentation/cgroups/cgroups.txt |   1 +
 include/linux/cgroup.h            |  10 +--
 kernel/cgroup.c                   | 239 +++++++++++-----------------------------------------
 kernel/cpuset.c                   |   2 +-
 4 files changed, 52 insertions(+), 200 deletions(-)

[PATCH 0/8] cgroup: a bunch of cleanups

[Li Zefan]

Hi Tejun,

If you're busy with other stuff, just take your time to go through those
patches.

0001-cgroup-remove-cgroup_is_descentant.patch
0002-cgroup-remove-unused-variables-in-cgroup_destroy_loc.patch
0003-cgroup-hold-cgroup_mutex-before-calling-css_offline.patch
0004-cgroup-don-t-bother-to-resize-pid-array.patch
0005-cgroup-remove-useless-code-in-cgroup_write_event_con.patch
0006-cgroup-remove-unneeded-includes-from-cgroup.h.patch
0007-cgroup-fix-an-almost-harmless-off-by-one-bug.patch
0008-cgroup-consolidate-cgroup_attach_task-and-cgroup_att.patch

---
 Documentation/cgroups/cgroups.txt |   1 +
 include/linux/cgroup.h            |  10 +--
 kernel/cgroup.c                   | 239 +++++++++++-----------------------------------------
 kernel/cpuset.c                   |   2 +-
 4 files changed, 52 insertions(+), 200 deletions(-)

[PATCH 1/8] cgroup: remove cgroup_is_descentant()

[Li Zefan]

It was used by ns cgroup, and ns cgroup was removed long ago.

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 include/linux/cgroup.h |  3 ---
 kernel/cgroup.c        | 28 ----------------------------
 2 files changed, 31 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 5f76829..7e818a3 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -448,9 +448,6 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
 
 int cgroup_task_count(const struct cgroup *cgrp);
 
-/* Return true if cgrp is a descendant of the task's cgroup */
-int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task);
-
 /*
  * Control Group taskset, used to pass around set of tasks to cgroup_subsys
  * methods.
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index e72c44e..02b02ca 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -5036,34 +5036,6 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 	put_css_set_taskexit(cg);
 }
 
-/**
- * cgroup_is_descendant - see if @cgrp is a descendant of @task's cgrp
- * @cgrp: the cgroup in question
- * @task: the task in question
- *
- * See if @cgrp is a descendant of @task's cgroup in the appropriate
- * hierarchy.
- *
- * If we are sending in dummytop, then presumably we are creating
- * the top cgroup in the subsystem.
- *
- * Called only by the ns (nsproxy) cgroup.
- */
-int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task)
-{
-	int ret;
-	struct cgroup *target;
-
-	if (cgrp == dummytop)
-		return 1;
-
-	target = task_cgroup_from_root(task, cgrp->root);
-	while (cgrp != target && cgrp!= cgrp->top_cgroup)
-		cgrp = cgrp->parent;
-	ret = (cgrp == target);
-	return ret;
-}
-
 static void check_for_release(struct cgroup *cgrp)
 {
 	/* All of these checks rely on RCU to keep the cgroup
-- 
1.8.0.2

[PATCH 2/8] cgroup: remove unused variables in cgroup_destroy_locked()

[Li Zefan]

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 02b02ca..71bc33d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4347,10 +4347,8 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)
 {
 	struct dentry *d = cgrp->dentry;
 	struct cgroup *parent = cgrp->parent;
-	DEFINE_WAIT(wait);
 	struct cgroup_event *event, *tmp;
 	struct cgroup_subsys *ss;
-	LIST_HEAD(tmp_list);
 
 	lockdep_assert_held(&d->d_inode->i_mutex);
 	lockdep_assert_held(&cgroup_mutex);
-- 
1.8.0.2

[PATCH 3/8] cgroup: hold cgroup_mutex before calling css_offline()

[Li Zefan]

cpuset no longer nests cgroup_mutex inside cpu_hotplug lock, so
we don't have to release cgroup_mutex before calling css_offline().

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 Documentation/cgroups/cgroups.txt |  1 +
 kernel/cgroup.c                   | 11 +----------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
index bcf1a00..0028e88 100644
--- a/Documentation/cgroups/cgroups.txt
+++ b/Documentation/cgroups/cgroups.txt
@@ -580,6 +580,7 @@ propagation along the hierarchy. See the comment on
 cgroup_for_each_descendant_pre() for details.
 
 void css_offline(struct cgroup *cgrp);
+(cgroup_mutex held by caller)
 
 This is the counterpart of css_online() and called iff css_online()
 has succeeded on @cgrp. This signifies the beginning of the end of
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 71bc33d..be524ac 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4170,17 +4170,8 @@ static void offline_css(struct cgroup_subsys *ss, struct cgroup *cgrp)
 	if (!(css->flags & CSS_ONLINE))
 		return;
 
-	/*
-	 * css_offline() should be called with cgroup_mutex unlocked.  See
-	 * 3fa59dfbc3 ("cgroup: fix potential deadlock in pre_destroy") for
-	 * details.  This temporary unlocking should go away once
-	 * cgroup_mutex is unexported from controllers.
-	 */
-	if (ss->css_offline) {
-		mutex_unlock(&cgroup_mutex);
+	if (ss->css_offline)
 		ss->css_offline(cgrp);
-		mutex_lock(&cgroup_mutex);
-	}
 
 	cgrp->subsys[ss->subsys_id]->flags &= ~CSS_ONLINE;
 }
-- 
1.8.0.2

[PATCH 4/8] cgroup: don't bother to resize pid array

[Li Zefan]

When we open cgroup.procs, we'll allocate an buffer and store all tasks'
tgid in it, and then duplicate entries will be stripped. If that results
in a much smaller pid list, we'll re-allocate a smaller buffer.

But we've already sucessfully allocated memory and reading the procs
file is a short period and the memory will be freed very soon, so why
bother to re-allocate memory.

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 37 +++----------------------------------
 1 file changed, 3 insertions(+), 34 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index be524ac..58ae040 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3400,35 +3400,14 @@ static void pidlist_free(void *p)
 	else
 		kfree(p);
 }
-static void *pidlist_resize(void *p, int newcount)
-{
-	void *newlist;
-	/* note: if new alloc fails, old p will still be valid either way */
-	if (is_vmalloc_addr(p)) {
-		newlist = vmalloc(newcount * sizeof(pid_t));
-		if (!newlist)
-			return NULL;
-		memcpy(newlist, p, newcount * sizeof(pid_t));
-		vfree(p);
-	} else {
-		newlist = krealloc(p, newcount * sizeof(pid_t), GFP_KERNEL);
-	}
-	return newlist;
-}
 
 /*
  * pidlist_uniq - given a kmalloc()ed list, strip out all duplicate entries
- * If the new stripped list is sufficiently smaller and there's enough memory
- * to allocate a new buffer, will let go of the unneeded memory. Returns the
- * number of unique elements.
+ * Returns the number of unique elements.
  */
-/* is the size difference enough that we should re-allocate the array? */
-#define PIDLIST_REALLOC_DIFFERENCE(old, new) ((old) - PAGE_SIZE >= (new))
-static int pidlist_uniq(pid_t **p, int length)
+static int pidlist_uniq(pid_t *list, int length)
 {
 	int src, dest = 1;
-	pid_t *list = *p;
-	pid_t *newlist;
 
 	/*
 	 * we presume the 0th element is unique, so i starts at 1. trivial
@@ -3449,16 +3428,6 @@ static int pidlist_uniq(pid_t **p, int length)
 		dest++;
 	}
 after:
-	/*
-	 * if the length difference is large enough, we want to allocate a
-	 * smaller buffer to save memory. if this fails due to out of memory,
-	 * we'll just stay with what we've got.
-	 */
-	if (PIDLIST_REALLOC_DIFFERENCE(length, dest)) {
-		newlist = pidlist_resize(list, dest);
-		if (newlist)
-			*p = newlist;
-	}
 	return dest;
 }
 
@@ -3554,7 +3523,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
 	/* now sort & (if procs) strip out duplicates */
 	sort(array, length, sizeof(pid_t), cmppid, NULL);
 	if (type == CGROUP_FILE_PROCS)
-		length = pidlist_uniq(&array, length);
+		length = pidlist_uniq(array, length);
 	l = cgroup_pidlist_find(cgrp, type);
 	if (!l) {
 		pidlist_free(array);
-- 
1.8.0.2

[PATCH 5/8] cgroup: remove useless code in cgroup_write_event_control()

[Li Zefan]

eventfd_poll() never returns POLLHUP.

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 58ae040..3206137 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3938,12 +3938,6 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	if (ret)
 		goto fail;
 
-	if (efile->f_op->poll(efile, &event->pt) & POLLHUP) {
-		event->cft->unregister_event(cgrp, event->cft, event->eventfd);
-		ret = 0;
-		goto fail;
-	}
-
 	/*
 	 * Events should be removed after rmdir of cgroup directory, but before
 	 * destroying subsystem state objects. Let's take reference to cgroup
-- 
1.8.0.2

Re: [PATCH 0/8] cgroup: a bunch of cleanups

[Li Zefan]

[PATCH 6/8] cgroup: remove unneeded includes from cgroup.h

Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 include/linux/cgroup.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 7e818a3..1f75a59 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -9,11 +9,8 @@
  */
 
 #include <linux/sched.h>
-#include <linux/cpumask.h>
-#include <linux/nodemask.h>
 #include <linux/rcupdate.h>
 #include <linux/rculist.h>
-#include <linux/cgroupstats.h>
 #include <linux/prio_heap.h>
 #include <linux/rwsem.h>
 #include <linux/idr.h>
@@ -22,6 +19,7 @@
 
 #ifdef CONFIG_CGROUPS
 
+struct cgroupstats;
 struct cgroupfs_root;
 struct cgroup_subsys;
 struct inode;
-- 
1.8.0.2

Re: [PATCH 0/8] cgroup: a bunch of cleanups

[Li Zefan]

[PATCH 6/8] cgroup: remove unneeded includes from cgroup.h

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 include/linux/cgroup.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 7e818a3..1f75a59 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -9,11 +9,8 @@
  */
 
 #include <linux/sched.h>
-#include <linux/cpumask.h>
-#include <linux/nodemask.h>
 #include <linux/rcupdate.h>
 #include <linux/rculist.h>
-#include <linux/cgroupstats.h>
 #include <linux/prio_heap.h>
 #include <linux/rwsem.h>
 #include <linux/idr.h>
@@ -22,6 +19,7 @@
 
 #ifdef CONFIG_CGROUPS
 
+struct cgroupstats;
 struct cgroupfs_root;
 struct cgroup_subsys;
 struct inode;
-- 
1.8.0.2

[PATCH 7/8] cgroup: fix an almost harmless off-by-one bug

[Li Zefan]

The 3rd parameter of flex_array_prealloc() is the number of elements,
not the index of the last element.

The effect of the bug is, when opening cgroup.procs, a flex array will
be allocated and all elements of the array is allocated with GFP_KERNEL
flag, but the last one is GFP_ATOMIC, , and if we fail to allocate memory
for it, it'll trigger a BUG_ON().

Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 3206137..4b92b9a 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2076,7 +2076,7 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	if (!group)
 		return -ENOMEM;
 	/* pre-allocate to guarantee space while iterating in rcu read-side. */
-	retval = flex_array_prealloc(group, 0, group_size - 1, GFP_KERNEL);
+	retval = flex_array_prealloc(group, 0, group_size, GFP_KERNEL);
 	if (retval)
 		goto out_free_group_list;
 
-- 
1.8.0.2

[PATCH 7/8] cgroup: fix an almost harmless off-by-one bug

[Li Zefan]

The 3rd parameter of flex_array_prealloc() is the number of elements,
not the index of the last element.

The effect of the bug is, when opening cgroup.procs, a flex array will
be allocated and all elements of the array is allocated with GFP_KERNEL
flag, but the last one is GFP_ATOMIC, , and if we fail to allocate memory
for it, it'll trigger a BUG_ON().

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 3206137..4b92b9a 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2076,7 +2076,7 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	if (!group)
 		return -ENOMEM;
 	/* pre-allocate to guarantee space while iterating in rcu read-side. */
-	retval = flex_array_prealloc(group, 0, group_size - 1, GFP_KERNEL);
+	retval = flex_array_prealloc(group, 0, group_size, GFP_KERNEL);
 	if (retval)
 		goto out_free_group_list;
 
-- 
1.8.0.2

[PATCH 8/8] cgroup: consolidate cgroup_attach_task() and cgroup_attach_proc()

[Li Zefan]

These two functions share most of the code.

Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 include/linux/cgroup.h |   3 +-
 kernel/cgroup.c        | 153 ++++++++++++++-----------------------------------
 kernel/cpuset.c        |   2 +-
 3 files changed, 45 insertions(+), 113 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1f75a59..013c6ce 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -691,7 +691,8 @@ struct task_struct *cgroup_iter_next(struct cgroup *cgrp,
 					struct cgroup_iter *it);
 void cgroup_iter_end(struct cgroup *cgrp, struct cgroup_iter *it);
 int cgroup_scan_tasks(struct cgroup_scanner *scan);
-int cgroup_attach_task(struct cgroup *, struct task_struct *);
+int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
+		       bool threadgroup);
 int cgroup_attach_task_all(struct task_struct *from, struct task_struct *);
 
 /*
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4b92b9a..14417b8 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -59,7 +59,7 @@
 #include <linux/vmalloc.h> /* TODO: replace with more sophisticated array */
 #include <linux/eventfd.h>
 #include <linux/poll.h>
-#include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/flex_array.h> /* used in cgroup_attach_task */
 #include <linux/kthread.h>
 
 #include <linux/atomic.h>
@@ -1944,121 +1944,22 @@ static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 }
 
 /**
- * cgroup_attach_task - attach task 'tsk' to cgroup 'cgrp'
- * @cgrp: the cgroup the task is attaching to
- * @tsk: the task to be attached
- *
- * Call with cgroup_mutex and threadgroup locked. May take task_lock of
- * @tsk during call.
- */
-int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
-{
-	int retval = 0;
-	struct cgroup_subsys *ss, *failed_ss = NULL;
-	struct cgroup *oldcgrp;
-	struct cgroupfs_root *root = cgrp->root;
-	struct cgroup_taskset tset = { };
-	struct css_set *newcg;
-
-	/* @tsk either already exited or can't exit until the end */
-	if (tsk->flags & PF_EXITING)
-		return -ESRCH;
-
-	/* Nothing to do if the task is already in that cgroup */
-	oldcgrp = task_cgroup_from_root(tsk, root);
-	if (cgrp == oldcgrp)
-		return 0;
-
-	tset.single.task = tsk;
-	tset.single.cgrp = oldcgrp;
-
-	for_each_subsys(root, ss) {
-		if (ss->can_attach) {
-			retval = ss->can_attach(cgrp, &tset);
-			if (retval) {
-				/*
-				 * Remember on which subsystem the can_attach()
-				 * failed, so that we only call cancel_attach()
-				 * against the subsystems whose can_attach()
-				 * succeeded. (See below)
-				 */
-				failed_ss = ss;
-				goto out;
-			}
-		}
-	}
-
-	newcg = find_css_set(tsk->cgroups, cgrp);
-	if (!newcg) {
-		retval = -ENOMEM;
-		goto out;
-	}
-
-	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
-
-	for_each_subsys(root, ss) {
-		if (ss->attach)
-			ss->attach(cgrp, &tset);
-	}
-
-out:
-	if (retval) {
-		for_each_subsys(root, ss) {
-			if (ss == failed_ss)
-				/*
-				 * This subsystem was the one that failed the
-				 * can_attach() check earlier, so we don't need
-				 * to call cancel_attach() against it or any
-				 * remaining subsystems.
-				 */
-				break;
-			if (ss->cancel_attach)
-				ss->cancel_attach(cgrp, &tset);
-		}
-	}
-	return retval;
-}
-
-/**
- * cgroup_attach_task_all - attach task 'tsk' to all cgroups of task 'from'
- * @from: attach to all cgroups of a given task
- * @tsk: the task to be attached
- */
-int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
-{
-	struct cgroupfs_root *root;
-	int retval = 0;
-
-	cgroup_lock();
-	for_each_active_root(root) {
-		struct cgroup *from_cg = task_cgroup_from_root(from, root);
-
-		retval = cgroup_attach_task(from_cg, tsk);
-		if (retval)
-			break;
-	}
-	cgroup_unlock();
-
-	return retval;
-}
-EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
-
-/**
- * cgroup_attach_proc - attach all threads in a threadgroup to a cgroup
+ * cgroup_attach_task - attach a task or a whole threadgroup to a cgroup
  * @cgrp: the cgroup to attach to
- * @leader: the threadgroup leader task_struct of the group to be attached
+ * @tsk: the task or the leader of the threadgroup to be attached
+ * @threadgroup: attach the whole threadgroup?
  *
  * Call holding cgroup_mutex and the group_rwsem of the leader. Will take
  * task_lock of each thread in leader's threadgroup individually in turn.
  */
-static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
+int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
+		       bool threadgroup)
 {
 	int retval, i, group_size;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
-	/* guaranteed to be initialized later, but the compiler needs this */
 	struct cgroupfs_root *root = cgrp->root;
+	struct task_struct *leader = tsk;
 	/* threadgroup list cursor and array */
-	struct task_struct *tsk;
 	struct task_and_cgroup *tc;
 	struct flex_array *group;
 	struct cgroup_taskset tset = { };
@@ -2070,7 +1971,10 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 * group - group_rwsem prevents new threads from appearing, and if
 	 * threads exit, this will just be an over-estimate.
 	 */
-	group_size = get_nr_threads(leader);
+	if (threadgroup)
+		group_size = get_nr_threads(tsk);
+	else
+		group_size = 1;
 	/* flex_array supports very large thread-groups better than kmalloc. */
 	group = flex_array_alloc(sizeof(*tc), group_size, GFP_KERNEL);
 	if (!group)
@@ -2080,7 +1984,6 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	if (retval)
 		goto out_free_group_list;
 
-	tsk = leader;
 	i = 0;
 	/*
 	 * Prevent freeing of tasks while we take a snapshot. Tasks that are
@@ -2109,6 +2012,9 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		retval = flex_array_put(group, i, &ent, GFP_ATOMIC);
 		BUG_ON(retval != 0);
 		i++;
+
+		if (!threadgroup)
+			break;
 	} while_each_thread(leader, tsk);
 	rcu_read_unlock();
 	/* remember the number of threads in the array for later. */
@@ -2193,6 +2099,30 @@ out_free_group_list:
 	return retval;
 }
 
+/**
+ * cgroup_attach_task_all - attach task 'tsk' to all cgroups of task 'from'
+ * @from: attach to all cgroups of a given task
+ * @tsk: the task to be attached
+ */
+int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
+{
+	struct cgroupfs_root *root;
+	int retval = 0;
+
+	cgroup_lock();
+	for_each_active_root(root) {
+		struct cgroup *from_cg = task_cgroup_from_root(from, root);
+
+		retval = cgroup_attach_task(from_cg, tsk, false);
+		if (retval)
+			break;
+	}
+	cgroup_unlock();
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
+
 /*
  * Find the task_struct of the task to attach by vpid and pass it along to the
  * function to attach either it or all tasks in its threadgroup. Will lock
@@ -2262,9 +2192,10 @@ retry_find_task:
 			put_task_struct(tsk);
 			goto retry_find_task;
 		}
-		ret = cgroup_attach_proc(cgrp, tsk);
-	} else
-		ret = cgroup_attach_task(cgrp, tsk);
+	}
+
+	ret = cgroup_attach_task(cgrp, tsk, threadgroup);
+
 	threadgroup_unlock(tsk);
 
 	put_task_struct(tsk);
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index ace5bfc..c55763b 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -2008,7 +2008,7 @@ static void cpuset_do_move_task(struct task_struct *tsk,
 	struct cgroup *new_cgroup = scan->data;
 
 	cgroup_lock();
-	cgroup_attach_task(new_cgroup, tsk);
+	cgroup_attach_task(new_cgroup, tsk, false);
 	cgroup_unlock();
 }
 
-- 
1.8.0.2

[PATCH 8/8] cgroup: consolidate cgroup_attach_task() and cgroup_attach_proc()

[Li Zefan]

These two functions share most of the code.

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 include/linux/cgroup.h |   3 +-
 kernel/cgroup.c        | 153 ++++++++++++++-----------------------------------
 kernel/cpuset.c        |   2 +-
 3 files changed, 45 insertions(+), 113 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1f75a59..013c6ce 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -691,7 +691,8 @@ struct task_struct *cgroup_iter_next(struct cgroup *cgrp,
 					struct cgroup_iter *it);
 void cgroup_iter_end(struct cgroup *cgrp, struct cgroup_iter *it);
 int cgroup_scan_tasks(struct cgroup_scanner *scan);
-int cgroup_attach_task(struct cgroup *, struct task_struct *);
+int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
+		       bool threadgroup);
 int cgroup_attach_task_all(struct task_struct *from, struct task_struct *);
 
 /*
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 4b92b9a..14417b8 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -59,7 +59,7 @@
 #include <linux/vmalloc.h> /* TODO: replace with more sophisticated array */
 #include <linux/eventfd.h>
 #include <linux/poll.h>
-#include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/flex_array.h> /* used in cgroup_attach_task */
 #include <linux/kthread.h>
 
 #include <linux/atomic.h>
@@ -1944,121 +1944,22 @@ static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
 }
 
 /**
- * cgroup_attach_task - attach task 'tsk' to cgroup 'cgrp'
- * @cgrp: the cgroup the task is attaching to
- * @tsk: the task to be attached
- *
- * Call with cgroup_mutex and threadgroup locked. May take task_lock of
- * @tsk during call.
- */
-int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
-{
-	int retval = 0;
-	struct cgroup_subsys *ss, *failed_ss = NULL;
-	struct cgroup *oldcgrp;
-	struct cgroupfs_root *root = cgrp->root;
-	struct cgroup_taskset tset = { };
-	struct css_set *newcg;
-
-	/* @tsk either already exited or can't exit until the end */
-	if (tsk->flags & PF_EXITING)
-		return -ESRCH;
-
-	/* Nothing to do if the task is already in that cgroup */
-	oldcgrp = task_cgroup_from_root(tsk, root);
-	if (cgrp == oldcgrp)
-		return 0;
-
-	tset.single.task = tsk;
-	tset.single.cgrp = oldcgrp;
-
-	for_each_subsys(root, ss) {
-		if (ss->can_attach) {
-			retval = ss->can_attach(cgrp, &tset);
-			if (retval) {
-				/*
-				 * Remember on which subsystem the can_attach()
-				 * failed, so that we only call cancel_attach()
-				 * against the subsystems whose can_attach()
-				 * succeeded. (See below)
-				 */
-				failed_ss = ss;
-				goto out;
-			}
-		}
-	}
-
-	newcg = find_css_set(tsk->cgroups, cgrp);
-	if (!newcg) {
-		retval = -ENOMEM;
-		goto out;
-	}
-
-	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
-
-	for_each_subsys(root, ss) {
-		if (ss->attach)
-			ss->attach(cgrp, &tset);
-	}
-
-out:
-	if (retval) {
-		for_each_subsys(root, ss) {
-			if (ss == failed_ss)
-				/*
-				 * This subsystem was the one that failed the
-				 * can_attach() check earlier, so we don't need
-				 * to call cancel_attach() against it or any
-				 * remaining subsystems.
-				 */
-				break;
-			if (ss->cancel_attach)
-				ss->cancel_attach(cgrp, &tset);
-		}
-	}
-	return retval;
-}
-
-/**
- * cgroup_attach_task_all - attach task 'tsk' to all cgroups of task 'from'
- * @from: attach to all cgroups of a given task
- * @tsk: the task to be attached
- */
-int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
-{
-	struct cgroupfs_root *root;
-	int retval = 0;
-
-	cgroup_lock();
-	for_each_active_root(root) {
-		struct cgroup *from_cg = task_cgroup_from_root(from, root);
-
-		retval = cgroup_attach_task(from_cg, tsk);
-		if (retval)
-			break;
-	}
-	cgroup_unlock();
-
-	return retval;
-}
-EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
-
-/**
- * cgroup_attach_proc - attach all threads in a threadgroup to a cgroup
+ * cgroup_attach_task - attach a task or a whole threadgroup to a cgroup
  * @cgrp: the cgroup to attach to
- * @leader: the threadgroup leader task_struct of the group to be attached
+ * @tsk: the task or the leader of the threadgroup to be attached
+ * @threadgroup: attach the whole threadgroup?
  *
  * Call holding cgroup_mutex and the group_rwsem of the leader. Will take
  * task_lock of each thread in leader's threadgroup individually in turn.
  */
-static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
+int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
+		       bool threadgroup)
 {
 	int retval, i, group_size;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
-	/* guaranteed to be initialized later, but the compiler needs this */
 	struct cgroupfs_root *root = cgrp->root;
+	struct task_struct *leader = tsk;
 	/* threadgroup list cursor and array */
-	struct task_struct *tsk;
 	struct task_and_cgroup *tc;
 	struct flex_array *group;
 	struct cgroup_taskset tset = { };
@@ -2070,7 +1971,10 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	 * group - group_rwsem prevents new threads from appearing, and if
 	 * threads exit, this will just be an over-estimate.
 	 */
-	group_size = get_nr_threads(leader);
+	if (threadgroup)
+		group_size = get_nr_threads(tsk);
+	else
+		group_size = 1;
 	/* flex_array supports very large thread-groups better than kmalloc. */
 	group = flex_array_alloc(sizeof(*tc), group_size, GFP_KERNEL);
 	if (!group)
@@ -2080,7 +1984,6 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	if (retval)
 		goto out_free_group_list;
 
-	tsk = leader;
 	i = 0;
 	/*
 	 * Prevent freeing of tasks while we take a snapshot. Tasks that are
@@ -2109,6 +2012,9 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		retval = flex_array_put(group, i, &ent, GFP_ATOMIC);
 		BUG_ON(retval != 0);
 		i++;
+
+		if (!threadgroup)
+			break;
 	} while_each_thread(leader, tsk);
 	rcu_read_unlock();
 	/* remember the number of threads in the array for later. */
@@ -2193,6 +2099,30 @@ out_free_group_list:
 	return retval;
 }
 
+/**
+ * cgroup_attach_task_all - attach task 'tsk' to all cgroups of task 'from'
+ * @from: attach to all cgroups of a given task
+ * @tsk: the task to be attached
+ */
+int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
+{
+	struct cgroupfs_root *root;
+	int retval = 0;
+
+	cgroup_lock();
+	for_each_active_root(root) {
+		struct cgroup *from_cg = task_cgroup_from_root(from, root);
+
+		retval = cgroup_attach_task(from_cg, tsk, false);
+		if (retval)
+			break;
+	}
+	cgroup_unlock();
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(cgroup_attach_task_all);
+
 /*
  * Find the task_struct of the task to attach by vpid and pass it along to the
  * function to attach either it or all tasks in its threadgroup. Will lock
@@ -2262,9 +2192,10 @@ retry_find_task:
 			put_task_struct(tsk);
 			goto retry_find_task;
 		}
-		ret = cgroup_attach_proc(cgrp, tsk);
-	} else
-		ret = cgroup_attach_task(cgrp, tsk);
+	}
+
+	ret = cgroup_attach_task(cgrp, tsk, threadgroup);
+
 	threadgroup_unlock(tsk);
 
 	put_task_struct(tsk);
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index ace5bfc..c55763b 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -2008,7 +2008,7 @@ static void cpuset_do_move_task(struct task_struct *tsk,
 	struct cgroup *new_cgroup = scan->data;
 
 	cgroup_lock();
-	cgroup_attach_task(new_cgroup, tsk);
+	cgroup_attach_task(new_cgroup, tsk, false);
 	cgroup_unlock();
 }
 
-- 
1.8.0.2

Re: [PATCH 0/8] cgroup: a bunch of cleanups

[Tejun Heo]

On Tue, Mar 12, 2013 at 02:49:52PM +0800, Li Zefan wrote:
> Hi Tejun,
> 
> If you're busy with other stuff, just take your time to go through those
> patches.
> 
> 0001-cgroup-remove-cgroup_is_descentant.patch
> 0002-cgroup-remove-unused-variables-in-cgroup_destroy_loc.patch
> 0003-cgroup-hold-cgroup_mutex-before-calling-css_offline.patch
> 0004-cgroup-don-t-bother-to-resize-pid-array.patch
> 0005-cgroup-remove-useless-code-in-cgroup_write_event_con.patch
> 0006-cgroup-remove-unneeded-includes-from-cgroup.h.patch
> 0007-cgroup-fix-an-almost-harmless-off-by-one-bug.patch
> 0008-cgroup-consolidate-cgroup_attach_task-and-cgroup_att.patch

0001-0007 applied to cgroup/for-3.10.  0008 looks fine but in the diff
cgroup_attach_task_all() is removed completely and added back in
slightly different form.  If you wanna keep the functions ordered like
you posted, please put a separate patch to move
cgroup_attach_task_all() before the consolidation.

Thanks.

-- 
tejun

Re: [PATCH 0/8] cgroup: a bunch of cleanups

[Tejun Heo]

On Tue, Mar 12, 2013 at 02:49:52PM +0800, Li Zefan wrote:
> Hi Tejun,
> 
> If you're busy with other stuff, just take your time to go through those
> patches.
> 
> 0001-cgroup-remove-cgroup_is_descentant.patch
> 0002-cgroup-remove-unused-variables-in-cgroup_destroy_loc.patch
> 0003-cgroup-hold-cgroup_mutex-before-calling-css_offline.patch
> 0004-cgroup-don-t-bother-to-resize-pid-array.patch
> 0005-cgroup-remove-useless-code-in-cgroup_write_event_con.patch
> 0006-cgroup-remove-unneeded-includes-from-cgroup.h.patch
> 0007-cgroup-fix-an-almost-harmless-off-by-one-bug.patch
> 0008-cgroup-consolidate-cgroup_attach_task-and-cgroup_att.patch

0001-0007 applied to cgroup/for-3.10.  0008 looks fine but in the diff
cgroup_attach_task_all() is removed completely and added back in
slightly different form.  If you wanna keep the functions ordered like
you posted, please put a separate patch to move
cgroup_attach_task_all() before the consolidation.

Thanks.

-- 
tejun

Re: [PATCH 0/8] cgroup: a bunch of cleanups

[Li Zefan]

On 2013/3/13 6:38, Tejun Heo wrote:
> On Tue, Mar 12, 2013 at 02:49:52PM +0800, Li Zefan wrote:
>> Hi Tejun,
>>
>> If you're busy with other stuff, just take your time to go through those
>> patches.
>>
>> 0001-cgroup-remove-cgroup_is_descentant.patch
>> 0002-cgroup-remove-unused-variables-in-cgroup_destroy_loc.patch
>> 0003-cgroup-hold-cgroup_mutex-before-calling-css_offline.patch
>> 0004-cgroup-don-t-bother-to-resize-pid-array.patch
>> 0005-cgroup-remove-useless-code-in-cgroup_write_event_con.patch
>> 0006-cgroup-remove-unneeded-includes-from-cgroup.h.patch
>> 0007-cgroup-fix-an-almost-harmless-off-by-one-bug.patch
>> 0008-cgroup-consolidate-cgroup_attach_task-and-cgroup_att.patch
> 
> 0001-0007 applied to cgroup/for-3.10.  0008 looks fine but in the diff
> cgroup_attach_task_all() is removed completely and added back in
> slightly different form.

At first I thought cgroup_attach_task() was static, so I thought I had
to either move cgroup_attach_task_all() after cgroup_attach_task(),
or add a forward declaration of cgroup_attach_task() in order to pass
compile.

Then I found it was extern, but I forgot to revert this diff.

Will send a v2.

> If you wanna keep the functions ordered like
> you posted, please put a separate patch to move
> cgroup_attach_task_all() before the consolidation.
> 

Re: [PATCH 0/8] cgroup: a bunch of cleanups

[Li Zefan]

On 2013/3/13 6:38, Tejun Heo wrote:
> On Tue, Mar 12, 2013 at 02:49:52PM +0800, Li Zefan wrote:
>> Hi Tejun,
>>
>> If you're busy with other stuff, just take your time to go through those
>> patches.
>>
>> 0001-cgroup-remove-cgroup_is_descentant.patch
>> 0002-cgroup-remove-unused-variables-in-cgroup_destroy_loc.patch
>> 0003-cgroup-hold-cgroup_mutex-before-calling-css_offline.patch
>> 0004-cgroup-don-t-bother-to-resize-pid-array.patch
>> 0005-cgroup-remove-useless-code-in-cgroup_write_event_con.patch
>> 0006-cgroup-remove-unneeded-includes-from-cgroup.h.patch
>> 0007-cgroup-fix-an-almost-harmless-off-by-one-bug.patch
>> 0008-cgroup-consolidate-cgroup_attach_task-and-cgroup_att.patch
> 
> 0001-0007 applied to cgroup/for-3.10.  0008 looks fine but in the diff
> cgroup_attach_task_all() is removed completely and added back in
> slightly different form.

At first I thought cgroup_attach_task() was static, so I thought I had
to either move cgroup_attach_task_all() after cgroup_attach_task(),
or add a forward declaration of cgroup_attach_task() in order to pass
compile.

Then I found it was extern, but I forgot to revert this diff.

Will send a v2.

> If you wanna keep the functions ordered like
> you posted, please put a separate patch to move
> cgroup_attach_task_all() before the consolidation.
> 

[PATCH 6/8] cgroup: remove unneeded includes from cgroup.h

[Li Zefan]

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 include/linux/cgroup.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 7e818a3..1f75a59 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -9,11 +9,8 @@
  */
 
 #include <linux/sched.h>
-#include <linux/cpumask.h>
-#include <linux/nodemask.h>
 #include <linux/rcupdate.h>
 #include <linux/rculist.h>
-#include <linux/cgroupstats.h>
 #include <linux/prio_heap.h>
 #include <linux/rwsem.h>
 #include <linux/idr.h>
@@ -22,6 +19,7 @@
 
 #ifdef CONFIG_CGROUPS
 
+struct cgroupstats;
 struct cgroupfs_root;
 struct cgroup_subsys;
 struct inode;
-- 1.8.0.2