[PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Li Zefan]

On x86_32, sizeof(struct cgroup_subsys) shrinks from 276 bytes
to 264.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
include/linux/cgroup.h |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index ed4ba11..e23ded6 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -481,14 +481,16 @@ struct cgroup_subsys {
 	void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
 
 	int subsys_id;
-	int active;
-	int disabled;
-	int early_init;
+
+	unsigned int active:1;
+	unsigned int disabled:1;
+	unsigned int early_init:1;
 	/*
 	 * True if this subsys uses ID. ID is not available before cgroup_init()
 	 * (not available in early_init time.)
 	 */
-	bool use_id;
+	unsigned int use_id:1;
+
 #define MAX_CGROUP_TYPE_NAMELEN 32
 	const char *name;
 
-- 
1.7.0.1

[PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Li Zefan]

Stephane posted a patchset to add perf_cgroup subsystem, so perf can
be used to monitor all threads belonging to a cgroup.

But if you already mounted a cgroup hierarchy but without perf_cgroup
and the hierarchy has sub-cgroups, you can't bind perf_cgroup to it,
and thus you're not able to use per-cgroup perf feature.

This patchset alleviates the pain, and then a subsytem can be bind/unbind
to/from a hierarchy which has sub-cgroups in it.

For a cgroup subsystem to become bindable, the can_bind flag of
struct cgroup_subsys should be set, and provide ->bind() callback
if necessary.

But for some constraints, not all subsystems can take advantage of
this patch. For example, we can't decide a cgroup's cpuset.mems and
cpuset.cpus automatically, so cpuset is not bindable.

Usage:

# mount -t cgroup -o cpuset xxx /mnt
# mkdir /mnt/tmp
# echo $$ > /mnt/tmp/tasks

(assume cpuacct is bindable, and we add cpuacct to the hierarchy)
# mount -o remount,cpuset,cpuacct xxx /mnt

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h |    5 +
 kernel/cgroup.c        |  225 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 187 insertions(+), 43 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index e23ded6..49369ff 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -490,6 +490,11 @@ struct cgroup_subsys {
 	 * (not available in early_init time.)
 	 */
 	unsigned int use_id:1;
+	/*
+	 * Indicate if this subsystem can be bound/unbound to/from a cgroup
+	 * hierarchy which has child cgroups.
+	 */
+	unsigned int can_bind:1;
 
 #define MAX_CGROUP_TYPE_NAMELEN 32
 	const char *name;
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 6c36750..46df5f8 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -57,6 +57,7 @@
 #include <linux/vmalloc.h> /* TODO: replace with more sophisticated array */
 #include <linux/eventfd.h>
 #include <linux/poll.h>
+#include <linux/bitops.h>
 
 #include <asm/atomic.h>
 
@@ -870,18 +871,13 @@ static void remove_dir(struct dentry *d)
 
 static void cgroup_clear_directory(struct dentry *dentry)
 {
-	struct list_head *node;
+	struct dentry *d, *tmp;
 
 	BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
 	spin_lock(&dcache_lock);
-	node = dentry->d_subdirs.next;
-	while (node != &dentry->d_subdirs) {
-		struct dentry *d = list_entry(node, struct dentry, d_u.d_child);
-		list_del_init(node);
-		if (d->d_inode) {
-			/* This should never be called on a cgroup
-			 * directory with child cgroups */
-			BUG_ON(d->d_inode->i_mode & S_IFDIR);
+	list_for_each_entry_safe(d, tmp, &dentry->d_subdirs, d_u.d_child) {
+		if (d->d_inode && !(d->d_inode->i_mode & S_IFDIR)) {
+			list_del_init(&d->d_u.d_child);
 			d = dget_locked(d);
 			spin_unlock(&dcache_lock);
 			d_delete(d);
@@ -889,7 +885,6 @@ static void cgroup_clear_directory(struct dentry *dentry)
 			dput(d);
 			spin_lock(&dcache_lock);
 		}
-		node = dentry->d_subdirs.next;
 	}
 	spin_unlock(&dcache_lock);
 }
@@ -934,6 +929,145 @@ void cgroup_release_and_wakeup_rmdir(struct cgroup_subsys_state *css)
 	css_put(css);
 }
 
+static void init_cgroup_css(struct cgroup_subsys_state *css,
+			       struct cgroup_subsys *ss,
+			       struct cgroup *cgrp)
+{
+	css->cgroup = cgrp;
+	atomic_set(&css->refcnt, 1);
+	css->flags = 0;
+	css->id = NULL;
+	if (cgrp == dummytop)
+		set_bit(CSS_ROOT, &css->flags);
+	BUG_ON(cgrp->subsys[ss->subsys_id]);
+	cgrp->subsys[ss->subsys_id] = css;
+}
+
+/*
+ * cgroup_walk_herarchy - iterate through a cgroup hierarchy
+ * @process_cgroup: callback called on each cgroup in the hierarchy
+ * @data: will be passed to @process_cgroup
+ * @top_cgrp: the root cgroup of the hierarchy
+ *
+ * For such a hierarchy:
+ *        a1        c1
+ *      /         /
+ * Root - a2 - b1 - c2
+ *      \
+ *        a3
+ *
+ * The iterating order is: a1, a2, b1, c1, c2, a3. So a parent will be
+ * processed before its children.
+ */
+static int cgroup_walk_hierarchy(int (*process_cgroup)(struct cgroup *, void *),
+				 void *data, struct cgroup *top_cgrp)
+{
+	struct cgroup *parent = top_cgrp;
+	struct cgroup *child;
+	struct list_head *node;
+	int ret;
+
+	node = parent->children.next;
+repeat:
+	while (node != &parent->children) {
+		child = list_entry(node, struct cgroup, sibling);
+
+		ret = process_cgroup(child, data);
+		if (ret)
+			return ret;
+
+		if (!list_empty(&child->children)) {
+			parent = child;
+			node = parent->children.next;
+			goto repeat;
+		} else
+			node = node->next;
+	}
+
+	if (parent != top_cgrp) {
+		child = parent;
+		parent = child->parent;
+		node = child->sibling.next;
+		goto repeat;
+	}
+
+	return 0;
+}
+
+static int hierarchy_attach_css_failed(struct cgroup *cgrp, void *data)
+{
+	unsigned long added_bits = (unsigned long)data;
+	int i;
+
+	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT);
+		if (cgrp->subsys[i])
+			subsys[i]->destroy(subsys[i], cgrp);
+
+	return 0;
+}
+
+static int hierarchy_attach_css(struct cgroup *cgrp, void *data)
+{
+	unsigned long added_bits = (unsigned long)data;
+	int i;
+	int ret = 0;
+
+	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
+		struct cgroup_subsys_state *css;
+		struct cgroup_subsys *ss = subsys[i];
+
+		css = ss->create(ss, cgrp);
+		if (IS_ERR(css)) {
+			ret = PTR_ERR(css);
+			break;
+		}
+		init_cgroup_css(css, ss, cgrp);
+
+		if (ss->use_id) {
+			ret = alloc_css_id(ss, cgrp->parent, cgrp);
+			if (ret)
+				break;
+		}
+	}
+
+	if (ret)
+		cgroup_walk_hierarchy(hierarchy_attach_css_failed, data,
+				      cgrp->top_cgroup);
+	return ret;
+}
+
+static int hierarchy_update_css_sets(struct cgroup *cgrp, void *data)
+{
+	unsigned long added_bits = (unsigned long)data;
+	int i;
+	struct cg_cgroup_link *link;
+
+	write_lock(&css_set_lock);
+	list_for_each_entry(link, &cgrp->css_sets, cgrp_link_list) {
+		struct css_set *cg = link->cg;
+		struct hlist_head *hhead;
+
+		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
+			cg->subsys[i] = cgrp->subsys[i];
+
+		/* rehash */
+		hlist_del(&cg->hlist);
+		hhead = css_set_hash(cg->subsys);
+		hlist_add_head(&cg->hlist, hhead);
+	}
+	write_unlock(&css_set_lock);
+
+	return 0;
+}
+
+static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
+{
+	mutex_lock_nested(&cgrp->dentry->d_inode->i_mutex, I_MUTEX_CHILD);
+	cgroup_populate_dir(cgrp);
+	mutex_unlock(&cgrp->dentry->d_inode->i_mutex);
+	return 0;
+}
+
 /*
  * Call with cgroup_mutex held. Drops reference counts on modules, including
  * any duplicate ones that parse_cgroupfs_options took. If this function
@@ -945,36 +1079,53 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 	unsigned long added_bits, removed_bits;
 	struct cgroup *cgrp = &root->top_cgroup;
 	int i;
+	int err;
 
 	BUG_ON(!mutex_is_locked(&cgroup_mutex));
 
 	removed_bits = root->actual_subsys_bits & ~final_bits;
 	added_bits = final_bits & ~root->actual_subsys_bits;
+
 	/* Check that any added subsystems are currently free */
-	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
-		unsigned long bit = 1UL << i;
-		struct cgroup_subsys *ss = subsys[i];
-		if (!(bit & added_bits))
-			continue;
+	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
 		/*
 		 * Nobody should tell us to do a subsys that doesn't exist:
 		 * parse_cgroupfs_options should catch that case and refcounts
 		 * ensure that subsystems won't disappear once selected.
 		 */
-		BUG_ON(ss == NULL);
-		if (ss->root != &rootnode) {
+		BUG_ON(subsys[i] == NULL);
+		if (subsys[i]->root != &rootnode) {
 			/* Subsystem isn't free */
 			return -EBUSY;
 		}
 	}
 
-	/* Currently we don't handle adding/removing subsystems when
-	 * any child cgroups exist. This is theoretically supportable
-	 * but involves complex error handling, so it's being left until
-	 * later */
-	if (root->number_of_cgroups > 1)
+	/* removing will be supported later */
+	if (root->number_of_cgroups > 1 && removed_bits)
 		return -EBUSY;
 
+	if (root->number_of_cgroups > 1) {
+		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
+			if (!subsys[i]->can_bind)
+				return -EBUSY;
+
+	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
+		BUG_ON(cgrp->subsys[i]);
+		BUG_ON(!dummytop->subsys[i]);
+		BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
+
+		cgrp->subsys[i] = dummytop->subsys[i];
+		cgrp->subsys[i]->cgroup = cgrp;
+	}
+
+	err = cgroup_walk_hierarchy(hierarchy_attach_css,
+				    (void *)added_bits, cgrp);
+	if (err)
+		goto failed;
+
+	cgroup_walk_hierarchy(hierarchy_update_css_sets,
+			      (void *)added_bits, cgrp);
+
 	/* Process each subsystem */
 	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		struct cgroup_subsys *ss = subsys[i];
@@ -982,12 +1133,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 		if (bit & added_bits) {
 			/* We're binding this subsystem to this hierarchy */
 			BUG_ON(ss == NULL);
-			BUG_ON(cgrp->subsys[i]);
-			BUG_ON(!dummytop->subsys[i]);
-			BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
 			mutex_lock(&ss->hierarchy_mutex);
-			cgrp->subsys[i] = dummytop->subsys[i];
-			cgrp->subsys[i]->cgroup = cgrp;
 			list_move(&ss->sibling, &root->subsys_list);
 			ss->root = root;
 			if (ss->bind)
@@ -1000,10 +1146,10 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 			BUG_ON(cgrp->subsys[i] != dummytop->subsys[i]);
 			BUG_ON(cgrp->subsys[i]->cgroup != cgrp);
 			mutex_lock(&ss->hierarchy_mutex);
-			if (ss->bind)
-				ss->bind(ss, dummytop);
 			dummytop->subsys[i]->cgroup = dummytop;
 			cgrp->subsys[i] = NULL;
+			if (ss->bind)
+				ss->bind(ss, dummytop);
 			subsys[i]->root = &rootnode;
 			list_move(&ss->sibling, &rootnode.subsys_list);
 			mutex_unlock(&ss->hierarchy_mutex);
@@ -1030,6 +1176,12 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 	synchronize_rcu();
 
 	return 0;
+
+failed:
+	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
+		cgrp->subsys[i] = NULL;
+
+	return err;
 }
 
 static int cgroup_show_options(struct seq_file *seq, struct vfsmount *vfs)
@@ -1285,6 +1437,7 @@ static int cgroup_remount(struct super_block *sb, int *flags, char *data)
 
 	/* (re)populate subsystem files */
 	cgroup_populate_dir(cgrp);
+	cgroup_walk_hierarchy(hierarchy_populate_dir, NULL, cgrp);
 
 	if (opts.release_agent)
 		strcpy(root->release_agent_path, opts.release_agent);
@@ -3313,20 +3466,6 @@ static int cgroup_populate_dir(struct cgroup *cgrp)
 	return 0;
 }
 
