Patchwork numactl: fix libnuma on big-endian 64-bit systems

login
register
mail settings
Submitter Arnd Bergmann
Date Dec. 4, 2008, 5:34 p.m.
Message ID <200812041834.45931.arnd@arndb.de>
Download mbox | patch
Permalink /patch/12273/
State Not Applicable
Headers show

Comments

Arnd Bergmann - Dec. 4, 2008, 5:34 p.m.
The read-mask function assumes that it is running in 32-bit mode,
by addressing the bitmask as a series of int values, instead of
longs. This is broken as can easily be reproduced by running numademo
on a bit-endian 64-bit system.

Changing the addressing to use 'long' values fixes the problem.

Reported-by: Mijo Safradin <safradin@de.ibm.com>

---

Note: the set_nodemask_size() function is broken as well, it seems
to always set the nodemask size to "17" with the s2nbits implementation.
The fallback path in there looks correct.
Lee Schermerhorn - Dec. 4, 2008, 5:46 p.m.
On Thu, 2008-12-04 at 18:34 +0100, Arnd Bergmann wrote:
> The read-mask function assumes that it is running in 32-bit mode,
> by addressing the bitmask as a series of int values, instead of
> longs. This is broken as can easily be reproduced by running numademo
> on a bit-endian 64-bit system.
> 
> Changing the addressing to use 'long' values fixes the problem.

Hi, Arnd:

Not sure what you mean here.  If the patch below is a proposed fix [I
don't see a 'Signed-off-by:", but maybe not needed for libnuma
patches?], the description above doesn't match the code.  Looks like
you're changing the addressing FROM 'long' values to use 'int' values so
that the size is compatible between 32- and 64-bits.  Or is that a
reverse patch/diff below?

Lee


> Reported-by: Mijo Safradin <safradin@de.ibm.com>
> 
> ---
> 
> Note: the set_nodemask_size() function is broken as well, it seems
> to always set the nodemask size to "17" with the s2nbits implementation.
> The fallback path in there looks correct.
> 
> --- a/libnuma.c	2008-12-04 14:25:30.000000000 +0100
> +++ b/libnuma.c	2008-11-20 13:40:29.000000000 +0100
> @@ -392,9 +372,9 @@ read_mask(char *s, struct bitmask *bmp)
>  {
>  	char *end = s;
>  	char *prevend;
> -	unsigned long *start = bmp->maskp;
> -	unsigned long *p = start;
> -	unsigned long *q;
> +	unsigned int *start = (unsigned int *)bmp->maskp;
> +	unsigned int *p = start;
> +	unsigned int *q;
>  	unsigned int i;
>  	unsigned int n = 0;
>  
> @@ -431,14 +411,14 @@
>  	}
>  
>  	/* Poor mans fls() */
> -	for(i = sizeof(long) * 8 - 1; i >= 0; i--)
> +	for(i = 31; i >= 0; i--)
>  		if (test_bit(i, start + n))
>  			break;
>  
>  	/*
>  	 * Return the last bit set
>  	 */
> -	return ((sizeof(unsigned long)*8) * n) + i;
> +	return ((sizeof(unsigned int)*8) * n) + i;
>  }
>  
>  /*
Arnd Bergmann - Dec. 4, 2008, 8:49 p.m.
On Thursday 04 December 2008, Lee Schermerhorn wrote:
> On Thu, 2008-12-04 at 18:34 +0100, Arnd Bergmann wrote:
> > The read-mask function assumes that it is running in 32-bit mode,
> > by addressing the bitmask as a series of int values, instead of
> > longs. This is broken as can easily be reproduced by running numademo
> > on a bit-endian 64-bit system.
> > 
> > Changing the addressing to use 'long' values fixes the problem.
> 
> Hi, Arnd:
> 
> Not sure what you mean here.  If the patch below is a proposed fix [I
> don't see a 'Signed-off-by:", but maybe not needed for libnuma
> patches?], the description above doesn't match the code.  Looks like
> you're changing the addressing FROM 'long' values to use 'int' values so
> that the size is compatible between 32- and 64-bits.  Or is that a
> reverse patch/diff below?

Sorry about that, I was in a hurry when sending this one out and attached
the wrong file, the reverse patch is needed indeed. I'll resend with
a proper Signed-off-by.

	Arnd <><

Patch

--- a/libnuma.c	2008-12-04 14:25:30.000000000 +0100
+++ b/libnuma.c	2008-11-20 13:40:29.000000000 +0100
@@ -392,9 +372,9 @@  read_mask(char *s, struct bitmask *bmp)
 {
 	char *end = s;
 	char *prevend;
-	unsigned long *start = bmp->maskp;
-	unsigned long *p = start;
-	unsigned long *q;
+	unsigned int *start = (unsigned int *)bmp->maskp;
+	unsigned int *p = start;
+	unsigned int *q;
 	unsigned int i;
 	unsigned int n = 0;
 
@@ -431,14 +411,14 @@ 
 	}
 
 	/* Poor mans fls() */
-	for(i = sizeof(long) * 8 - 1; i >= 0; i--)
+	for(i = 31; i >= 0; i--)
 		if (test_bit(i, start + n))
 			break;
 
 	/*
 	 * Return the last bit set
 	 */
-	return ((sizeof(unsigned long)*8) * n) + i;
+	return ((sizeof(unsigned int)*8) * n) + i;
 }
 
 /*