[PATCH 0/7] some pending cgroup patches

[PATCH 0/7] some pending cgroup patches

[Li Zefan]

The first one is a small bug fix, and the rest are pure cleanups.

Li Zefan (7):
      cgroup: fix a leak when percpu_ref_init() fails
      cgroup: remove sparse tags from offline_css()
      cgroup: restructure the failure path in cgroup_write_event_control()
      cgroup: rename cgroup_pidlist->mutex
      cgroup: remove struct cgroup_seqfile_state
      cgroup: rename current_css_set_cg_links
      cgroup: more naming cleanups
--
 include/linux/cgroup.h |   6 +--
 kernel/cgroup.c        | 138 ++++++++++++++++++++++++++++-----------------------------------
 kernel/cpuset.c        |  16 ++++----
 3 files changed, 72 insertions(+), 88 deletions(-)

[PATCH 1/7] cgroup: fix a leak when percpu_ref_init() fails

[Li Zefan]

ss->css_free() is not called when perfcpu_ref_init() fails.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a64b7b8..e60eba2 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4274,8 +4274,10 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 		}
 
 		err = percpu_ref_init(&css->refcnt, css_release);
-		if (err)
+		if (err) {
+			ss->css_free(cgrp);
 			goto err_free_all;
+		}
 
 		init_cgroup_css(css, ss, cgrp);
 	}
-- 
1.8.0.2

Re: [PATCH 1/7] cgroup: fix a leak when percpu_ref_init() fails

[Tejun Heo]

On Wed, Jul 31, 2013 at 04:16:28PM +0800, Li Zefan wrote:
> ss->css_free() is not called when perfcpu_ref_init() fails.
> 
> Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.11-fixes.

Thanks.

-- 
tejun

[PATCH 2/7] cgroup: remove sparse tags from offline_css()

[Li Zefan]

This should have been removed in commit d7eeac1913ff
("cgroup: hold cgroup_mutex before calling css_offline").

While at it, update the comments.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index e60eba2..916a699 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4164,7 +4164,7 @@ static void init_cgroup_css(struct cgroup_subsys_state *css,
 	INIT_WORK(&css->dput_work, css_dput_fn);
 }
 
-/* invoke ->post_create() on a new CSS and mark it online if successful */
+/* invoke ->css_online() on a new CSS and mark it online if successful */
 static int online_css(struct cgroup_subsys *ss, struct cgroup *cgrp)
 {
 	int ret = 0;
@@ -4178,9 +4178,8 @@ static int online_css(struct cgroup_subsys *ss, struct cgroup *cgrp)
 	return ret;
 }
 
-/* if the CSS is online, invoke ->pre_destory() on it and mark it offline */
+/* if the CSS is online, invoke ->css_offline() on it and mark it offline */
 static void offline_css(struct cgroup_subsys *ss, struct cgroup *cgrp)
