Skip to contentSkip to conversation

some pending cgroup patches

PATCH Applied Li Zefan 2 participants 22 messages Jul 31, 2013 – Aug 1, 2013 cgroup

lore
Share & tools
lore

Li Zefan

author
#0 lore raw

cover some pending cgroup patches

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(-)
author
#1 lore raw

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

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

---
 kernel/cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/cgroup.c +3 -1
index a64b7b8..e60eba2 100644
@@ -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);
 	}
Signature · 1 line
1.8.0.2
maintainer Applied
#2 lore raw

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

Quoted 3 lines · Li Zefan

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

Applied to cgroup/for-3.11-fixes.

Thanks.

Signature · 1 line
tejun
author
#3 lore raw

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

In reply to Li Zefan

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

While at it, update the comments.

---
 kernel/cgroup.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/cgroup.c +2 -3
index e60eba2..916a699 100644
@@ -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];
 
Signature · 1 line
1.8.0.2
maintainer Applied
#4 lore raw

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

Quoted 6 lines · Li Zefan

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

While at it, update the comments.

Applied to cgroup/for-3.12.

Thanks.

Signature · 1 line
tejun
author
#5 lore raw

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

In reply to 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.

---
 kernel/cgroup.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
kernel/cgroup.c +15 -18
index 916a699..f0d7f5d 100644
@@ -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;
Signature · 1 line
1.8.0.2
maintainer
#6 lore raw

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

Quoted 7 lines · 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.

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.

Signature · 1 line
tejun
author
#7 lore raw

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

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.

---
 kernel/cgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)
kernel/cgroup.c +18 -21
index fb11569..ae905a5 100644
@@ -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;
Signature · 1 line
1.8.0.2
maintainer Applied
#8 lore raw

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

Quoted 10 lines · 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.

Applied to cgroup/for-3.12.

Thanks.

Signature · 1 line
tejun
author
#9 lore raw

PATCH 4/7 cgroup: rename cgroup_pidlist->mutex

In reply to Li Zefan

It's a semaphore not a mutex.

---
 kernel/cgroup.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
kernel/cgroup.c +11 -11
index f0d7f5d..0d378b1 100644
@@ -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)
Signature · 1 line
1.8.0.2
maintainer
#10 lore raw

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

Quoted 1 line · Li Zefan

It's a semaphore not a mutex.

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

Thanks.

Signature · 1 line
tejun
author
#11 lore raw

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

It's a rw_semaphore not a mutex.

---
 kernel/cgroup.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
kernel/cgroup.c +11 -11
index ae905a5..6662811 100644
@@ -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)
Signature · 1 line
1.8.0.2
maintainer Applied
#12 lore raw

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

Quoted 3 lines · Li Zefan

It's a rw_semaphore not a mutex.

Applied to cgroup/for-3.12.

Thanks.

Signature · 1 line
tejun
author
#13 lore raw

PATCH 5/7 cgroup: remove struct cgroup_seqfile_state

In reply to Li Zefan

We can use struct struct cfent instead.

---
 kernel/cgroup.c | 38 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 26 deletions(-)
kernel/cgroup.c +12 -26
index 0d378b1..ac77d87 100644
@@ -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;
 }
Signature · 1 line
1.8.0.2
author
#14 lore raw

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

kernel/cgroup.c static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value) · line 2390

Quoted 6 lines

static int cgroup_seqfile_release(struct inode *inode, struct file *file) {

kernel/cgroup.c +0 -2
-	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...

maintainer
#15 lore raw

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

Quoted 10 lines · Li Zefan

kernel/cgroup.c static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value) · line 2390

Quoted 6 lines

static int cgroup_seqfile_release(struct inode *inode, struct file *file) {

kernel/cgroup.c +0 -2
-	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.

Signature · 1 line
tejun
author
#16 lore raw

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

Quoted 10 lines · Tejun Heo
Quoted 9 lines · Li Zefan

kernel/cgroup.c static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value) · line 2390

Quoted 6 lines

static int cgroup_seqfile_release(struct inode *inode, struct file *file) {

kernel/cgroup.c +0 -2
-	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().

Quoted 6 lines
Quoted 1 line

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.

author
#17 lore raw

PATCH 6/7 cgroup: rename current_css_set_cg_links

In reply to 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.

---
 kernel/cgroup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
kernel/cgroup.c +5 -5
index ac77d87..89d63b0 100644
@@ -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,
 	},
 
 	{
Signature · 1 line
1.8.0.2
maintainer
#18 lore raw

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

Hello,

Quoted 6 lines · 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.

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.

Signature · 1 line
tejun
author
#19 lore raw

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

Quoted 7 lines · Tejun Heo

Hello,

Quoted 4 lines · 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.

Quoted 6 lines
Quoted 2 lines

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

author
#20 lore raw

PATCH 7/7 cgroup: more naming cleanups

In reply to Li Zefan

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

---
 include/linux/cgroup.h |  6 +++---
 kernel/cgroup.c        | 26 +++++++++++++-------------
 kernel/cpuset.c        | 16 ++++++++--------
 3 files changed, 24 insertions(+), 24 deletions(-)
include/linux/cgroup.h +3 -3
index c07e175..ee048ba 100644
@@ -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);
kernel/cgroup.c +13 -13
index 89d63b0..3729f43 100644
@@ -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;
kernel/cpuset.c +8 -8
index e565778..299cd71 100644
@@ -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;
Signature · 1 line
1.8.0.2
maintainer Applied
#21 lore raw

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

Quoted 4 lines · Li Zefan

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

Applied to cgroup/for-3.12.

Thanks.

Signature · 1 line
tejun