-static void init_cgroup_css(struct cgroup_subsys_state *css,
-			       struct cgroup_subsys *ss,
-			       struct cgroup *cgrp)
-{
-	css->cgroup = cgrp;
-	atomic_set(&css->refcnt, 1);
-	css->flags = 0;
-	css->id = NULL;
-	if (cgrp == dummytop)
-		set_bit(CSS_ROOT, &css->flags);
-	BUG_ON(cgrp->subsys[ss->subsys_id]);
-	cgrp->subsys[ss->subsys_id] = css;
-}
-
 static void cgroup_lock_hierarchy(struct cgroupfs_root *root)
 {
 	/* We need to take each hierarchy_mutex in a consistent order */
-- 
1.7.0.1

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Peter Zijlstra]

On Fri, 2010-10-22 at 16:09 +0800, Li Zefan wrote:
> For example, we can't decide a cgroup's cpuset.mems and
> cpuset.cpus automatically, so cpuset is not bindable. 

You mean to say that you cannot add cpuset to an existing hierarchy
right? Not that you cannot add perf/cpuacct to an existing cpuset
hierarchy?

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Matt Helsley]

On Fri, Oct 22, 2010 at 04:09:56PM +0800, Li Zefan wrote:
> Stephane posted a patchset to add perf_cgroup subsystem, so perf can
> be used to monitor all threads belonging to a cgroup.
> 
> But if you already mounted a cgroup hierarchy but without perf_cgroup
> and the hierarchy has sub-cgroups, you can't bind perf_cgroup to it,
> and thus you're not able to use per-cgroup perf feature.
> 
> This patchset alleviates the pain, and then a subsytem can be bind/unbind
> to/from a hierarchy which has sub-cgroups in it.
> 
> For a cgroup subsystem to become bindable, the can_bind flag of
> struct cgroup_subsys should be set, and provide ->bind() callback
> if necessary.
> 
> But for some constraints, not all subsystems can take advantage of
> this patch. For example, we can't decide a cgroup's cpuset.mems and
> cpuset.cpus automatically, so cpuset is not bindable.

<snip>

> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 6c36750..46df5f8 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c

<snip>

> +/*
> + * cgroup_walk_herarchy - iterate through a cgroup hierarchy
> + * @process_cgroup: callback called on each cgroup in the hierarchy
> + * @data: will be passed to @process_cgroup
> + * @top_cgrp: the root cgroup of the hierarchy
> + *
> + * For such a hierarchy:
> + *        a1        c1
> + *      /         /
> + * Root - a2 - b1 - c2
> + *      \
> + *        a3
> + *
> + * The iterating order is: a1, a2, b1, c1, c2, a3. So a parent will be
> + * processed before its children.
> + */

You could just say it's a depth-first walk except we process the parent before
its children.

> +static int cgroup_walk_hierarchy(int (*process_cgroup)(struct cgroup *, void *),
> +				 void *data, struct cgroup *top_cgrp)

<snip>

> +static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
> +{
> +	mutex_lock_nested(&cgrp->dentry->d_inode->i_mutex, I_MUTEX_CHILD);
> +	cgroup_populate_dir(cgrp);
> +	mutex_unlock(&cgrp->dentry->d_inode->i_mutex);
> +	return 0;
> +}
> +
>  /*
>   * Call with cgroup_mutex held. Drops reference counts on modules, including
>   * any duplicate ones that parse_cgroupfs_options took. If this function
> @@ -945,36 +1079,53 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>  	unsigned long added_bits, removed_bits;
>  	struct cgroup *cgrp = &root->top_cgroup;
>  	int i;
> +	int err;
> 
>  	BUG_ON(!mutex_is_locked(&cgroup_mutex));
> 
>  	removed_bits = root->actual_subsys_bits & ~final_bits;
>  	added_bits = final_bits & ~root->actual_subsys_bits;
> +
>  	/* Check that any added subsystems are currently free */
> -	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> -		unsigned long bit = 1UL << i;
> -		struct cgroup_subsys *ss = subsys[i];
> -		if (!(bit & added_bits))
> -			continue;
> +	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
>  		/*
>  		 * Nobody should tell us to do a subsys that doesn't exist:
>  		 * parse_cgroupfs_options should catch that case and refcounts
>  		 * ensure that subsystems won't disappear once selected.
>  		 */
> -		BUG_ON(ss == NULL);
> -		if (ss->root != &rootnode) {
> +		BUG_ON(subsys[i] == NULL);
> +		if (subsys[i]->root != &rootnode) {
>  			/* Subsystem isn't free */
>  			return -EBUSY;
>  		}
>  	}
> 
> -	/* Currently we don't handle adding/removing subsystems when
> -	 * any child cgroups exist. This is theoretically supportable
> -	 * but involves complex error handling, so it's being left until
> -	 * later */
> -	if (root->number_of_cgroups > 1)
> +	/* removing will be supported later */
> +	if (root->number_of_cgroups > 1 && removed_bits)
>  		return -EBUSY;
> 
> +	if (root->number_of_cgroups > 1) {

Is there something wrong with the indentation here? I can't
see the closing brace for the "if (root->number_of_cgroups > 1)"
that should precede the for_each_set_bit() loop below.

> +		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
> +			if (!subsys[i]->can_bind)
> +				return -EBUSY;

I think you could avoid the can_bind flag field entirely and do:

			if (!subsys[i]->bind)

Also, if we're going with my "split out unbind" suggestion I think
the part here would be:

		for_each_set_bit(i, &removed_bits, CGROUP_SUBSYS_COUNT)
			if (!subsys[i]->unbind)
				return -EBUSY;

<snip>

Cheers,
	-Matt Helsley

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Paul Menage]

