Re: [PATCH] Removes extra checking in kernel/cpuset.c
- From: Paul Jackson <pj@xxxxxxx>
- Date: Thu, 31 Jul 2008 14:13:28 -0500
Paul M wrote:
True, but if top_cpuset.cpus_allowed doesn't intersect with
cpu_online_map then maybe we have other problems?
Good point.
... if I did agree to this patch, there are two nits that would have
to be fixed, and the patch resubmitted:
1) The file pathnames should be one level deeper. As stated in
Documentation/applying-patches.txt:
This means that paths to files inside the patch file contain the name of the
kernel source directories it was generated against (or some other directory
names like "a/" and "b/").
This patch has:
--- kernel/cpuset.c.orig 2008-07-31 17:03:34.000000000 +0600
+++ kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600
There needs to be one more level of path, as in:
--- a/kernel/cpuset.c 2008-07-31 17:03:34.000000000 +0600
+++ b/kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600
I prefer to use specific directory names here, reflecting the
actual kernel version used in the development, as in:
--- 2.6.27-rc1-mm1.orig/kernel/cpuset.c 2008-07-31 17:03:34.000000000 +0600
+++ 2.6.27-rc1-mm1/kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600
(though "2.6.27-rc1-mm1" is not the actual version that was
used in composing this patch -- I don't what version was used.)
2) The spacing of the "else" should place it on the same line as the
preceding closing "}" from the preceding "if (...) { ... }", as in:
if (cs) {
while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
cs = cs->parent;
cpus_and(*pmask, cs->cpus_allowed, cpu_online_map);
} else
As submitted, the "else" was on the next line, by itself.
However ... I still don't like the patch all that much. Granted, it
seems to save a few bytes of kernel text space (11 bytes on my x86_64
build), which is always tempting.
My problem with the patch is that it entangles the logic a (slight)
little bit more. This can be seen in the above observation of Paul M,
which is -needed- in the case of the new code to prove that the kernel
can't crash here, but is not needed in the case of the old code to help
prove that.
I work very hard in code to minimize the special cases and conditions
that need to be in affect for each code statement. Doing so makes for
more robust code, that is easier to understand, and less likely to get
broken by some future change.
I am presuming here that this code change was motivated by reading the
source code and noticing that it resulted in some non-essential runtime
checks for "cs" not being NULL. If this proposed change was motivated
by actual performance analysis -- some real life need to optimize this
code path more than I ever imagined it needed to be optimized, then my
bias would change, toward accepting this patch.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@xxxxxxx> 1.940.382.4214
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- References:
- [PATCH] Removes extra checking in kernel/cpuset.c
- From: Rakib Mullick
- Re: [PATCH] Removes extra checking in kernel/cpuset.c
- From: Paul Jackson
- Re: [PATCH] Removes extra checking in kernel/cpuset.c
- From: Paul Menage
- [PATCH] Removes extra checking in kernel/cpuset.c
- Prev by Date: Re: linux-next: Tree for July 30
- Next by Date: Re: linux-next: Tree for July 30
- Previous by thread: Re: [PATCH] Removes extra checking in kernel/cpuset.c
- Next by thread: Re: [PATCH] Removes extra checking in kernel/cpuset.c
- Index(es):
Relevant Pages
|