-	__releases(&cgroup_mutex) __acquires(&cgroup_mutex)
 {
 	struct cgroup_subsys_state *css = cgrp->subsys[ss->subsys_id];
 
-- 
1.8.0.2

Re: [PATCH 2/7] cgroup: remove sparse tags from offline_css()

[Tejun Heo]

On Wed, Jul 31, 2013 at 04:16:40PM +0800, Li Zefan wrote:
> This should have been removed in commit d7eeac1913ff
> ("cgroup: hold cgroup_mutex before calling css_offline").
> 
> While at it, update the comments.
> 
> Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.12.

Thanks.

-- 
tejun

[PATCH 3/7] cgroup: restructure the failure path in cgroup_write_event_control()

[Li Zefan]

It uses a single label and checks the validity of each pointer. This
is err-prone, and actually we once had a bug because one of the check
was insufficient.

Use multi lables as we do in other places.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 916a699..f0d7f5d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3945,31 +3945,31 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	efile = eventfd_fget(efd);
 	if (IS_ERR(efile)) {
 		ret = PTR_ERR(efile);
-		goto fail;
+		goto out_kfree;
 	}
 
 	event->eventfd = eventfd_ctx_fileget(efile);
 	if (IS_ERR(event->eventfd)) {
 		ret = PTR_ERR(event->eventfd);
-		goto fail;
+		goto out_put_efile;
 	}
 
 	cfile = fget(cfd);
 	if (!cfile) {
 		ret = -EBADF;
-		goto fail;
+		goto out_put_eventfd;
 	}
 
 	/* the process need read permission on control file */
 	/* AV: shouldn't we check that it's been opened for read instead? */
 	ret = inode_permission(file_inode(cfile), MAY_READ);
 	if (ret < 0)
-		goto fail;
+		goto out_put_cfile;
 
 	event->cft = __file_cft(cfile);
 	if (IS_ERR(event->cft)) {
 		ret = PTR_ERR(event->cft);
-		goto fail;
+		goto out_put_cfile;
 	}
 
 	/*
@@ -3979,18 +3979,18 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	cgrp_cfile = __d_cgrp(cfile->f_dentry->d_parent);
 	if (cgrp_cfile != cgrp) {
 		ret = -EINVAL;
-		goto fail;
+		goto out_put_cfile;
 	}
 
 	if (!event->cft->register_event || !event->cft->unregister_event) {
 		ret = -EINVAL;
-		goto fail;
+		goto out_put_cfile;
 	}
 
 	ret = event->cft->register_event(cgrp, event->cft,
 			event->eventfd, buffer);
 	if (ret)
-		goto fail;
+		goto out_put_cfile;
 
 	efile->f_op->poll(efile, &event->pt);
 
@@ -4010,16 +4010,13 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 
 	return 0;
 
-fail:
-	if (cfile)
-		fput(cfile);
-
-	if (event && event->eventfd && !IS_ERR(event->eventfd))
-		eventfd_ctx_put(event->eventfd);
-
-	if (!IS_ERR_OR_NULL(efile))
-		fput(efile);
-
+out_put_cfile:
+	fput(cfile);
+out_put_eventfd:
+	eventfd_ctx_put(event->eventfd);
+out_put_efile:
+	fput(efile);
+out_kfree:
 	kfree(event);
 
 	return ret;
-- 
1.8.0.2

Re: [PATCH 3/7] cgroup: restructure the failure path in cgroup_write_event_control()

[Tejun Heo]

On Wed, Jul 31, 2013 at 04:17:06PM +0800, Li Zefan wrote:
> It uses a single label and checks the validity of each pointer. This
> is err-prone, and actually we once had a bug because one of the check
> was insufficient.
> 
> Use multi lables as we do in other places.
> 
> Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Hmm... I usually prefer mix of the two approaches as both extremes
tend to be pretty ugly when things get complex.  I don't mind the
conversion but can you please drop the no longer unnecessary NULL
assignments on variable definitions?

Thanks.

-- 
tejun

[PATCH v2 3/7] cgroup: restructure the failure path in cgroup_write_event_control()

[Li Zefan]

It uses a single label and checks the validity of each pointer. This
is err-prone, and actually we had a bug because one of the check was
insufficient.

Use multi lables as we do in other places.

v2:
- drop initializations of local variables.

Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index fb11569..ae905a5 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3934,11 +3934,11 @@ static void cgroup_event_ptable_queue_proc(struct file *file,
 static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 				      const char *buffer)
 {
-	struct cgroup_event *event = NULL;
+	struct cgroup_event *event;
 	struct cgroup *cgrp_cfile;
 	unsigned int efd, cfd;
-	struct file *efile = NULL;
-	struct file *cfile = NULL;
+	struct file *efile;
+	struct file *cfile;
 	char *endp;
 	int ret;
 
@@ -3964,31 +3964,31 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	efile = eventfd_fget(efd);
 	if (IS_ERR(efile)) {
 		ret = PTR_ERR(efile);
-		goto fail;
+		goto out_kfree;
 	}
 
 	event->eventfd = eventfd_ctx_fileget(efile);
 	if (IS_ERR(event->eventfd)) {
 		ret = PTR_ERR(event->eventfd);
-		goto fail;
+		goto out_put_efile;
 	}
 
 	cfile = fget(cfd);
 	if (!cfile) {
 		ret = -EBADF;
-		goto fail;
+		goto out_put_eventfd;
 	}
 
 	/* the process need read permission on control file */
 	/* AV: shouldn't we check that it's been opened for read instead? */
 	ret = inode_permission(file_inode(cfile), MAY_READ);
 	if (ret < 0)
-		goto fail;
+		goto out_put_cfile;
 
 	event->cft = __file_cft(cfile);
 	if (IS_ERR(event->cft)) {
 		ret = PTR_ERR(event->cft);
-		goto fail;
+		goto out_put_cfile;
 	}
 
 	/*
@@ -3998,18 +3998,18 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	cgrp_cfile = __d_cgrp(cfile->f_dentry->d_parent);
 	if (cgrp_cfile != cgrp) {
 		ret = -EINVAL;
-		goto fail;
+		goto out_put_cfile;
 	}
 
 	if (!event->cft->register_event || !event->cft->unregister_event) {
 		ret = -EINVAL;
-		goto fail;
+		goto out_put_cfile;
 	}
 
 	ret = event->cft->register_event(cgrp, event->cft,
 			event->eventfd, buffer);
 	if (ret)
-		goto fail;
+		goto out_put_cfile;
 
 	efile->f_op->poll(efile, &event->pt);
 
@@ -4029,16 +4029,13 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 
 	return 0;
 
-fail:
-	if (cfile)
-		fput(cfile);
-
-	if (event && event->eventfd && !IS_ERR(event->eventfd))
-		eventfd_ctx_put(event->eventfd);
-
-	if (!IS_ERR_OR_NULL(efile))
-		fput(efile);
-
+out_put_cfile:
+	fput(cfile);
+out_put_eventfd:
+	eventfd_ctx_put(event->eventfd);
+out_put_efile:
+	fput(efile);
+out_kfree:
 	kfree(event);
 
 	return ret;
-- 
1.8.0.2

Re: [PATCH v2 3/7] cgroup: restructure the failure path in cgroup_write_event_control()

[Tejun Heo]

On Thu, Aug 01, 2013 at 09:51:47AM +0800, Li Zefan wrote:
> It uses a single label and checks the validity of each pointer. This
> is err-prone, and actually we had a bug because one of the check was
> insufficient.
> 
> Use multi lables as we do in other places.
> 
> v2:
> - drop initializations of local variables.
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>

Applied to cgroup/for-3.12.

Thanks.

-- 
tejun

[PATCH 4/7] cgroup: rename cgroup_pidlist->mutex

[Li Zefan]

It's a semaphore not a mutex.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f0d7f5d..0d378b1 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3417,7 +3417,7 @@ struct cgroup_pidlist {
 	/* pointer to the cgroup we belong to, for list removal purposes */
 	struct cgroup *owner;
 	/* protects the other fields */
-	struct rw_semaphore mutex;
+	struct rw_semaphore sem;
 };
 
 /*
@@ -3490,7 +3490,7 @@ static struct cgroup_pidlist *cgroup_pidlist_find(struct cgroup *cgrp,
 	struct pid_namespace *ns = task_active_pid_ns(current);
 
 	/*
-	 * We can't drop the pidlist_mutex before taking the l->mutex in case
+	 * We can't drop the pidlist_mutex before taking the l->sem in case
 	 * the last ref-holder is trying to remove l from the list at the same
 	 * time. Holding the pidlist_mutex precludes somebody taking whichever
 	 * list we find out from under us - compare release_pid_array().
@@ -3499,7 +3499,7 @@ static struct cgroup_pidlist *cgroup_pidlist_find(struct cgroup *cgrp,
 	list_for_each_entry(l, &cgrp->pidlists, links) {
 		if (l->key.type == type && l->key.ns == ns) {
 			/* make sure l doesn't vanish out from under us */