On Fri, Oct 22, 2010 at 1:09 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> +       /*
> +        * Indicate if this subsystem can be bound/unbound to/from a cgroup
> +        * hierarchy which has child cgroups.
> +        */
> +       unsigned int can_bind:1;

Maybe call this "bindable"?

Basic idea looks great, it could do with a bunch more comments, and
maybe locking rules.

Is there any chance of a lock inversion between dir->i_mutex and
cgroup_lock in hierarchy_popuiate_dir() ?

Paul

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Li Zefan]

Paul Menage wrote:
> On Fri, Oct 22, 2010 at 1:09 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> +       /*
>> +        * Indicate if this subsystem can be bound/unbound to/from a cgroup
>> +        * hierarchy which has child cgroups.
>> +        */
>> +       unsigned int can_bind:1;
> 
> Maybe call this "bindable"?
> 
> Basic idea looks great, it could do with a bunch more comments, and
> maybe locking rules.
> 
> Is there any chance of a lock inversion between dir->i_mutex and
> cgroup_lock in hierarchy_popuiate_dir() ?
> 

The lock order is:

mutex_lock(&dir->i_mutex)
  mutex_lock(&cgroup_mutex)
    mutex_lock(&dir->i_mutex, I_MUTEX_CHILD)

it should be safe.

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Li Zefan]

Peter Zijlstra wrote:
> On Fri, 2010-10-22 at 16:09 +0800, Li Zefan wrote:
>> For example, we can't decide a cgroup's cpuset.mems and
>> cpuset.cpus automatically, so cpuset is not bindable. 
> 
> You mean to say that you cannot add cpuset to an existing hierarchy
> right? Not that you cannot add perf/cpuacct to an existing cpuset
> hierarchy?
> 

Exactly. You can see from the example. 

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Li Zefan]

>> +/*
>> + * cgroup_walk_herarchy - iterate through a cgroup hierarchy
>> + * @process_cgroup: callback called on each cgroup in the hierarchy
>> + * @data: will be passed to @process_cgroup
>> + * @top_cgrp: the root cgroup of the hierarchy
>> + *
>> + * For such a hierarchy:
>> + *        a1        c1
>> + *      /         /
>> + * Root - a2 - b1 - c2
>> + *      \
>> + *        a3
>> + *
>> + * The iterating order is: a1, a2, b1, c1, c2, a3. So a parent will be
>> + * processed before its children.
>> + */
> 
> You could just say it's a depth-first walk except we process the parent before
> its children.
> 

Will revise the comment. A diagram is intuitive. :)

>> +static int cgroup_walk_hierarchy(int (*process_cgroup)(struct cgroup *, void *),
>> +				 void *data, struct cgroup *top_cgrp)
> 
> <snip>
> 
>> +static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
>> +{
>> +	mutex_lock_nested(&cgrp->dentry->d_inode->i_mutex, I_MUTEX_CHILD);
>> +	cgroup_populate_dir(cgrp);
>> +	mutex_unlock(&cgrp->dentry->d_inode->i_mutex);
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Call with cgroup_mutex held. Drops reference counts on modules, including
>>   * any duplicate ones that parse_cgroupfs_options took. If this function
>> @@ -945,36 +1079,53 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>>  	unsigned long added_bits, removed_bits;
>>  	struct cgroup *cgrp = &root->top_cgroup;
>>  	int i;
>> +	int err;
>>
>>  	BUG_ON(!mutex_is_locked(&cgroup_mutex));
>>
>>  	removed_bits = root->actual_subsys_bits & ~final_bits;
>>  	added_bits = final_bits & ~root->actual_subsys_bits;
>> +
>>  	/* Check that any added subsystems are currently free */
>> -	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>> -		unsigned long bit = 1UL << i;
>> -		struct cgroup_subsys *ss = subsys[i];
>> -		if (!(bit & added_bits))
>> -			continue;
>> +	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
>>  		/*
>>  		 * Nobody should tell us to do a subsys that doesn't exist:
>>  		 * parse_cgroupfs_options should catch that case and refcounts
>>  		 * ensure that subsystems won't disappear once selected.
>>  		 */
>> -		BUG_ON(ss == NULL);
>> -		if (ss->root != &rootnode) {
>> +		BUG_ON(subsys[i] == NULL);
>> +		if (subsys[i]->root != &rootnode) {
>>  			/* Subsystem isn't free */
>>  			return -EBUSY;
>>  		}
>>  	}
>>
>> -	/* Currently we don't handle adding/removing subsystems when
>> -	 * any child cgroups exist. This is theoretically supportable
>> -	 * but involves complex error handling, so it's being left until
>> -	 * later */
>> -	if (root->number_of_cgroups > 1)
>> +	/* removing will be supported later */
>> +	if (root->number_of_cgroups > 1 && removed_bits)
>>  		return -EBUSY;
>>
>> +	if (root->number_of_cgroups > 1) {
> 
> Is there something wrong with the indentation here? I can't
> see the closing brace for the "if (root->number_of_cgroups > 1)"
> that should precede the for_each_set_bit() loop below.
> 

Oops, I must have forgot to commit the change when I fixed this.

>> +		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
>> +			if (!subsys[i]->can_bind)
>> +				return -EBUSY;
> 
> I think you could avoid the can_bind flag field entirely and do:
> 
> 			if (!subsys[i]->bind)
> 

Nope. For some subsystems we just set the flag and need not to
provide a bind() callback.

> Also, if we're going with my "split out unbind" suggestion I think
> the part here would be:
> 
> 		for_each_set_bit(i, &removed_bits, CGROUP_SUBSYS_COUNT)
> 			if (!subsys[i]->unbind)
> 				return -EBUSY;
> 

Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Paul Menage]

On Fri, Oct 22, 2010 at 2:38 PM, Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
>> + *
>> + * The iterating order is: a1, a2, b1, c1, c2, a3. So a parent will be
>> + * processed before its children.
>> + */
>
> You could just say it's a depth-first walk except we process the parent before
> its children.

The standard term for that is "pre-order traversal". You shouldn't
need a diagram.

Paul

[PATCH 3/7] cgroups: Allow to unbind subsystem from a cgroup hierarachy

[Li Zefan]

This allows us to unbind a cgroup subsystem from a hierarchy
which has sub-cgroups in it.

Due to some complexity, for now subsytems that use css_get/put()
can't be unbound from such a hierarchy.

Usage:

# mount -t cgroup -o cpuset,cpuacct xxx /mnt
# mkdir /mnt/tmp
# echo $$ > /mnt/tmp/tasks

(remove it from the hierarchy)
# mount -o remount,cpuset xxx /mnt

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 kernel/cgroup.c |   76 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 46df5f8..9ce3fdb 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1036,19 +1036,42 @@ static int hierarchy_attach_css(struct cgroup *cgrp, void *data)
 	return ret;
 }
 
-static int hierarchy_update_css_sets(struct cgroup *cgrp, void *data)
+static int hierarchy_remove_css(struct cgroup *cgrp, void *data)
 {
-	unsigned long added_bits = (unsigned long)data;
+	unsigned long removed_bits = (unsigned long)data;
 	int i;
+
+	/*
+	 * There might be some pointers to the cgroup_subsys_state
+	 * that we are going to destroy.
+	 */
+	synchronize_rcu();
+
+	for_each_set_bit(i, &removed_bits, CGROUP_SUBSYS_COUNT) {
+		subsys[i]->destroy(subsys[i], cgrp);
+		cgrp->subsys[i] = NULL;
+	}
+
+	return 0;
+}
+
+static void hierarchy_update_css_sets(struct cgroup *cgrp,
+				      unsigned long bits, bool add)
+{
 	struct cg_cgroup_link *link;
+	int i;
 
 	write_lock(&css_set_lock);
 	list_for_each_entry(link, &cgrp->css_sets, cgrp_link_list) {
 		struct css_set *cg = link->cg;
 		struct hlist_head *hhead;
 
-		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
-			cg->subsys[i] = cgrp->subsys[i];
+		for_each_set_bit(i, &bits, CGROUP_SUBSYS_COUNT) {
+			if (add)
+				cg->subsys[i] = cgrp->subsys[i];
+			else
+				cg->subsys[i] = dummytop->subsys[i];
+		}
 
 		/* rehash */
 		hlist_del(&cg->hlist);
@@ -1056,7 +1079,21 @@ static int hierarchy_update_css_sets(struct cgroup *cgrp, void *data)
 		hlist_add_head(&cg->hlist, hhead);
 	}
 	write_unlock(&css_set_lock);
+}
 
