Hello guys, this patch removes an extra checking over 'cs' in
functions 'guarantee_online_cpus' and 'guarantee_online_mems'.
Thanks.
Signed-off-by: Md.Rakib H. Mullick ([email protected])
--- kernel/cpuset.c.orig 2008-07-31 17:03:34.000000000 +0600
+++ kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600
@@ -282,10 +282,11 @@ static struct file_system_type cpuset_fs
static void guarantee_online_cpus(const struct cpuset *cs, cpumask_t *pmask)
{
- while (cs && !cpus_intersects(cs->cpus_allowed, cpu_online_map))
- cs = cs->parent;
- if (cs)
+ if (cs) {
+ while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
+ cs = cs->parent;
cpus_and(*pmask, cs->cpus_allowed, cpu_online_map);
+ }
else
*pmask = cpu_online_map;
BUG_ON(!cpus_intersects(*pmask, cpu_online_map));
@@ -306,12 +307,13 @@ static void guarantee_online_cpus(const
static void guarantee_online_mems(const struct cpuset *cs, nodemask_t *pmask)
{
- while (cs && !nodes_intersects(cs->mems_allowed,
+ if (cs) {
+ while (!nodes_intersects(cs->mems_allowed,
node_states[N_HIGH_MEMORY]))
- cs = cs->parent;
- if (cs)
+ cs = cs->parent;
nodes_and(*pmask, cs->mems_allowed,
node_states[N_HIGH_MEMORY]);
+ }
else
*pmask = node_states[N_HIGH_MEMORY];
BUG_ON(!nodes_intersects(*pmask, node_states[N_HIGH_MEMORY]));
Rakib wrote:
> Hello guys, this patch removes an extra checking over 'cs' in
> functions 'guarantee_online_cpus' and 'guarantee_online_mems'.
> ...
> - while (cs && !cpus_intersects(cs->cpus_allowed, cpu_online_map))
> - cs = cs->parent;
> - if (cs)
> + if (cs) {
> + while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
> + cs = cs->parent;
I don't think that works - "cs->parent" can be NULL, and will be NULL
if cs is the root cpuset.
So we must check for non-NULL cs each step of the way back up the
cpuset tree.
I must NAQ this patch.
Holler if you think I'm missing something.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.940.382.4214
On Thu, Jul 31, 2008 at 9:15 AM, Paul Jackson <[email protected]> wrote:
> Rakib wrote:
>> Hello guys, this patch removes an extra checking over 'cs' in
>> functions 'guarantee_online_cpus' and 'guarantee_online_mems'.
>> ...
>> - while (cs && !cpus_intersects(cs->cpus_allowed, cpu_online_map))
>> - cs = cs->parent;
>> - if (cs)
>> + if (cs) {
>> + while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
>> + cs = cs->parent;
>
> I don't think that works - "cs->parent" can be NULL, and will be NULL
> if cs is the root cpuset.
True, but if top_cpuset.cpus_allowed doesn't intersect with
cpu_online_map then maybe we have other problems?
Paul
Rakib Mullick wrote:
> Hello guys, this patch removes an extra checking over 'cs' in
> functions 'guarantee_online_cpus' and 'guarantee_online_mems'.
>
> Thanks.
>
> Signed-off-by: Md.Rakib H. Mullick ([email protected])
>
I'd have to NAK this one as well. 2-reasons:
1) Your patch is guaranteed to generate a NULL pointer dereference at
some point. You keep using cs; and assigning cs = cs->parent in the
loop, at some point you may end up at the TOP of the tree where
cs->parent == NULL.
2) The change breaks the actual meaning of the code. Which is
suggesting my the short snippet, find an entry (while loop)... if the
entry is found (if cs test), do this otherwise BUG out. Your code is
saying, if cs is not NULL initially... find someplace then do something.
The function never BUGS out unless someone calls with cs NULL initially.
James Kosin
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 <[email protected]> 1.940.382.4214