-			down_write(&l->mutex);
+			down_write(&l->sem);
 			mutex_unlock(&cgrp->pidlist_mutex);
 			return l;
 		}
@@ -3510,8 +3510,8 @@ static struct cgroup_pidlist *cgroup_pidlist_find(struct cgroup *cgrp,
 		mutex_unlock(&cgrp->pidlist_mutex);
 		return l;
 	}
-	init_rwsem(&l->mutex);
-	down_write(&l->mutex);
+	init_rwsem(&l->sem);
+	down_write(&l->sem);
 	l->key.type = type;
 	l->key.ns = get_pid_ns(ns);
 	l->owner = cgrp;
@@ -3572,7 +3572,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
 	l->list = array;
 	l->length = length;
 	l->use_count++;
-	up_write(&l->mutex);
+	up_write(&l->sem);
 	*lp = l;
 	return 0;
 }
@@ -3650,7 +3650,7 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 	int index = 0, pid = *pos;
 	int *iter;
 
-	down_read(&l->mutex);
+	down_read(&l->sem);
 	if (pid) {
 		int end = l->length;
 
@@ -3677,7 +3677,7 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 static void cgroup_pidlist_stop(struct seq_file *s, void *v)
 {
 	struct cgroup_pidlist *l = s->private;
-	up_read(&l->mutex);
+	up_read(&l->sem);
 }
 
 static void *cgroup_pidlist_next(struct seq_file *s, void *v, loff_t *pos)
@@ -3723,7 +3723,7 @@ static void cgroup_release_pid_array(struct cgroup_pidlist *l)
 	 * pidlist_mutex, we have to take pidlist_mutex first.
 	 */
 	mutex_lock(&l->owner->pidlist_mutex);
-	down_write(&l->mutex);
+	down_write(&l->sem);
 	BUG_ON(!l->use_count);
 	if (!--l->use_count) {
 		/* we're the last user if refcount is 0; remove and free */
@@ -3731,12 +3731,12 @@ static void cgroup_release_pid_array(struct cgroup_pidlist *l)
 		mutex_unlock(&l->owner->pidlist_mutex);
 		pidlist_free(l->list);
 		put_pid_ns(l->key.ns);
-		up_write(&l->mutex);
+		up_write(&l->sem);
 		kfree(l);
 		return;
 	}
 	mutex_unlock(&l->owner->pidlist_mutex);
-	up_write(&l->mutex);
+	up_write(&l->sem);
 }
 
 static int cgroup_pidlist_release(struct inode *inode, struct file *file)