+static int hierarchy_add_to_css_sets(struct cgroup *cgrp, void *data)
+{
+	unsigned long added_bits = (unsigned long)data;
+
+	hierarchy_update_css_sets(cgrp, added_bits, true);
+	return 0;
+}
+
+static int hierarchy_remove_from_css_sets(struct cgroup *cgrp, void *data)
+{
+	unsigned long removed_bits = (unsigned long)data;
+
+	hierarchy_update_css_sets(cgrp, removed_bits, false);
 	return 0;
 }
 
@@ -1076,7 +1113,7 @@ static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
 static int rebind_subsystems(struct cgroupfs_root *root,
 			      unsigned long final_bits)
 {
-	unsigned long added_bits, removed_bits;
+	unsigned long added_bits, removed_bits, changed_bits;
 	struct cgroup *cgrp = &root->top_cgroup;
 	int i;
 	int err;
@@ -1085,6 +1122,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 
 	removed_bits = root->actual_subsys_bits & ~final_bits;
 	added_bits = final_bits & ~root->actual_subsys_bits;
+	changed_bits = removed_bits | added_bits;
 
 	/* Check that any added subsystems are currently free */
 	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
@@ -1100,14 +1138,11 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 		}
 	}
 
-	/* removing will be supported later */
-	if (root->number_of_cgroups > 1 && removed_bits)
-		return -EBUSY;
-
 	if (root->number_of_cgroups > 1) {
-		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
+		for_each_set_bit(i, &changed_bits, CGROUP_SUBSYS_COUNT)
 			if (!subsys[i]->can_bind)
 				return -EBUSY;
+	}
 
 	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
 		BUG_ON(cgrp->subsys[i]);
@@ -1123,7 +1158,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 	if (err)
 		goto failed;
 
-	cgroup_walk_hierarchy(hierarchy_update_css_sets,
+	cgroup_walk_hierarchy(hierarchy_add_to_css_sets,
 			      (void *)added_bits, cgrp);
 
 	/* Process each subsystem */
@@ -1143,11 +1178,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 		} else if (bit & removed_bits) {
 			/* We're removing this subsystem */
 			BUG_ON(ss == NULL);
-			BUG_ON(cgrp->subsys[i] != dummytop->subsys[i]);
-			BUG_ON(cgrp->subsys[i]->cgroup != cgrp);
 			mutex_lock(&ss->hierarchy_mutex);
-			dummytop->subsys[i]->cgroup = dummytop;
-			cgrp->subsys[i] = NULL;
 			if (ss->bind)
 				ss->bind(ss, dummytop);
 			subsys[i]->root = &rootnode;
@@ -1173,6 +1204,21 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 		}
 	}
 	root->subsys_bits = root->actual_subsys_bits = final_bits;
+
+	for_each_set_bit(i, &removed_bits, CGROUP_SUBSYS_COUNT) {
+		BUG_ON(cgrp->subsys[i] != dummytop->subsys[i]);
+		BUG_ON(cgrp->subsys[i]->cgroup != cgrp);
+
+		dummytop->subsys[i]->cgroup = dummytop;
+		cgrp->subsys[i] = NULL;
+	}
+
+	cgroup_walk_hierarchy(hierarchy_remove_from_css_sets,
+			      (void *)removed_bits, cgrp);
+
+	cgroup_walk_hierarchy(hierarchy_remove_css,
+			      (void *)removed_bits, cgrp);
+
 	synchronize_rcu();
 
 	return 0;
-- 
1.7.0.1

[PATCH 4/7] cgroups: Mark some subsystems bindable

[Li Zefan]

For those subsystems (debug, cpuacct, net_cls and devices),
setting the can_bind flag is sufficient.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 kernel/cgroup.c          |    1 +
 kernel/sched.c           |    1 +
 net/sched/cls_cgroup.c   |    1 +
 security/device_cgroup.c |    1 +
 4 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 9ce3fdb..6364bb5 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -5124,5 +5124,6 @@ struct cgroup_subsys debug_subsys = {
 	.destroy = debug_destroy,
 	.populate = debug_populate,
 	.subsys_id = debug_subsys_id,
+	.can_bind = 1,
 };
 #endif /* CONFIG_CGROUP_DEBUG */
diff --git a/kernel/sched.c b/kernel/sched.c
index 51944e8..cae104f 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -9329,6 +9329,7 @@ struct cgroup_subsys cpuacct_subsys = {
 	.destroy = cpuacct_destroy,
 	.populate = cpuacct_populate,
 	.subsys_id = cpuacct_subsys_id,
+	.can_bind = 1,
 };
 #endif	/* CONFIG_CGROUP_CPUACCT */
 
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 37dff78..020ddfe 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -38,6 +38,7 @@ struct cgroup_subsys net_cls_subsys = {
 #define net_cls_subsys_id net_cls_subsys.subsys_id
 #endif
 	.module		= THIS_MODULE,
+	.can_bind	= 1,
 };
 
 
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 8d9c48f..b8136fc 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -473,6 +473,7 @@ struct cgroup_subsys devices_subsys = {
 	.destroy = devcgroup_destroy,
 	.populate = devcgroup_populate,
 	.subsys_id = devices_subsys_id,
+	.can_bind = 1,
 };
 
 int devcgroup_inode_permission(struct inode *inode, int mask)
-- 
1.7.0.1

[PATCH 5/7] cgroups: Make freezer subsystem bindable

[Li Zefan]

To make it bindable, we need to thaw all processes when unbinding
the freezer subsystem from a cgroup hierarchy.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h  |    3 ++-
 kernel/cgroup.c         |   22 ++++++++++++++++++++--
 kernel/cgroup_freezer.c |   19 +++++++++++++++++--
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 49369ff..1e4e1df 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -478,7 +478,8 @@ struct cgroup_subsys {
 	int (*populate)(struct cgroup_subsys *ss,
 			struct cgroup *cgrp);
 	void (*post_clone)(struct cgroup_subsys *ss, struct cgroup *cgrp);
-	void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
+	void (*bind)(struct cgroup_subsys *ss, struct cgroup *cgrp,
+		     bool unbind);
 
 	int subsys_id;
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 6364bb5..12c1f7c 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1105,6 +1105,22 @@ static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
 	return 0;
 }
 
+static int hierarchy_subsys_bind(struct cgroup *cgrp, void *data)
+{
+	struct cgroup_subsys *ss = data;
+
+	ss->bind(ss, cgrp, false);
+	return 0;
+}
+
+static int hierarchy_subsys_unbind(struct cgroup *cgrp, void *data)
+{
+	struct cgroup_subsys *ss = data;;
+
+	ss->bind(ss, cgrp, true);
+	return 0;
+}
+
 /*
  * Call with cgroup_mutex held. Drops reference counts on modules, including
  * any duplicate ones that parse_cgroupfs_options took. If this function
@@ -1172,7 +1188,8 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 			list_move(&ss->sibling, &root->subsys_list);
 			ss->root = root;
 			if (ss->bind)
-				ss->bind(ss, cgrp);
+				cgroup_walk_hierarchy(hierarchy_subsys_bind,
+						      ss, cgrp);
 			mutex_unlock(&ss->hierarchy_mutex);
 			/* refcount was already taken, and we're keeping it */
 		} else if (bit & removed_bits) {
@@ -1180,7 +1197,8 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 			BUG_ON(ss == NULL);
 			mutex_lock(&ss->hierarchy_mutex);
 			if (ss->bind)
-				ss->bind(ss, dummytop);
+				cgroup_walk_hierarchy(hierarchy_subsys_unbind,
+						      ss, cgrp);
 			subsys[i]->root = &rootnode;
 			list_move(&ss->sibling, &rootnode.subsys_list);
 			mutex_unlock(&ss->hierarchy_mutex);
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index e7bebb7..de13ce4 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -383,6 +383,21 @@ static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }
 
