Re: [PATCH] Removes extra checking in kernel/cpuset.c

Re: [PATCH] Removes extra checking in kernel/cpuset.c

[James Kosin]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

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 (rakib.mullick@gmail.com)
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]

[PATCH] Removes extra checking in kernel/cpuset.c

[Rakib Mullick]

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 (rakib.mullick@gmail.com)

--- 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]));

Re: [PATCH] Removes extra checking in kernel/cpuset.c

[Paul Jackson]

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 <pj@sgi.com> 1.940.382.4214

Re: [PATCH] Removes extra checking in kernel/cpuset.c

[Paul Menage]

On Thu, Jul 31, 2008 at 9:15 AM, Paul Jackson <pj@sgi.com> 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

Re: [PATCH] Removes extra checking in kernel/cpuset.c

[Paul Jackson]

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@sgi.com> 1.940.382.4214