-- 
1.8.0.2

Re: [PATCH 4/7] cgroup: rename cgroup_pidlist->mutex

[Tejun Heo]

On Wed, Jul 31, 2013 at 04:17:28PM +0800, Li Zefan wrote:
> It's a semaphore not a mutex.

It's not even a normal semaphore.  Maybe naming it rwsem is better?

Thanks.

-- 
tejun

[PATCH v2 4/7] cgroup: rename cgroup_pidlist->mutex

[Li Zefan]

It's a rw_semaphore not a mutex.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ae905a5..6662811 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3436,7 +3436,7 @@ struct cgroup_pidlist {
 	/* pointer to the cgroup we belong to, for list removal purposes */
 	struct cgroup *owner;
 	/* protects the other fields */
-	struct rw_semaphore mutex;
+	struct rw_semaphore rwsem;
 };
 
 /*
@@ -3509,7 +3509,7 @@ static struct cgroup_pidlist *cgroup_pidlist_find(struct cgroup *cgrp,
 	struct pid_namespace *ns = task_active_pid_ns(current);
 
 	/*
-	 * We can't drop the pidlist_mutex before taking the l->mutex in case
+	 * We can't drop the pidlist_mutex before taking the l->rwsem in case
 	 * the last ref-holder is trying to remove l from the list at the same
 	 * time. Holding the pidlist_mutex precludes somebody taking whichever
 	 * list we find out from under us - compare release_pid_array().
@@ -3518,7 +3518,7 @@ static struct cgroup_pidlist *cgroup_pidlist_find(struct cgroup *cgrp,
 	list_for_each_entry(l, &cgrp->pidlists, links) {
 		if (l->key.type == type && l->key.ns == ns) {
 			/* make sure l doesn't vanish out from under us */
-			down_write(&l->mutex);
+			down_write(&l->rwsem);
 			mutex_unlock(&cgrp->pidlist_mutex);
 			return l;
 		}
@@ -3529,8 +3529,8 @@ static struct cgroup_pidlist *cgroup_pidlist_find(struct cgroup *cgrp,
 		mutex_unlock(&cgrp->pidlist_mutex);
 		return l;
 	}
-	init_rwsem(&l->mutex);
-	down_write(&l->mutex);
+	init_rwsem(&l->rwsem);
+	down_write(&l->rwsem);
 	l->key.type = type;
 	l->key.ns = get_pid_ns(ns);
 	l->owner = cgrp;
@@ -3591,7 +3591,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
 	l->list = array;
 	l->length = length;
 	l->use_count++;
-	up_write(&l->mutex);
+	up_write(&l->rwsem);
 	*lp = l;
 	return 0;
 }
@@ -3669,7 +3669,7 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 	int index = 0, pid = *pos;
 	int *iter;
 