+/*
+ * Thaw all processes before unbinding the freezer subsysem from
+ * cgroup hierarchy.
+ * */
+static void freezer_bind(struct cgroup_subsys *ss, struct cgroup *cgrp,
+			 bool unbind)
+{
+	struct freezer *freezer = cgroup_freezer(cgrp);
+
+	if (!unbind)
+		return;
+
+	unfreeze_cgroup(cgrp, freezer);
+}
+
 struct cgroup_subsys freezer_subsys = {
 	.name		= "freezer",
 	.create		= freezer_create,
@@ -390,7 +405,7 @@ struct cgroup_subsys freezer_subsys = {
 	.populate	= freezer_populate,
 	.subsys_id	= freezer_subsys_id,
 	.can_attach	= freezer_can_attach,
-	.attach		= NULL,
 	.fork		= freezer_fork,
-	.exit		= NULL,
+	.bind		= freezer_bind,
+	.can_bind	= 1,
 };
-- 
1.7.0.1

[PATCH 6/7] cgroups: Warn if a bindable subsystem calls css_get()

[Li Zefan]

For now bindable subsystems should not use css_get/put(), so warn
on this misuse.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h |    7 +++++--
 kernel/cgroup.c        |    3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1e4e1df..10e4e02 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -80,13 +80,15 @@ struct cgroup_subsys_state {
 
 /* bits in struct cgroup_subsys_state flags field */
 enum {
-	CSS_ROOT, /* This CSS is the root of the subsystem */
-	CSS_REMOVED, /* This CSS is dead */
+	CSS_ROOT,	/* This CSS is the root of the subsystem */
+	CSS_REMOVED,	/* This CSS is dead */
+	CSS_NO_GET,	/* Forbid calling css_get/put() */
 };
 
 /* Caller must verify that the css is not for root cgroup */
 static inline void __css_get(struct cgroup_subsys_state *css, int count)
 {
+	BUG_ON(test_bit(CSS_NO_GET, &css->flags));
 	atomic_add(count, &css->refcnt);
 }
 
@@ -119,6 +121,7 @@ static inline bool css_tryget(struct cgroup_subsys_state *css)
 {
 	if (test_bit(CSS_ROOT, &css->flags))
 		return true;
+	BUG_ON(test_bit(CSS_NO_GET, &css->flags));
 	while (!atomic_inc_not_zero(&css->refcnt)) {
 		if (test_bit(CSS_REMOVED, &css->flags))
 			return false;
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 12c1f7c..88e265d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -936,6 +936,9 @@ static void init_cgroup_css(struct cgroup_subsys_state *css,
 	css->cgroup = cgrp;
 	atomic_set(&css->refcnt, 1);
 	css->flags = 0;
+	/* For now, a bindable subsystem should not call css_get/put(). */
+	if (ss->can_bind)
+		set_bit(CSS_NO_GET, &css->flags);
 	css->id = NULL;
 	if (cgrp == dummytop)
 		set_bit(CSS_ROOT, &css->flags);
-- 
1.7.0.1

[PATCH 7/7] cgroups: Update documentation for bindable subsystems

[Li Zefan]

Provide a usage example, and update the bind() callback API.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 Documentation/cgroups/cgroups.txt |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
index 190018b..5b5382a 100644
--- a/Documentation/cgroups/cgroups.txt
+++ b/Documentation/cgroups/cgroups.txt
@@ -363,17 +363,23 @@ Note this will add ns to the hierarchy but won't remove memory or
 cpuset, because the new options are appended to the old ones:
 # mount -o remount,ns /dev/cgroup
 
+For some subsystems you can bind them to a mounted hierarchy or
+remove them from it, even if there're sub-cgroups in it:
+# mount -t cgroup -o freezer hier1 /dev/cgroup
+# echo $$ > /dev/cgroup/my_cgroup
+# mount -o freezer,cpuset hier1 /dev/cgroup
+(failed)
+# mount -o freezer,cpuacct hier1 /dev/cgroup
+# mount -o cpuacct hier1 /dev/cgroup
+
+Note cpuacct should be sit in the default hierarchy before remount.
+
 To Specify a hierarchy's release_agent:
 # mount -t cgroup -o cpuset,release_agent="/sbin/cpuset_release_agent" \
   xxx /dev/cgroup
 
 Note that specifying 'release_agent' more than once will return failure.
 
-Note that changing the set of subsystems is currently only supported
-when the hierarchy consists of a single (root) cgroup. Supporting
-the ability to arbitrarily bind/unbind subsystems from an existing
-cgroup hierarchy is intended to be implemented in the future.
-
 Then under /dev/cgroup you can find a tree that corresponds to the
 tree of the cgroups in the system. For instance, /dev/cgroup
 is the cgroup that holds the whole system.
@@ -623,13 +629,15 @@ initialization which might be required before a task could attach.  For
 example in cpusets, no task may attach before 'cpus' and 'mems' are set
 up.
 
-void bind(struct cgroup_subsys *ss, struct cgroup *root)
+void bind(struct cgroup_subsys *ss, struct cgroup *cgrp, bool unbind)
 (cgroup_mutex and ss->hierarchy_mutex held by caller)
 
 Called when a cgroup subsystem is rebound to a different hierarchy
-and root cgroup. Currently this will only involve movement between
-the default hierarchy (which never has sub-cgroups) and a hierarchy
-that is being created/destroyed (and hence has no sub-cgroups).
+and root cgroup. For some subsystems this will only involve movement
+between the default hierarchy (which never has sub-cgroups) and a
+hierarchy that is being created/destroyed (and hence has no sub-cgroups).
+For some other subsystems this can involve movement between the default
+hierarchy and a mounted hierarchy which may have sub-cgroups in it.
 
 4. Questions
 ============
-- 
1.7.0.1

Re: [PATCH 0/7] cgroups: Allow to bind/unbind subsystems to/from non-trival hierarchy

[Peter Zijlstra]

On Fri, 2010-10-22 at 16:09 +0800, Li Zefan wrote:
> Stephane posted a patchset to add perf_cgroup subsystem, so perf can
> be used to monitor all threads belonging to a cgroup.
> 
> But if you already mounted a cgroup hierarchy but without perf_cgroup
> and the hierarchy has sub-cgroups, you can't bind perf_cgroup to it,
> and thus you're not able to use per-cgroup perf feature.
> 
> This patchset alleviates the pain, and then a subsytem can be
> bound/unbound to/from a hierarchy which has sub-cgroups in it.
> 
> Some subsystems still can't take advantage of this patchset, memcgroup
> and cpuset in specific.
> 
> For cpuset, if a hierarchy has a sub-cgroup and the cgroup has tasks,
> we can't decide sub-cgroup's cpuset.mems and cpuset.cpus automatically
> if we try to bind cpuset to this hierarchy.
> 
> For memcgroup, memcgroup uses css_get/put(), and due to some complexity,
> for now bindable subsystems should not use css_get/put().
> 
> Usage:
> 
> # mount -t cgroup -o cpuset xxx /mnt
> # mkdir /mnt/tmp
> # echo $$ > /mnt/tmp/tasks
> 
> (add cpuacct to the hierarchy)
> # mount -o remount,cpuset,cpuacct xxx /mnt
> 
> (remove it from the hierarchy)
> # mount -o remount,cpuset xxx /mnt
> 
> There's another limitation, cpuacct should not be bound to any mounted
> hierarchy before the above operation. But that's not a problem, as you
> can remove it from a hierarchy and bind it to another one.

Right, so the only remaining problem I see with this approach is that
you cannot profile two different hierarchies at the same time, but I
can't really think of a solution to that problem (nor do I care very
much).

Seems like a nice approach, Thanks Li!

Re: [PATCH 5/7] cgroups: Make freezer subsystem bindable

[Matt Helsley]

On Fri, Oct 22, 2010 at 04:11:41PM +0800, Li Zefan wrote:
> To make it bindable, we need to thaw all processes when unbinding
> the freezer subsystem from a cgroup hierarchy.
> 
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Based on experience using cgroups and questions we've fielded in the
past on IRC I'd say users will really appreciate this.

We're planning to use the freezer in checkpoint/restart. Since
checkpoint requires the tasks to remain frozen for the duration of
the syscall we add a kernel-internal freezer subsystem interface
which prevents the cgroup from thawing. So we'll need some way to
"block" unbinding for that as well.

Perhaps the bind op should be able to return an error when
unbind == true? Although that raises the question of what to do if
only one of multiple unbinds fails..

Alternately you could split the bind/unbind op function pointers
and get rid of the boolean argument. Then just don't fill in the
freezer's unbind op and refuse to unbind subsystems that lack
the unbind op. That seems a bit cleaner for now at least.

Cheers,
	-Matt Helsley

> ---
>  include/linux/cgroup.h  |    3 ++-
>  kernel/cgroup.c         |   22 ++++++++++++++++++++--
>  kernel/cgroup_freezer.c |   19 +++++++++++++++++--
>  3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 49369ff..1e4e1df 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -478,7 +478,8 @@ struct cgroup_subsys {
>  	int (*populate)(struct cgroup_subsys *ss,
>  			struct cgroup *cgrp);
>  	void (*post_clone)(struct cgroup_subsys *ss, struct cgroup *cgrp);
> -	void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
> +	void (*bind)(struct cgroup_subsys *ss, struct cgroup *cgrp,
> +		     bool unbind);
> 
>  	int subsys_id;
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 6364bb5..12c1f7c 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1105,6 +1105,22 @@ static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
>  	return 0;
>  }
> 
> +static int hierarchy_subsys_bind(struct cgroup *cgrp, void *data)
> +{
> +	struct cgroup_subsys *ss = data;
> +
> +	ss->bind(ss, cgrp, false);
> +	return 0;
> +}
> +
> +static int hierarchy_subsys_unbind(struct cgroup *cgrp, void *data)
> +{
> +	struct cgroup_subsys *ss = data;;

nit: two semicolons

> +
> +	ss->bind(ss, cgrp, true);
> +	return 0;
> +}
> +
>  /*
>   * Call with cgroup_mutex held. Drops reference counts on modules, including
>   * any duplicate ones that parse_cgroupfs_options took. If this function
> @@ -1172,7 +1188,8 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>  			list_move(&ss->sibling, &root->subsys_list);
>  			ss->root = root;
>  			if (ss->bind)
> -				ss->bind(ss, cgrp);
> +				cgroup_walk_hierarchy(hierarchy_subsys_bind,
> +						      ss, cgrp);
>  			mutex_unlock(&ss->hierarchy_mutex);
>  			/* refcount was already taken, and we're keeping it */
>  		} else if (bit & removed_bits) {
> @@ -1180,7 +1197,8 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>  			BUG_ON(ss == NULL);
>  			mutex_lock(&ss->hierarchy_mutex);
>  			if (ss->bind)
> -				ss->bind(ss, dummytop);
> +				cgroup_walk_hierarchy(hierarchy_subsys_unbind,
> +						      ss, cgrp);
>  			subsys[i]->root = &rootnode;
>  			list_move(&ss->sibling, &rootnode.subsys_list);
>  			mutex_unlock(&ss->hierarchy_mutex);
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e7bebb7..de13ce4 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -383,6 +383,21 @@ static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
>  	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
>  }
> 
> +/*
> + * Thaw all processes before unbinding the freezer subsysem from
> + * cgroup hierarchy.
> + * */
> +static void freezer_bind(struct cgroup_subsys *ss, struct cgroup *cgrp,
> +			 bool unbind)
> +{
> +	struct freezer *freezer = cgroup_freezer(cgrp);
> +
> +	if (!unbind)
> +		return;
> +
> +	unfreeze_cgroup(cgrp, freezer);
> +}
> +
>  struct cgroup_subsys freezer_subsys = {
>  	.name		= "freezer",
>  	.create		= freezer_create,
> @@ -390,7 +405,7 @@ struct cgroup_subsys freezer_subsys = {
>  	.populate	= freezer_populate,
>  	.subsys_id	= freezer_subsys_id,
>  	.can_attach	= freezer_can_attach,
> -	.attach		= NULL,
>  	.fork		= freezer_fork,
> -	.exit		= NULL,
> +	.bind		= freezer_bind,
> +	.can_bind	= 1,
>  };
> -- 
> 1.7.0.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] cgroups: Make freezer subsystem bindable

[Matt Helsley]

On Fri, Oct 22, 2010 at 01:57:55PM -0700, Matt Helsley wrote:
> On Fri, Oct 22, 2010 at 04:11:41PM +0800, Li Zefan wrote:
> > To make it bindable, we need to thaw all processes when unbinding
> > the freezer subsystem from a cgroup hierarchy.
> > 
> > Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

<snip>

> 
> Alternately you could split the bind/unbind op function pointers
> and get rid of the boolean argument. Then just don't fill in the
> freezer's unbind op and refuse to unbind subsystems that lack
> the unbind op. That seems a bit cleaner for now at least.

Or alternately to the alternate :):  add a can_unbind:1 field and keep
the boolean bind op argument. That might be best -- avoids an extra
function pointer and still splits the bind/unbind-ability of a
subsystem.

Cheers,
	-Matt Helsley

Re: [PATCH 5/7] cgroups: Make freezer subsystem bindable

[Matt Helsley]

On Fri, Oct 22, 2010 at 01:57:55PM -0700, Matt Helsley wrote:
> On Fri, Oct 22, 2010 at 04:11:41PM +0800, Li Zefan wrote:
> > To make it bindable, we need to thaw all processes when unbinding
> > the freezer subsystem from a cgroup hierarchy.
> > 
> > Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> 
> Based on experience using cgroups and questions we've fielded in the
> past on IRC I'd say users will really appreciate this.
> 
> We're planning to use the freezer in checkpoint/restart. Since
> checkpoint requires the tasks to remain frozen for the duration of
> the syscall we add a kernel-internal freezer subsystem interface
> which prevents the cgroup from thawing. So we'll need some way to
> "block" unbinding for that as well.

And in case you're curious here are the portions of the checkpoint/restart
tree which implement the above:

http://www.linux-cr.org/git/?p=linux-cr.git;a=commitdiff;h=a584c1c6b729a544c76cbb173ce62ffb6575b1d3
http://www.linux-cr.org/git/?p=linux-cr.git;a=commitdiff;h=34869146ecd662352c0024e0769ec77e643fb1e1

Cheers,
	-Matt Helsley

Re: [PATCH 5/7] cgroups: Make freezer subsystem bindable

[Li Zefan]

Matt Helsley wrote:
> On Fri, Oct 22, 2010 at 04:11:41PM +0800, Li Zefan wrote:
>> To make it bindable, we need to thaw all processes when unbinding
>> the freezer subsystem from a cgroup hierarchy.
>>
>> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> 
> Based on experience using cgroups and questions we've fielded in the
> past on IRC I'd say users will really appreciate this.
> 
> We're planning to use the freezer in checkpoint/restart. Since
> checkpoint requires the tasks to remain frozen for the duration of
> the syscall we add a kernel-internal freezer subsystem interface
> which prevents the cgroup from thawing. So we'll need some way to
> "block" unbinding for that as well.
> 

How about, we allow unbinding only when all the cgroups' state is
THAWED ?

> Perhaps the bind op should be able to return an error when
> unbind == true? Although that raises the question of what to do if
> only one of multiple unbinds fails..
> 
> Alternately you could split the bind/unbind op function pointers
> and get rid of the boolean argument. Then just don't fill in the
> freezer's unbind op and refuse to unbind subsystems that lack
> the unbind op. That seems a bit cleaner for now at least.
> 

We can use bindable:1 along with a callback can_bind().

For some subsystems, we just set bindable to true. For freezer subsystem,
we set it to true and provide freezer_can_bind(), and return true only
if no cgroup's state is FROZEN.

Re: [PATCH 5/7] cgroups: Make freezer subsystem bindable

[Paul Menage]

On Fri, Oct 22, 2010 at 2:46 PM, Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
>> Alternately you could split the bind/unbind op function pointers
>> and get rid of the boolean argument. Then just don't fill in the
>> freezer's unbind op and refuse to unbind subsystems that lack
>> the unbind op. That seems a bit cleaner for now at least.
>
> Or alternately to the alternate :):  add a can_unbind:1 field and keep
> the boolean bind op argument. That might be best -- avoids an extra
> function pointer and still splits the bind/unbind-ability of a
> subsystem.

I think I'd vote for the separate bind() / unbind() callbacks.

Paul

Re: [PATCH 7/7] cgroups: Update documentation for bindable subsystems

[KAMEZAWA Hiroyuki]

On Fri, 22 Oct 2010 16:12:25 +0800
Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:

> Provide a usage example, and update the bind() callback API.
> 
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> ---
>  Documentation/cgroups/cgroups.txt |   26 +++++++++++++++++---------
>  1 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
> index 190018b..5b5382a 100644
> --- a/Documentation/cgroups/cgroups.txt
> +++ b/Documentation/cgroups/cgroups.txt
> @@ -363,17 +363,23 @@ Note this will add ns to the hierarchy but won't remove memory or
>  cpuset, because the new options are appended to the old ones:
>  # mount -o remount,ns /dev/cgroup
>  
> +For some subsystems you can bind them to a mounted hierarchy or
> +remove them from it, even if there're sub-cgroups in it:
> +# mount -t cgroup -o freezer hier1 /dev/cgroup
> +# echo $$ > /dev/cgroup/my_cgroup
> +# mount -o freezer,cpuset hier1 /dev/cgroup
> +(failed)
> +# mount -o freezer,cpuacct hier1 /dev/cgroup
> +# mount -o cpuacct hier1 /dev/cgroup
> +
> +Note cpuacct should be sit in the default hierarchy before remount.
> +

Can this operation be used with noop cgroup ?
(IOW, perf cgroup can be attached to noop cgroup ?)

Thanks,
-Kame

Re: [PATCH 7/7] cgroups: Update documentation for bindable subsystems

[Paul Menage]

>
>  Called when a cgroup subsystem is rebound to a different hierarchy
> -and root cgroup. Currently this will only involve movement between
> -the default hierarchy (which never has sub-cgroups) and a hierarchy
> -that is being created/destroyed (and hence has no sub-cgroups).
> +and root cgroup. For some subsystems this will only involve movement
> +between the default hierarchy (which never has sub-cgroups) and a
> +hierarchy that is being created/destroyed (and hence has no sub-cgroups).
> +For some other subsystems this can involve movement between the default
> +hierarchy and a mounted hierarchy which may have sub-cgroups in it.

This is a bit vague. How about:

For non-bindable subsystems, this will  only involve movement
between the default hierarchy (which never has sub-cgroups) and a
hierarchy that is being created/destroyed (and hence has no sub-cgroups).

For binadable subsystems, this may also involve movement between the
default hierarchy and a mounted hierarchy that's populated with
sub-cgroups.

Also, the docs should mention that a cgroup setting the can_bind flag
has to be able to support side-effect free movement of a task into any
just-created cgroup, and into the root cgroup at any time. i.e. it's
not suitable for any subsystem where can_attach() might return false
for the root cgroup or a newly-created cgroup, or attach() might have
side-effects for those same cases.

Actually, perhaps we should forbid the combination of having both an
attach() callback and can_bind=true ?

Also, post_clone() doesn't get called when creating the css hierarchy
during binding.

Paul

Re: [PATCH 7/7] cgroups: Update documentation for bindable subsystems

[Li Zefan]

KAMEZAWA Hiroyuki wrote:
> On Fri, 22 Oct 2010 16:12:25 +0800
> Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> 
>> Provide a usage example, and update the bind() callback API.
>>
>> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>> ---
>>  Documentation/cgroups/cgroups.txt |   26 +++++++++++++++++---------
>>  1 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
>> index 190018b..5b5382a 100644
>> --- a/Documentation/cgroups/cgroups.txt
>> +++ b/Documentation/cgroups/cgroups.txt
>> @@ -363,17 +363,23 @@ Note this will add ns to the hierarchy but won't remove memory or
>>  cpuset, because the new options are appended to the old ones:
>>  # mount -o remount,ns /dev/cgroup
>>  
>> +For some subsystems you can bind them to a mounted hierarchy or
>> +remove them from it, even if there're sub-cgroups in it:
>> +# mount -t cgroup -o freezer hier1 /dev/cgroup
>> +# echo $$ > /dev/cgroup/my_cgroup
>> +# mount -o freezer,cpuset hier1 /dev/cgroup
>> +(failed)
>> +# mount -o freezer,cpuacct hier1 /dev/cgroup
>> +# mount -o cpuacct hier1 /dev/cgroup
>> +
>> +Note cpuacct should be sit in the default hierarchy before remount.
>> +
> 
> Can this operation be used with noop cgroup ?
> (IOW, perf cgroup can be attached to noop cgroup ?)
> 

Yes, you can. I've tested it.

Re: [PATCH 7/7] cgroups: Update documentation for bindable subsystems

[Li Zefan]

Li Zefan wrote:
> KAMEZAWA Hiroyuki wrote:
>> On Fri, 22 Oct 2010 16:12:25 +0800
>> Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>>
>>> Provide a usage example, and update the bind() callback API.
>>>
>>> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>>> ---
>>>  Documentation/cgroups/cgroups.txt |   26 +++++++++++++++++---------
>>>  1 files changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
>>> index 190018b..5b5382a 100644
>>> --- a/Documentation/cgroups/cgroups.txt
>>> +++ b/Documentation/cgroups/cgroups.txt
>>> @@ -363,17 +363,23 @@ Note this will add ns to the hierarchy but won't remove memory or
>>>  cpuset, because the new options are appended to the old ones:
>>>  # mount -o remount,ns /dev/cgroup
>>>  
>>> +For some subsystems you can bind them to a mounted hierarchy or
>>> +remove them from it, even if there're sub-cgroups in it:
>>> +# mount -t cgroup -o freezer hier1 /dev/cgroup
>>> +# echo $$ > /dev/cgroup/my_cgroup
>>> +# mount -o freezer,cpuset hier1 /dev/cgroup
>>> +(failed)
>>> +# mount -o freezer,cpuacct hier1 /dev/cgroup
>>> +# mount -o cpuacct hier1 /dev/cgroup
>>> +
>>> +Note cpuacct should be sit in the default hierarchy before remount.
>>> +
>> Can this operation be used with noop cgroup ?
>> (IOW, perf cgroup can be attached to noop cgroup ?)
>>
> 
> Yes, you can. I've tested it.

But, wait. It stuck when I moved a task from a sub-cgroup to the root.
Need to dig more..

Re: [PATCH 7/7] cgroups: Update documentation for bindable subsystems

[Paul Menage]

On Thu, Oct 28, 2010 at 5:13 PM, Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> Also, post_clone() doesn't get called when creating the css hierarchy
> during binding.
>

Oops, ignore that - I noticed that it does.

Paul

Re: [PATCH 7/7] cgroups: Update documentation for bindable subsystems

[Li Zefan]

Paul Menage wrote:
>>  Called when a cgroup subsystem is rebound to a different hierarchy
>> -and root cgroup. Currently this will only involve movement between
>> -the default hierarchy (which never has sub-cgroups) and a hierarchy
>> -that is being created/destroyed (and hence has no sub-cgroups).
>> +and root cgroup. For some subsystems this will only involve movement
>> +between the default hierarchy (which never has sub-cgroups) and a
>> +hierarchy that is being created/destroyed (and hence has no sub-cgroups).
>> +For some other subsystems this can involve movement between the default
>> +hierarchy and a mounted hierarchy which may have sub-cgroups in it.
> 
> This is a bit vague. How about:
> 
> For non-bindable subsystems, this will  only involve movement
> between the default hierarchy (which never has sub-cgroups) and a
> hierarchy that is being created/destroyed (and hence has no sub-cgroups).
> 
> For binadable subsystems, this may also involve movement between the
> default hierarchy and a mounted hierarchy that's populated with
> sub-cgroups.
> 
> Also, the docs should mention that a cgroup setting the can_bind flag
> has to be able to support side-effect free movement of a task into any
> just-created cgroup, and into the root cgroup at any time. i.e. it's
> not suitable for any subsystem where can_attach() might return false
> for the root cgroup or a newly-created cgroup, or attach() might have
> side-effects for those same cases.
> 
> Actually, perhaps we should forbid the combination of having both an
> attach() callback and can_bind=true ?
> 
> Also, post_clone() doesn't get called when creating the css hierarchy
> during binding.
> 

This is much better. :)

