Re: [PATCH] nodemask_t x86_64 changes [5/7]

[William Lee Irwin III <wli@holomorphy.com>]

At some point in the past, I wrote:
>> Any idea where this bogon might be?

On Tue, Mar 23, 2004 at 08:11:01PM -0800, Paul Jackson wrote:
> Unless I'm missing something (quite possible), the following would
> expose the complement operators setting of high unused bits.
>     You have 32 bit unsigned longs and NR_CPUS == 24, you complement
>     some cpumask, and you then ask if it is empty, or ask if it is
>     equal to another cpumask that differs only in the unused
>     high byte, or ask its Hamming weight?
> The arithmetic cpumask ops don't mask off the unused bits above NR_CPUS.
> This might never be noticed, because cpumask_complement is the only op
> that sets these unused bits, the complement op is very rarely used, and
> none of its current uses can lead to the above bugs that I see offhand.

It's supposed to do this. bitmap_weight() masks off the trailing bits to
filter out "don't cares" from whole-word ops. Everything else operates
bitwise. Something like the following will keep the bits clear as you
wish, since apparently the more simplistic single-word ops aren't obeying
this. It's likely possible to better this by exploiting some invariants;
feel free improve on it.


-- wli


Index: pgcl-2.6.5-rc2/include/asm-generic/cpumask_arith.h
===================================================================
--- pgcl-2.6.5-rc2.orig/include/asm-generic/cpumask_arith.h	2004-03-19 16:11:33.000000000 -0800
+++ pgcl-2.6.5-rc2/include/asm-generic/cpumask_arith.h	2004-03-23 20:34:54.000000000 -0800
@@ -6,6 +6,12 @@
  * to contain the whole cpu bitmap.
  */
 
+#if NR_CPUS % BITS_PER_LONG
+#define __CPU_MASK_VALID_BITS__		((1UL << (NR_CPUS % BITS_PER_LONG)) - 1)
+#else
+#define __CPU_MASK_VALID_BITS__		(~0UL)
+#endif
+
 #define cpu_set(cpu, map)		set_bit(cpu, &(map))
 #define cpu_clear(cpu, map)		clear_bit(cpu, &(map))
 #define cpu_isset(cpu, map)		test_bit(cpu, &(map))
@@ -14,15 +20,15 @@
 #define cpus_and(dst,src1,src2)		do { dst = (src1) & (src2); } while (0)
 #define cpus_or(dst,src1,src2)		do { dst = (src1) | (src2); } while (0)
 #define cpus_clear(map)			do { map = 0; } while (0)
-#define cpus_complement(map)		do { map = ~(map); } while (0)
-#define cpus_equal(map1, map2)		((map1) == (map2))
-#define cpus_empty(map)			((map) == 0)
+#define cpus_complement(map)		do { map = ~(map) & __CPU_MASK_VALID_BITS__; } while (0)
+#define cpus_equal(map1, map2)		(!(((map1) ^ (map2)) & __CPU_MASK_VALID_BITS__))
+#define cpus_empty(map)			(!((map) & __CPU_MASK_VALID_BITS__))
 #define cpus_addr(map)			(&(map))
 
 #if BITS_PER_LONG == 32
-#define cpus_weight(map)		hweight32(map)
+#define cpus_weight(map)		hweight32((map) & __CPU_MASK_VALID_BITS__)
 #elif BITS_PER_LONG == 64
-#define cpus_weight(map)		hweight64(map)
+#define cpus_weight(map)		hweight64((map) & __CPU_MASK_VALID_BITS__)
 #endif
 
 #define cpus_shift_right(dst, src, n)	do { dst = (src) >> (n); } while (0)
@@ -39,11 +45,13 @@
 #define CPU_MASK_NONE	((cpumask_t)0)
 
 /* only ever use this for things that are _never_ used on large boxen */
-#define cpus_coerce(map)		((unsigned long)(map))
+#define cpus_coerce(map)		((unsigned long)(map) & __CPU_MASK_VALID_BITS__)
 #define cpus_promote(map)		({ map; })
 #define cpumask_of_cpu(cpu)		({ ((cpumask_t)1) << (cpu); })
 
 #define first_cpu(map)			__ffs(map)
 #define next_cpu(cpu, map)		find_next_bit(&(map), NR_CPUS, cpu + 1)
 
+#undef __CPU_MASK_VALID_BITS__
+
 #endif /* __ASM_GENERIC_CPUMASK_ARITH_H */