Skip to contentSkip to conversation

remove unnecessary memmove() in cgroup_path()

PATCH In review Lai Jiangshan 3 participants 5 messages May 22, 2008 cgroup

lore
Share & tools
lore

Lai Jiangshan

author
#0 lore raw

PATCH 1/1 remove unnecessary memmove() in cgroup_path()

memmove() is unnecessary in cgroup_path(), the following patch will remove it.

Signed-off-by Lai Jiangshan <[email protected]>
include/linux/cgroup.h +1 -1
index e155aa7..6efa4a0 100644
@@ -281,7 +281,7 @@ int cgroup_add_files(struct cgroup *cgrp,
 
 int cgroup_is_removed(const struct cgroup *cgrp);
 
-int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
+int cgroup_path(const struct cgroup *cgrp, char **buf, int buflen);
 
 int cgroup_task_count(const struct cgroup *cgrp);
kernel/cgroup.c +15 -13
index fbc6fc8..db65898 100644
@@ -1151,13 +1151,14 @@ static inline struct cftype *__d_cft(struct dentry *dentry)
 /**
  * cgroup_path - generate the path of a cgroup
  * @cgrp: the cgroup in question
- * @buf: the buffer to write the path into
+ * @buf: *buf is the buffer to write the path into, and it was set
+ *       to the start of the path when return
  * @buflen: the length of the buffer
  *
  * Called with cgroup_mutex held. Writes path of cgroup into buf.
  * Returns 0 on success, -errno on error.
  */
-int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)
+int cgroup_path(const struct cgroup *cgrp, char **buf, int buflen)
 {
 	char *start;
 

@@ -1166,16 +1167,16 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)
 		 * Inactive subsystems have no dentry for their root
 		 * cgroup
 		 */
-		strcpy(buf, "/");
+		strcpy(*buf, "/");
 		return 0;
 	}
 
-	start = buf + buflen;
+	start = *buf + buflen;
 
 	*--start = '\0';
 	for (;;) {
 		int len = cgrp->dentry->d_name.len;
-		if ((start -= len) < buf)
+		if ((start -= len) < *buf)
 			return -ENAMETOOLONG;
 		memcpy(start, cgrp->dentry->d_name.name, len);
 		cgrp = cgrp->parent;

@@ -1183,11 +1184,11 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)
 			break;
 		if (!cgrp->parent)
 			continue;
-		if (--start < buf)
+		if (--start < *buf)
 			return -ENAMETOOLONG;
 		*start = '/';
 	}
-	memmove(buf, start, buf + buflen - start);
+	*buf = start;
 	return 0;
 }
 

@@ -2637,6 +2638,7 @@ static int proc_cgroup_show(struct seq_file *m, void *v)
 		struct cgroup *cgrp;
 		int subsys_id;
 		int count = 0;
+		char *path = buf;
 
 		/* Skip this hierarchy if it has no active subsystems */
 		if (!root->actual_subsys_bits)

@@ -2647,10 +2649,10 @@ static int proc_cgroup_show(struct seq_file *m, void *v)
 		seq_putc(m, ':');
 		get_first_subsys(&root->top_cgroup, NULL, &subsys_id);
 		cgrp = task_cgroup(tsk, subsys_id);
-		retval = cgroup_path(cgrp, buf, PAGE_SIZE);
+		retval = cgroup_path(cgrp, &path, PAGE_SIZE);
 		if (retval < 0)
 			goto out_unlock;
-		seq_puts(m, buf);
+		seq_puts(m, path);
 		seq_putc(m, '\n');
 	}
 