Documentation often causes my headache due to my limited English skill.

Re: [PATCH 0/7] cgroups: Allow to bind/unbind subsystems to/from non-trival hierarchy

[Li Zefan]

Peter Zijlstra wrote:
> On Fri, 2010-10-22 at 16:09 +0800, Li Zefan wrote:
>> Stephane posted a patchset to add perf_cgroup subsystem, so perf can
>> be used to monitor all threads belonging to a cgroup.
>>
>> But if you already mounted a cgroup hierarchy but without perf_cgroup
>> and the hierarchy has sub-cgroups, you can't bind perf_cgroup to it,
>> and thus you're not able to use per-cgroup perf feature.
>>
>> This patchset alleviates the pain, and then a subsytem can be
>> bound/unbound to/from a hierarchy which has sub-cgroups in it.
>>
>> Some subsystems still can't take advantage of this patchset, memcgroup
>> and cpuset in specific.
>>
>> For cpuset, if a hierarchy has a sub-cgroup and the cgroup has tasks,
>> we can't decide sub-cgroup's cpuset.mems and cpuset.cpus automatically
>> if we try to bind cpuset to this hierarchy.
>>
>> For memcgroup, memcgroup uses css_get/put(), and due to some complexity,
>> for now bindable subsystems should not use css_get/put().
>>
>> Usage:
>>
>> # mount -t cgroup -o cpuset xxx /mnt
>> # mkdir /mnt/tmp
>> # echo $$ > /mnt/tmp/tasks
>>
>> (add cpuacct to the hierarchy)
>> # mount -o remount,cpuset,cpuacct xxx /mnt
>>
>> (remove it from the hierarchy)
>> # mount -o remount,cpuset xxx /mnt
>>
>> There's another limitation, cpuacct should not be bound to any mounted
>> hierarchy before the above operation. But that's not a problem, as you
>> can remove it from a hierarchy and bind it to another one.
> 
> Right, so the only remaining problem I see with this approach is that
> you cannot profile two different hierarchies at the same time, but I
> can't really think of a solution to that problem (nor do I care very
> much).
> 