-	down_read(&l->mutex);
+	down_read(&l->rwsem);
 	if (pid) {
 		int end = l->length;
 
@@ -3696,7 +3696,7 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 static void cgroup_pidlist_stop(struct seq_file *s, void *v)
 {
 	struct cgroup_pidlist *l = s->private;
-	up_read(&l->mutex);
+	up_read(&l->rwsem);
 }
 
 static void *cgroup_pidlist_next(struct seq_file *s, void *v, loff_t *pos)
@@ -3742,7 +3742,7 @@ static void cgroup_release_pid_array(struct cgroup_pidlist *l)
 	 * pidlist_mutex, we have to take pidlist_mutex first.
 	 */
 	mutex_lock(&l->owner->pidlist_mutex);
-	down_write(&l->mutex);
+	down_write(&l->rwsem);
 	BUG_ON(!l->use_count);
 	if (!--l->use_count) {
 		/* we're the last user if refcount is 0; remove and free */
@@ -3750,12 +3750,12 @@ static void cgroup_release_pid_array(struct cgroup_pidlist *l)
 		mutex_unlock(&l->owner->pidlist_mutex);
 		pidlist_free(l->list);
 		put_pid_ns(l->key.ns);
-		up_write(&l->mutex);
+		up_write(&l->rwsem);
 		kfree(l);
 		return;
 	}
 	mutex_unlock(&l->owner->pidlist_mutex);
-	up_write(&l->mutex);
+	up_write(&l->rwsem);
 }
 
 static int cgroup_pidlist_release(struct inode *inode, struct file *file)
-- 
1.8.0.2

Re: [PATCH v2 4/7] cgroup: rename cgroup_pidlist->mutex

[Tejun Heo]

On Thu, Aug 01, 2013 at 09:52:15AM +0800, Li Zefan wrote:
> It's a rw_semaphore not a mutex.
> 
> Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.12.

Thanks.

-- 
tejun

[PATCH 5/7] cgroup: remove struct cgroup_seqfile_state

[Li Zefan]

We can use struct struct cfent instead.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 0d378b1..ac77d87 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2362,11 +2362,6 @@ static ssize_t cgroup_file_read(struct file *file, char __user *buf,
  * supports string->u64 maps, but can be extended in future.
  */
 
-struct cgroup_seqfile_state {
-	struct cftype *cft;
-	struct cgroup *cgroup;
-};
-
 static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
 {
 	struct seq_file *sf = cb->state;
@@ -2375,22 +2370,22 @@ static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
 
 static int cgroup_seqfile_show(struct seq_file *m, void *arg)
 {
-	struct cgroup_seqfile_state *state = m->private;
-	struct cftype *cft = state->cft;
+	struct cfent *cfe = m->private;
+	struct cftype *cft = cfe->type;
+	struct cgroup *cgrp = __d_cgrp(cfe->dentry->d_parent);
+
 	if (cft->read_map) {
 		struct cgroup_map_cb cb = {
 			.fill = cgroup_map_add,
 			.state = m,
 		};
-		return cft->read_map(state->cgroup, cft, &cb);
+		return cft->read_map(cgrp, cft, &cb);
 	}
-	return cft->read_seq_string(state->cgroup, cft, m);
+	return cft->read_seq_string(cgrp, cft, m);
 }
 
 static int cgroup_seqfile_release(struct inode *inode, struct file *file)
 {
-	struct seq_file *seq = file->private_data;
-	kfree(seq->private);
 	return single_release(inode, file);
 }
 
@@ -2404,30 +2399,21 @@ static const struct file_operations cgroup_seqfile_operations = {
 static int cgroup_file_open(struct inode *inode, struct file *file)
 {
 	int err;
+	struct cfent *cfe;
 	struct cftype *cft;
 
 	err = generic_file_open(inode, file);
 	if (err)
 		return err;
-	cft = __d_cft(file->f_dentry);
+	cfe = __d_cfe(file->f_dentry);
+	cft = cfe->type;
 
 	if (cft->read_map || cft->read_seq_string) {
-		struct cgroup_seqfile_state *state;
-
-		state = kzalloc(sizeof(*state), GFP_USER);
-		if (!state)
-			return -ENOMEM;
-
-		state->cft = cft;
-		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
 		file->f_op = &cgroup_seqfile_operations;
-		err = single_open(file, cgroup_seqfile_show, state);
-		if (err < 0)
-			kfree(state);
-	} else if (cft->open)
+		err = single_open(file, cgroup_seqfile_show, cfe);
+	} else if (cft->open) {
 		err = cft->open(inode, file);
-	else
-		err = 0;
+	}
 
 	return err;
 }
-- 
1.8.0.2

Re: [PATCH 5/7] cgroup: remove struct cgroup_seqfile_state

[Li Zefan]

>  static int cgroup_seqfile_release(struct inode *inode, struct file *file)
>  {
> -	struct seq_file *seq = file->private_data;
> -	kfree(seq->private);
>  	return single_release(inode, file);
>  }

Oh, I just realized now we can remove cgroup_seqfile_release().

Will send out an updated patch...

Re: [PATCH 5/7] cgroup: remove struct cgroup_seqfile_state

[Tejun Heo]

On Wed, Jul 31, 2013 at 05:27:39PM +0800, Li Zefan wrote:
> >  static int cgroup_seqfile_release(struct inode *inode, struct file *file)
> >  {
> > -	struct seq_file *seq = file->private_data;
> > -	kfree(seq->private);
> >  	return single_release(inode, file);
> >  }
> 
> Oh, I just realized now we can remove cgroup_seqfile_release().
> 
> Will send out an updated patch...

Applied the updated patch.  Can you please post updated patches as
replies to the original path from next time?  That makes things easier
to track on my side.

Thanks.

-- 
tejun

Re: [PATCH 5/7] cgroup: remove struct cgroup_seqfile_state

[Li Zefan]

On 2013/7/31 19:35, Tejun Heo wrote:
> On Wed, Jul 31, 2013 at 05:27:39PM +0800, Li Zefan wrote:
>>>  static int cgroup_seqfile_release(struct inode *inode, struct file *file)
>>>  {
>>> -	struct seq_file *seq = file->private_data;
>>> -	kfree(seq->private);
>>>  	return single_release(inode, file);
>>>  }
>>
>> Oh, I just realized now we can remove cgroup_seqfile_release().
>>
>> Will send out an updated patch...
> 
> Applied the updated patch.  Can you please post updated patches as
> replies to the original path from next time?  That makes things easier
> to track on my side.
> 

Sure.

[PATCH 6/7] cgroup: rename current_css_set_cg_links

[Li Zefan]

As we've renamed css_set->cg_links to css_set->cgrp_links (See commit
69d0206c79 "cgroup: bring some sanity to naming around cg_cgroup_link"),
it's better to also rename the debug file.

As the "debug" cgroup subsystem is for debug only and acts as a sample
on how to write a cgroup subsystem, it's fine to change the interface.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ac77d87..89d63b0 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -5312,9 +5312,9 @@ static u64 current_css_set_refcount_read(struct cgroup *cgrp,
 	return count;
 }
 