@@ -3078,19 +3080,19 @@ static void cgroup_release_agent(struct work_struct *work)
 	while (!list_empty(&release_list)) {
 		char *argv[3], *envp[3];
 		int i;
-		char *pathbuf;
+		char *pathbuf, *path;
 		struct cgroup *cgrp = list_entry(release_list.next,
 						    struct cgroup,
 						    release_list);
 		list_del_init(&cgrp->release_list);
 		spin_unlock(&release_list_lock);
-		pathbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+		pathbuf = path = kmalloc(PAGE_SIZE, GFP_KERNEL);
 		if (!pathbuf) {
 			spin_lock(&release_list_lock);
 			continue;
 		}
 
-		if (cgroup_path(cgrp, pathbuf, PAGE_SIZE) < 0) {
+		if (cgroup_path(cgrp, &path, PAGE_SIZE) < 0) {
 			kfree(pathbuf);
 			spin_lock(&release_list_lock);
 			continue;

@@ -3098,7 +3100,7 @@ static void cgroup_release_agent(struct work_struct *work)
 
 		i = 0;
 		argv[i++] = cgrp->root->release_agent_path;
-		argv[i++] = (char *)pathbuf;
+		argv[i++] = path;
 		argv[i] = NULL;
 
 		i = 0;
kernel/cpuset.c +4 -4
index 86ea9e3..59ff9cb 100644
@@ -2290,12 +2290,12 @@ static int proc_cpuset_show(struct seq_file *m, void *unused_v)
 {
 	struct pid *pid;
 	struct task_struct *tsk;
-	char *buf;
+	char *buf, *path;
 	struct cgroup_subsys_state *css;
 	int retval;
 
 	retval = -ENOMEM;
-	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	buf = path = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!buf)
 		goto out;
 

@@ -2308,10 +2308,10 @@ static int proc_cpuset_show(struct seq_file *m, void *unused_v)
 	retval = -EINVAL;
 	cgroup_lock();
 	css = task_subsys_state(tsk, cpuset_subsys_id);
-	retval = cgroup_path(css->cgroup, buf, PAGE_SIZE);
+	retval = cgroup_path(css->cgroup, &path, PAGE_SIZE);
 	if (retval < 0)
 		goto out_unlock;
-	seq_puts(m, buf);
+	seq_puts(m, path);
 	seq_putc(m, '\n');
 out_unlock:
 	cgroup_unlock();
kernel/sched_debug.c +6 -4
index 5f06118..4fe01b2 100644
@@ -78,9 +78,10 @@ print_task(struct seq_file *m, struct rq *rq, struct task_struct *p)
 
 #ifdef CONFIG_CGROUP_SCHED
 	{
-		char path[64];
+		char buf[64];
+		char *path = buf;
 
-		cgroup_path(task_group(p)->css.cgroup, path, sizeof(path));
+		cgroup_path(task_group(p)->css.cgroup, &path, sizeof(buf));
 		SEQ_printf(m, " %s", path);
 	}
 #endif

@@ -122,7 +123,8 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
 #if !defined(CONFIG_CGROUP_SCHED) || !defined(CONFIG_USER_SCHED)
 	SEQ_printf(m, "\ncfs_rq[%d]:\n", cpu);
 #else
-	char path[128] = "";
+	char buf[128] = "";
+	char *path = buf;
 	struct cgroup *cgroup = NULL;
 	struct task_group *tg = cfs_rq->tg;
 

@@ -130,7 +132,7 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
 		cgroup = tg->css.cgroup;
 
 	if (cgroup)
-		cgroup_path(cgroup, path, sizeof(path));
+		cgroup_path(cgroup, &path, sizeof(buf));
 
 	SEQ_printf(m, "\ncfs_rq[%d]:%s\n", cpu, path);
 #endif
#1 lore raw
Quoted 1 line · Lai

memmove() is unnecessary in cgroup_path(), the following patch will remove it.

True, I think -- memmove() can be removed as you suggest.

However, it makes the code a little harder to read, in my opinion, because the meaning of the "@buf" parameter passed into cgroup_path() is no longer quite the same as the meaning of that same parameter, upon return from cgroup_path().

I have a fairly consistent preference for code clarity, even if it means an occassional extra bit of code gets executed, unless we're on some code path where the performance gained from the tighter code is important. I don't think that cgroup_path() is on such a path; however I could be wrong on that point. Did you discover this non- essential call to memmove() by code reading, or by observing that it was causing some noticeable performance loss for some situation that you care about?

If we did go with this patch as you suggest, then I would like to suggest that we elaborate your explanation of what the "@buf" parameter to cgroup_path() means.

Your patch states:

+ * @buf: *buf is the buffer to write the path into, and it was set
+ * to the start of the path when return

I would suggest stating instead something like:

* @buf: On entry, @buf is a pointer to a pointer to a buffer of

  *       length @buflen into which the path will be written.  In most
  *       cases, excepting some trivial cases such as returning "/",
  *       the path will be written into the -high- end of the buffer,
  *       and the pointer to which buf points will be updated on
  *       return from cgroup_path() to point to the beginning of that
  *       path, somewhere within the original passed in buffer.

One more minor suggestion ... your patch has:
- char *pathbuf;
+ char *pathbuf, *path;

and later on it has:
- char *buf;
+ char *buf, *path;

Could you use the same variable names, when referring to the same things, in both places? It makes the code a little easier to read.

Overall, however, I am not sure I like this patch, unless you have good performance reasons to get rid of that memmove(). The complications to what the "@buf" parameter to cgroup_path() means just aren't worth it, in my current opinion.

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
#2 lore raw

One more question ... how does the kernel text size (size vmlinux) change with this patch?

It may well be that the kernel gets a few bytes -larger-, rather than smaller, due to the extra instructions involved in manipulating the more indirect buffer pointer and in tracking the value of that pointer on cgroup_path() entry separately from the value on return.

If that's the case, then the net performance impact of this patch might be negative, depending on how one measures it. Given the increasingly high costs of cache misses on modern CPU architectures, it is often better to execute a few additional instructions that are likely already in the cache (as 'memmove()' might be) than to increase the kernel text size with rarely executed code lines that will (1) miss the cache more often, even though (2) they consume fewer CPU cycles to execute.

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
#3 lore raw
In reply to Paul Jackson
Quoted 10 lines · Paul Jackson
Quoted 1 line · Lai

memmove() is unnecessary in cgroup_path(), the following patch will remove it.

True, I think -- memmove() can be removed as you suggest.

However, it makes the code a little harder to read, in my opinion, because the meaning of the "@buf" parameter passed into cgroup_path() is no longer quite the same as the meaning of that same parameter, upon return from cgroup_path().

Quoted 4 lines

I have a fairly consistent preference for code clarity, even if it means an occassional extra bit of code gets executed, unless we're on some code path where the performance gained from the tighter code is important. I don't think that cgroup_path() is on such a path;

Nor do I. cgroup_path() will be executed in 2 places. One is when you cat /proc/<pid>/cgroup or a few other cgroup-related files in /proc, one is when run the release agent. So I don't think the patch will improve performance.

Quoted 41 lines

however I could be wrong on that point. Did you discover this non- essential call to memmove() by code reading, or by observing that it was causing some noticeable performance loss for some situation that you care about?

If we did go with this patch as you suggest, then I would like to suggest that we elaborate your explanation of what the "@buf" parameter to cgroup_path() means.

Your patch states:

+ * @buf: *buf is the buffer to write the path into, and it was set
+ * to the start of the path when return

I would suggest stating instead something like:

* @buf: On entry, @buf is a pointer to a pointer to a buffer of

  *       length @buflen into which the path will be written.  In most
  *       cases, excepting some trivial cases such as returning "/",
  *       the path will be written into the -high- end of the buffer,
  *       and the pointer to which buf points will be updated on
  *       return from cgroup_path() to point to the beginning of that
  *       path, somewhere within the original passed in buffer.

One more minor suggestion ... your patch has:

kernel/cgroup.c +1 -1
-		char *pathbuf;
+		char *pathbuf, *path;

and later on it has:

kernel/cpuset.c +1 -1
-	char *buf;
+	char *buf, *path;

Could you use the same variable names, when referring to the same things, in both places? It makes the code a little easier to read.

Overall, however, I am not sure I like this patch, unless you have good performance reasons to get rid of that memmove(). The complications to what the "@buf" parameter to cgroup_path() means just aren't worth it, in my current opinion.

#4 lore raw
In reply to Lai Jiangshan

Paul Menage -- since you are the MAINTAINER of kernel/cgroup.c, I should have CC'd you on my responses to this patch, sent out earlier this evening. My apologies for not doing so.

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214