Paul had a patch to allow some subsystems to be added to multi-hierarchies,
which may help. But it forbids accessing t->cgroups, which makes this
feature of limited use.

Anyway I too don't care much about this.

> Seems like a nice approach, Thanks Li!
> 

Thanks!

Re: [PATCH 0/7] cgroups: Allow to bind/unbind subsystems to/from non-trival hierarchy

[Paul Menage]

On Sun, Oct 24, 2010 at 6:07 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>
> Paul had a patch to allow some subsystems to be added to multi-hierarchies,
> which may help.

I more or less dropped it since I couldn't see a big use for it - all
the examples I had were more easily implemented as basic cgroup
control files (and hence available in all hierarchies automatically)

> But it forbids accessing t->cgroups, which makes this
> feature of limited use.

Not exactly - it meant that you couldn't do a fast dereference via
t->cgroups to get to your cgroup_subsys_state, since there was no
longer a unique css for each task for that subsystem - there would be
one per mounted instance of the subsystem. Instead it became necessary
to follow a chain of pointers to find the appropriate css for the
hierarchy in question.

In general this also means that it's harder for the multi-bound
subsystem have any real-world meaning - it's mostly useful for
stateless subsystems, or ones where the cgroup_subsys_state contents
have no direct connection with machine-wide data.

Paul

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Paul Menage]

On Fri, Oct 22, 2010 at 1:09 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> On x86_32, sizeof(struct cgroup_subsys) shrinks from 276 bytes
> to 264.
>
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Acked-by: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Maybe use "bool" here?

Paul

