Re: [PATCH 1/5] cgroup: ID notification call back

[Paul Menage <menage@google.com>]

On Tue, Aug 24, 2010 at 6:03 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> Hmm. How this pseudo code looks like ? This passes "new id" via
> cgroup->subsys[array] at creation. (Using union will be better, maybe).
>

That's rather ugly. I was thinking of something more like this. (Not
even compiled yet, and the only subsystem updated is cpuset).

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index ed3e92e..063d9f2 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -458,8 +458,7 @@ void cgroup_release_and_wakeup_rmdir(struct
cgroup_subsys_state *css);
  */

 struct cgroup_subsys {
-	struct cgroup_subsys_state *(*create)(struct cgroup_subsys *ss,
-						  struct cgroup *cgrp);
+	int (*create)(struct cgroup_subsys *ss, struct cgroup *cgrp);
 	int (*pre_destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
 	void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
 	int (*can_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
@@ -513,6 +512,12 @@ struct cgroup_subsys {

 	/* should be defined only by modular subsystems */
 	struct module *module;
+
+	/* Total size of the subsystem's CSS object */
+	size_t css_size;
+
+	/* If non-NULL, the CSS to use for the root cgroup */
+	struct cgroup_subsys_state *root_css;
 };

 #define SUBSYS(_x) extern struct cgroup_subsys _x ## _subsys;
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 192f88c..c589a41 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3307,6 +3307,7 @@ static long cgroup_create(struct cgroup *parent,
struct dentry *dentry,
 			     mode_t mode)
 {
 	struct cgroup *cgrp;
+	struct cgroup_subsys_state *new_css[CGROUP_SUBSYS_COUNT] = {};
 	struct cgroupfs_root *root = parent->root;
 	int err = 0;
 	struct cgroup_subsys *ss;
@@ -3325,6 +3326,16 @@ static long cgroup_create(struct cgroup
*parent, struct dentry *dentry,

 	mutex_lock(&cgroup_mutex);

+	for_each_subsys(root, ss) {
+		int id = ss->subsys_id;
+		new_css[id] = kzalloc(ss->css_size, GFP_KERNEL);
+		if (!new_css) {
+			/* Failed to allocate memory */
+			err = -ENOMEM;
+			goto err_destroy;
+		}
+	}
+
 	init_cgroup_housekeeping(cgrp);

 	cgrp->parent = parent;
@@ -3335,19 +3346,19 @@ static long cgroup_create(struct cgroup
*parent, struct dentry *dentry,
 		set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);

 	for_each_subsys(root, ss) {
-		struct cgroup_subsys_state *css = ss->create(ss, cgrp);
-
-		if (IS_ERR(css)) {
-			err = PTR_ERR(css);
-			goto err_destroy;
-		}
-		init_cgroup_css(css, ss, cgrp);
+		int id = ss->subsys_id;
+		init_cgroup_css(new_css[id], ss, cgrp);
 		if (ss->use_id) {
 			err = alloc_css_id(ss, parent, cgrp);
 			if (err)
 				goto err_destroy;
 		}
-		/* At error, ->destroy() callback has to free assigned ID. */
+		err = ss->create(ss, cgrp);
+		if (err) {
+			free_css_id(ss, css->id);
+			goto err_destroy;
+		}
+		new_css[id] = NULL;
 	}

 	cgroup_lock_hierarchy(root);
@@ -3380,7 +3391,10 @@ static long cgroup_create(struct cgroup
*parent, struct dentry *dentry,
  err_destroy:

 	for_each_subsys(root, ss) {
-		if (cgrp->subsys[ss->subsys_id])
+		int id = ss->subsys_id;
+		if (new_css[id])
+			kfree(new_css[id]);
+		else if (cgrp->subsys[id])
 			ss->destroy(ss, cgrp);
 	}

@@ -3607,11 +3621,16 @@ static void __init cgroup_init_subsys(struct
cgroup_subsys *ss)
 	/* Create the top cgroup state for this subsystem */
 	list_add(&ss->sibling, &rootnode.subsys_list);
 	ss->root = &rootnode;
-	css = ss->create(ss, dummytop);
+	if (ss->root_css)
+		css = ss->root_css;
+	else
+		css = kzalloc(ss->css_size, GFP_KERNEL);
 	/* We don't handle early failures gracefully */
-	BUG_ON(IS_ERR(css));
+	BUG_ON(!css);
 	init_cgroup_css(css, ss, dummytop);

+	BUG_ON(ss->create(ss, dummytop));
+
 	/* Update the init_css_set to contain a subsys
 	 * pointer to this state - since the subsystem is
 	 * newly registered, all tasks and hence the
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index b23c097..7720a79 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1871,24 +1871,12 @@ static void cpuset_post_clone(struct cgroup_subsys *ss,
  *	cont:	control group that the new cpuset will be part of
  */

-static struct cgroup_subsys_state *cpuset_create(
-	struct cgroup_subsys *ss,
-	struct cgroup *cont)
+static int cpuset_create(struct cgroup_subsys *ss, struct cgroup *cont)
 {
-	struct cpuset *cs;
-	struct cpuset *parent;
-
-	if (!cont->parent) {
-		return &top_cpuset.css;
-	}
-	parent = cgroup_cs(cont->parent);
-	cs = kmalloc(sizeof(*cs), GFP_KERNEL);
-	if (!cs)
-		return ERR_PTR(-ENOMEM);
-	if (!alloc_cpumask_var(&cs->cpus_allowed, GFP_KERNEL)) {
-		kfree(cs);
-		return ERR_PTR(-ENOMEM);
-	}
+	struct cpuset *cs = cgroup_cs(cont);
+	struct cpuset *parent = cgroup_cs(cont->parent);
+	if (!alloc_cpumask_var(&cs->cpus_allowed, GFP_KERNEL))
+		return -ENOMEM;

 	cs->flags = 0;
 	if (is_spread_page(parent))
@@ -1903,7 +1891,7 @@ static struct cgroup_subsys_state *cpuset_create(

 	cs->parent = parent;
 	number_of_cpusets++;
-	return &cs->css ;
+	return 0;
 }

 /*
@@ -1934,6 +1922,8 @@ struct cgroup_subsys cpuset_subsys = {
 	.post_clone = cpuset_post_clone,
 	.subsys_id = cpuset_subsys_id,
 	.early_init = 1,
+	.css_size = sizeof(struct cpuset),
+	.root_css = &top_cpuset.css;
 };

 /**