-static int current_css_set_cg_links_read(struct cgroup *cgrp,
-					 struct cftype *cft,
-					 struct seq_file *seq)
+static int current_css_set_cgrp_links_read(struct cgroup *cgrp,
+					   struct cftype *cft,
+					   struct seq_file *seq)
 {
 	struct cgrp_cset_link *link;
 	struct css_set *cset;
@@ -5387,8 +5387,8 @@ static struct cftype debug_files[] =  {
 	},
 
 	{
-		.name = "current_css_set_cg_links",
-		.read_seq_string = current_css_set_cg_links_read,
+		.name = "current_css_set_cgrp_links",
+		.read_seq_string = current_css_set_cgrp_links_read,
 	},
 
 	{
-- 
1.8.0.2

Re: [PATCH 6/7] cgroup: rename current_css_set_cg_links

[Tejun Heo]

Hello,

On Wed, Jul 31, 2013 at 04:18:07PM +0800, Li Zefan wrote:
> As we've renamed css_set->cg_links to css_set->cgrp_links (See commit
> 69d0206c79 "cgroup: bring some sanity to naming around cg_cgroup_link"),
> it's better to also rename the debug file.
> 
> As the "debug" cgroup subsystem is for debug only and acts as a sample
> on how to write a cgroup subsystem, it's fine to change the interface.

I'd rather not do this.  It doesn't really buy us anything and stuff
like cgrp_links isn't interesting to actual controllers anyway.

Thanks.

-- 
tejun

Re: [PATCH 6/7] cgroup: rename current_css_set_cg_links

[Li Zefan]

On 2013/7/31 19:38, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 31, 2013 at 04:18:07PM +0800, Li Zefan wrote:
>> As we've renamed css_set->cg_links to css_set->cgrp_links (See commit
>> 69d0206c79 "cgroup: bring some sanity to naming around cg_cgroup_link"),
>> it's better to also rename the debug file.
>>
>> As the "debug" cgroup subsystem is for debug only and acts as a sample
>> on how to write a cgroup subsystem, it's fine to change the interface.
> 
> I'd rather not do this.  It doesn't really buy us anything and stuff
> like cgrp_links isn't interesting to actual controllers anyway.
> 

not a big deal