> ---
> include/linux/cgroup.h |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index ed4ba11..e23ded6 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -481,14 +481,16 @@ struct cgroup_subsys {
>        void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
>
>        int subsys_id;
> -       int active;
> -       int disabled;
> -       int early_init;
> +
> +       unsigned int active:1;
> +       unsigned int disabled:1;
> +       unsigned int early_init:1;
>        /*
>         * True if this subsys uses ID. ID is not available before cgroup_init()
>         * (not available in early_init time.)
>         */
> -       bool use_id;
> +       unsigned int use_id:1;
> +
>  #define MAX_CGROUP_TYPE_NAMELEN 32
>        const char *name;
>
> --
> 1.7.0.1
>
>

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Li Zefan]

(Sorry for the delayed reply)

Paul Menage wrote:
> On Fri, Oct 22, 2010 at 1:09 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> On x86_32, sizeof(struct cgroup_subsys) shrinks from 276 bytes
>> to 264.
>>
>> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> 
> Acked-by: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> Maybe use "bool" here?
> 

Do you mean:

bool active;
bool disabled;
...

?

With alignment 5-8 bool values == 8 bytes in 64-bit machine, compared to
4 bytes with the approach this patch takes.

> Paul
> 
>> ---
>> include/linux/cgroup.h |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index ed4ba11..e23ded6 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -481,14 +481,16 @@ struct cgroup_subsys {
>>        void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
>>
>>        int subsys_id;
>> -       int active;
>> -       int disabled;
>> -       int early_init;
>> +
>> +       unsigned int active:1;
>> +       unsigned int disabled:1;
>> +       unsigned int early_init:1;
>>        /*
>>         * True if this subsys uses ID. ID is not available before cgroup_init()
>>         * (not available in early_init time.)
>>         */
>> -       bool use_id;
>> +       unsigned int use_id:1;
>> +
>>  #define MAX_CGROUP_TYPE_NAMELEN 32
>>        const char *name;
>>
>> --
>> 1.7.0.1
>>
>>

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Paul Menage]

On Sun, Nov 7, 2010 at 9:23 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>
> bool active;
> bool disabled;
> ...
>
> ?
>
> With alignment 5-8 bool values == 8 bytes in 64-bit machine, compared to
> 4 bytes with the approach this patch takes.

I meant specifying it as:

bool active:1;
bool disabled:1;

i.e. keeping the bit-sized flags but also keeping the bool semantics.
Having said that, I'm not really sure why saving 12 bytes per
subsystem is worth a patch.

Paul

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Li Zefan]

Paul Menage wrote:
> On Sun, Nov 7, 2010 at 9:23 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> bool active;
>> bool disabled;
>> ...
>>
>> ?
>>
>> With alignment 5-8 bool values == 8 bytes in 64-bit machine, compared to
>> 4 bytes with the approach this patch takes.
> 
> I meant specifying it as:
> 
> bool active:1;
> bool disabled:1;
> 

It won't compile, but unsigned char active:1 will do. ;)

> i.e. keeping the bit-sized flags but also keeping the bool semantics.
> Having said that, I'm not really sure why saving 12 bytes per
> subsystem is worth a patch.

Every thing that reduces code size (without sacrifice readability
and maintain maintainability) should be worth.

This is one of the reasons we accept patches that replacing
kmalloc+memset with kzalloc, which just saves 8 bytes in my box.

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Paul Menage]

On Tue, Nov 9, 2010 at 4:52 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>>
>> bool active:1;
>> bool disabled:1;
>>
>
> It won't compile, but unsigned char active:1 will do. ;)

Are you sure? I don't have a buildable kernel tree at the moment, but
the following fragment compiled fine for me (with gcc 4.4.3):

struct foo {
  _Bool b1:1;
  _Bool b2:1;
};

and was sized at one byte. And "bool" is just a typedef of _Bool in
the kernel headers.

>
> Every thing that reduces code size (without sacrifice readability
> and maintain maintainability) should be worth.

Agreed, within reason. But this patch doesn't reduce code size - it
makes the code fractionally more complicated and reduces the *binary*
size by a few bytes.

>
> This is one of the reasons we accept patches that replacing
> kmalloc+memset with kzalloc, which just saves 8 bytes in my box.
>

Replacing two function calls with one function call is a code
simplification and hence (generally) a good thing - the minuscule
reduction in binary size reduction that comes with it is just noise.

Paul

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Li Zefan]

Paul Menage wrote:
> On Tue, Nov 9, 2010 at 4:52 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>>> bool active:1;
>>> bool disabled:1;
>>>
>> It won't compile, but unsigned char active:1 will do. ;)
> 
> Are you sure? I don't have a buildable kernel tree at the moment, but
> the following fragment compiled fine for me (with gcc 4.4.3):
> 
> struct foo {
>   _Bool b1:1;
>   _Bool b2:1;
> };
> 
> and was sized at one byte. And "bool" is just a typedef of _Bool in
> the kernel headers.
> 

Oops, I just used bool outside kernel tree..

>> Every thing that reduces code size (without sacrifice readability
>> and maintain maintainability) should be worth.
> 
> Agreed, within reason. But this patch doesn't reduce code size - it

I meant binary size.

> makes the code fractionally more complicated and reduces the *binary*
> size by a few bytes.
> 

It's a commonly used skill in kernel code, so I can't say it makes
code more complicated.

That said, I'll happily drop this patch. It just came to me when I
started to add new bool values to the structure. Or if you prefer
bool xxx:1 or just bool xxx, I can do that.

>> This is one of the reasons we accept patches that replacing
>> kmalloc+memset with kzalloc, which just saves 8 bytes in my box.
>>
> 
> Replacing two function calls with one function call is a code
> simplification and hence (generally) a good thing - the minuscule
> reduction in binary size reduction that comes with it is just noise.
> 

Re: [PATCH 1/7] cgroups: Shrink struct cgroup_subsys

[Paul Menage]

On Tue, Nov 9, 2010 at 6:06 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> That said, I'll happily drop this patch. It just came to me when I
> started to add new bool values to the structure. Or if you prefer
> bool xxx:1 or just bool xxx, I can do that.

bool xxx:1 is fine with me - I think it's worth keeping the semantics
of these being boolean flags as obvious as possible.

I just wouldn't invest much effort in similar patches in the future
(given that there are only likely to be, what, <20 instances of
cgroup_subsys in the kernel?). Shrinking a structure that had
potentially very many instances, such as css_set or cg_cgroup_link,
would be different of course.

Paul

Re: [PATCH 3/7] cgroups: Allow to unbind subsystem from a cgroup hierarachy

[Paul Menage]

On Fri, Oct 22, 2010 at 1:10 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> -               for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
> -                       cg->subsys[i] = cgrp->subsys[i];
> +               for_each_set_bit(i, &bits, CGROUP_SUBSYS_COUNT) {
> +                       if (add)
> +                               cg->subsys[i] = cgrp->subsys[i];
> +                       else
> +                               cg->subsys[i] = dummytop->subsys[i];
> +               }

Should these technically be rcu_assign_pointer() ?

Paul

Re: [PATCH 6/7] cgroups: Warn if a bindable subsystem calls css_get()

[Paul Menage]

Maybe re-title this patch to BUG() if a bindable subsystem calls css_get() ?

Paul

On Fri, Oct 22, 2010 at 1:12 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> For now bindable subsystems should not use css_get/put(), so warn
> on this misuse.
>
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> ---
>  include/linux/cgroup.h |    7 +++++--
>  kernel/cgroup.c        |    3 +++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 1e4e1df..10e4e02 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -80,13 +80,15 @@ struct cgroup_subsys_state {
>
>  /* bits in struct cgroup_subsys_state flags field */
>  enum {
> -       CSS_ROOT, /* This CSS is the root of the subsystem */
> -       CSS_REMOVED, /* This CSS is dead */
> +       CSS_ROOT,       /* This CSS is the root of the subsystem */
> +       CSS_REMOVED,    /* This CSS is dead */
> +       CSS_NO_GET,     /* Forbid calling css_get/put() */
>  };
>
>  /* Caller must verify that the css is not for root cgroup */
>  static inline void __css_get(struct cgroup_subsys_state *css, int count)
>  {
> +       BUG_ON(test_bit(CSS_NO_GET, &css->flags));
>        atomic_add(count, &css->refcnt);
>  }
>
> @@ -119,6 +121,7 @@ static inline bool css_tryget(struct cgroup_subsys_state *css)
>  {
>        if (test_bit(CSS_ROOT, &css->flags))
>                return true;
> +       BUG_ON(test_bit(CSS_NO_GET, &css->flags));
>        while (!atomic_inc_not_zero(&css->refcnt)) {
>                if (test_bit(CSS_REMOVED, &css->flags))
>                        return false;
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 12c1f7c..88e265d 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -936,6 +936,9 @@ static void init_cgroup_css(struct cgroup_subsys_state *css,
>        css->cgroup = cgrp;
>        atomic_set(&css->refcnt, 1);
>        css->flags = 0;
> +       /* For now, a bindable subsystem should not call css_get/put(). */
> +       if (ss->can_bind)
> +               set_bit(CSS_NO_GET, &css->flags);
>        css->id = NULL;
>        if (cgrp == dummytop)
>                set_bit(CSS_ROOT, &css->flags);
> --
> 1.7.0.1
>
>