Re: [PATCH] remove unnecessary memmove() in cgroup_path()

Li Zefan
2008-05-22T08:27:37Z

Paul Jackson wrote:
> Lai wrote:
>> 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;

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.

> 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.
>