[PATCH 7/7] cgroup: more naming cleanups

[Li Zefan]

Constantly use @cset for css_set varabbles and use @cgrp as cgroup
variables.

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

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c07e175..ee048ba 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -400,8 +400,8 @@ struct cgroup_map_cb {
 
 /* cftype->flags */
 enum {
-	CFTYPE_ONLY_ON_ROOT	= (1 << 0),	/* only create on root cg */
-	CFTYPE_NOT_ON_ROOT	= (1 << 1),	/* don't create on root cg */
+	CFTYPE_ONLY_ON_ROOT	= (1 << 0),	/* only create on root cgrp */
+	CFTYPE_NOT_ON_ROOT	= (1 << 1),	/* don't create on root cgrp */
 	CFTYPE_INSANE		= (1 << 2),	/* don't create if sane_behavior */
 };
 
@@ -519,7 +519,7 @@ struct cftype_set {
 };
 
 struct cgroup_scanner {
-	struct cgroup *cg;
+	struct cgroup *cgrp;
 	int (*test_task)(struct task_struct *p, struct cgroup_scanner *scan);
 	void (*process_task)(struct task_struct *p,
 			struct cgroup_scanner *scan);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 89d63b0..3729f43 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -431,7 +431,7 @@ static inline void put_css_set_taskexit(struct css_set *cset)
  * @new_cgrp: cgroup that's being entered by the task
  * @template: desired set of css pointers in css_set (pre-calculated)
  *
- * Returns true if "cg" matches "old_cg" except for the hierarchy
+ * Returns true if "cset" matches "old_cset" except for the hierarchy
  * which "new_cgrp" belongs to, for which it should match "new_cgrp".
  */
 static bool compare_css_sets(struct css_set *cset,
@@ -1804,7 +1804,7 @@ EXPORT_SYMBOL_GPL(task_cgroup_path_from_hierarchy);
 struct task_and_cgroup {
 	struct task_struct	*task;
 	struct cgroup		*cgrp;
-	struct css_set		*cg;
+	struct css_set		*cset;
 };
 
 struct cgroup_taskset {
@@ -2022,8 +2022,8 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
 
 		tc = flex_array_get(group, i);
 		old_cset = task_css_set(tc->task);
-		tc->cg = find_css_set(old_cset, cgrp);
-		if (!tc->cg) {
+		tc->cset = find_css_set(old_cset, cgrp);
+		if (!tc->cset) {
 			retval = -ENOMEM;
 			goto out_put_css_set_refs;
 		}
@@ -2036,7 +2036,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
 	 */
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
-		cgroup_task_migrate(tc->cgrp, tc->task, tc->cg);
+		cgroup_task_migrate(tc->cgrp, tc->task, tc->cset);
 	}
 	/* nothing is sensitive to fork() after this point. */
 
@@ -2056,9 +2056,9 @@ out_put_css_set_refs:
 	if (retval) {
 		for (i = 0; i < group_size; i++) {
 			tc = flex_array_get(group, i);
-			if (!tc->cg)
+			if (!tc->cset)
 				break;
-			put_css_set(tc->cg);
+			put_css_set(tc->cset);
 		}
 	}
 out_cancel_attach:
@@ -2168,9 +2168,9 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
 
 	mutex_lock(&cgroup_mutex);
 	for_each_active_root(root) {
-		struct cgroup *from_cg = task_cgroup_from_root(from, root);
+		struct cgroup *from_cgrp = task_cgroup_from_root(from, root);
 
-		retval = cgroup_attach_task(from_cg, tsk, false);
+		retval = cgroup_attach_task(from_cgrp, tsk, false);
 		if (retval)
 			break;
 	}
@@ -3275,8 +3275,8 @@ int cgroup_scan_tasks(struct cgroup_scanner *scan)
 	 * guarantees forward progress and that we don't miss any tasks.
 	 */
 	heap->size = 0;
-	cgroup_iter_start(scan->cg, &it);
-	while ((p = cgroup_iter_next(scan->cg, &it))) {
+	cgroup_iter_start(scan->cgrp, &it);
+	while ((p = cgroup_iter_next(scan->cgrp, &it))) {
 		/*
 		 * Only affect tasks that qualify per the caller's callback,
 		 * if he provided one
@@ -3309,7 +3309,7 @@ int cgroup_scan_tasks(struct cgroup_scanner *scan)
 		 * the heap and wasn't inserted
 		 */
 	}
-	cgroup_iter_end(scan->cg, &it);
+	cgroup_iter_end(scan->cgrp, &it);
 
 	if (heap->size) {
 		for (i = 0; i < heap->size; i++) {
@@ -3355,7 +3355,7 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)
 {
 	struct cgroup_scanner scan;
 
-	scan.cg = from;
+	scan.cgrp = from;
 	scan.test_task = NULL; /* select all tasks in cgroup */
 	scan.process_task = cgroup_transfer_one_task;
 	scan.heap = NULL;
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index e565778..299cd71 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -846,7 +846,7 @@ static void cpuset_change_cpumask(struct task_struct *tsk,
 {
 	struct cpuset *cpus_cs;
 
-	cpus_cs = effective_cpumask_cpuset(cgroup_cs(scan->cg));
+	cpus_cs = effective_cpumask_cpuset(cgroup_cs(scan->cgrp));
 	set_cpus_allowed_ptr(tsk, cpus_cs->cpus_allowed);
 }
 
@@ -867,7 +867,7 @@ static void update_tasks_cpumask(struct cpuset *cs, struct ptr_heap *heap)
 {
 	struct cgroup_scanner scan;
 
-	scan.cg = cs->css.cgroup;
+	scan.cgrp = cs->css.cgroup;
 	scan.test_task = NULL;
 	scan.process_task = cpuset_change_cpumask;
 	scan.heap = heap;
@@ -1063,7 +1063,7 @@ static void cpuset_change_task_nodemask(struct task_struct *tsk,
 static void cpuset_change_nodemask(struct task_struct *p,
 				   struct cgroup_scanner *scan)
 {
-	struct cpuset *cs = cgroup_cs(scan->cg);
+	struct cpuset *cs = cgroup_cs(scan->cgrp);
 	struct mm_struct *mm;
 	int migrate;
 	nodemask_t *newmems = scan->data;
@@ -1103,7 +1103,7 @@ static void update_tasks_nodemask(struct cpuset *cs, struct ptr_heap *heap)
 
 	guarantee_online_mems(mems_cs, &newmems);
 
-	scan.cg = cs->css.cgroup;
+	scan.cgrp = cs->css.cgroup;
 	scan.test_task = NULL;
 	scan.process_task = cpuset_change_nodemask;
 	scan.heap = heap;
@@ -1276,7 +1276,7 @@ static int update_relax_domain_level(struct cpuset *cs, s64 val)
 static void cpuset_change_flag(struct task_struct *tsk,
 				struct cgroup_scanner *scan)
 {
-	cpuset_update_task_spread_flag(cgroup_cs(scan->cg), tsk);
+	cpuset_update_task_spread_flag(cgroup_cs(scan->cgrp), tsk);
 }
 
 /*
@@ -1296,7 +1296,7 @@ static void update_tasks_flags(struct cpuset *cs, struct ptr_heap *heap)
 {
 	struct cgroup_scanner scan;
 
-	scan.cg = cs->css.cgroup;
+	scan.cgrp = cs->css.cgroup;
 	scan.test_task = NULL;
 	scan.process_task = cpuset_change_flag;
 	scan.heap = heap;
@@ -1972,7 +1972,7 @@ static int cpuset_css_online(struct cgroup *cgrp)
 	struct cpuset *cs = cgroup_cs(cgrp);
 	struct cpuset *parent = parent_cs(cs);
 	struct cpuset *tmp_cs;
-	struct cgroup *pos_cg;
+	struct cgroup *pos_cgrp;
 
 	if (!parent)
 		return 0;
@@ -2004,7 +2004,7 @@ static int cpuset_css_online(struct cgroup *cgrp)
 	 * (and likewise for mems) to the new cgroup.
 	 */
 	rcu_read_lock();
-	cpuset_for_each_child(tmp_cs, pos_cg, parent) {
+	cpuset_for_each_child(tmp_cs, pos_cgrp, parent) {
 		if (is_mem_exclusive(tmp_cs) || is_cpu_exclusive(tmp_cs)) {
 			rcu_read_unlock();
 			goto out_unlock;
-- 
1.8.0.2

Re: [PATCH 7/7] cgroup: more naming cleanups

[Tejun Heo]

On Wed, Jul 31, 2013 at 04:18:36PM +0800, Li Zefan wrote:
> Constantly use @cset for css_set varabbles and use @cgrp as cgroup
> variables.
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>

Applied to cgroup/for-3.12.

Thanks.

-- 
tejun