Fix cpu iterator on empty bitmask
PATCH 1/1 Fix cpu iterator on empty bitmask
Name: Fix cpumask iterator over empty cpu set
Status: Trivial
Can't use _ffs() without first checking for zero, and if bits beyond NR_CPUS set it'll give bogus results. Use find_first_bit
include/asm-generic/cpumask_arith.h +1 -1
@@ -43,7 +43,7 @@#define cpus_promote (map) ({ map; })#define cpumask_of_cpu (cpu) ({ ((cpumask_t)1) << (cpu); }) -#define first_cpu (map) __ffs(map) +#define first_cpu (map) find_first_bit(&(map), NR_CPUS)#define next_cpu (cpu, map) find_next_bit(&(map), NR_CPUS, cpu + 1)#endif /* __ASM_GENERIC_CPUMASK_ARITH_H */
Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
Quoted 4 lines · Rusty Russell
Name: Fix cpumask iterator over empty cpu set
Status: Trivial
Can't use_ffs()without first checking for zero, and if bits beyondNR_CPUSset it'll give bogus results. Usefind_first_bit
I sent in something equivalent to this along with a number of other fixes (cpus_shift_right() leaking junk bits in in and cpus_weight() and cpus_empty() and cpus_equal() and the like not ignoring tails) and got a NAK since it clashes with something Paul Jackson's doing.
-- wli
Quoted 10 lines · William Lee Irwin III
Quoted 4 lines · Rusty Russell
Name: Fix cpumask iterator over empty cpu set
Status: Trivial
Can't use_ffs()without first checking for zero, and if bits beyondNR_CPUSset it'll give bogus results. Usefind_first_bitI sent in something equivalent to this along with a number of other fixes (
cpus_shift_right()leaking junk bits in in andcpus_weight()andcpus_empty()andcpus_equal()and the like not ignoring tails) and got a NAK since it clashes with something Paul Jackson's doing.
Agreed, I'm pretty sure Paul's work doesn't make this mistake, but this is a trivial patch for a real big which is causing oopses today.
Linus, please apply...
Rusty.
Name: Fix cpumask iterator over empty cpu set
Status: Trivial
Can't use _ffs() without first checking for zero, and if bits beyond NR_CPUS set it'll give bogus results. Use find_first_bit
include/asm-generic/cpumask_arith.h +1 -1
@@ -43,7 +43,7 @@#define cpus_promote (map) ({ map; })#define cpumask_of_cpu (cpu) ({ ((cpumask_t)1) << (cpu); }) -#define first_cpu (map) __ffs(map) +#define first_cpu (map) find_first_bit(&(map), NR_CPUS)#define next_cpu (cpu, map) find_next_bit(&(map), NR_CPUS, cpu + 1)#endif /* __ASM_GENERIC_CPUMASK_ARITH_H */
Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
Quoted 4 lines · Rusty
Agreed, I'm pretty sure Paul's work doesn't make this mistake, but this is a trivial patch for a real big which is causing oopses today.
Linus, please apply...
agreed
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373Quoted 3 lines · Rusty
Quoted 3 lines
Agreed, I'm pretty sure Paul's work doesn't make this mistake, but this is a trivial patch for a real big which is causing oopses today. Linus, please apply...
Quoted 1 line · Paul Jackson
agreed
Eh? Why are you now suddenly changing your tune from when I wrote a fix for this and others just 10 days ago? The remainder of the missing fixes below.
-- wli
include/asm-generic/cpumask_arith.h +11 -7
@@ -15,17 +15,17 @@#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_equal (map1, map2) (!(((map1) ^ (map2)) & CPU_MASK_ALL)) +#define cpus_empty (map) (!((map) & CPU_MASK_ALL))#define cpus_addr (map) (&(map))#if BITS_PER_LONG ==32 -#define cpus_weight (map) hweight32(map) +#define cpus_weight (map) hweight32((map) & CPU_MASK_ALL)#elif BITS_PER_LONG ==64 -#define cpus_weight (map) hweight64(map) +#define cpus_weight (map) hweight64((map) & CPU_MASK_ALL)#endif -#define cpus_shift_right (dst, src, n) do { dst = (src) >> (n); } while (0) +#define cpus_shift_right (dst, src, n) do { dst = ((src) >> (n)) & CPU_MASK_ALL; } while (0)#define cpus_shift_left (dst, src, n) do { dst = (src) << (n); } while (0)#define any_online_cpu (map) \
@@ -39,11 +39,15 @@#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_ALL)#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) +#define first_cpu (map) \ +({ \ + cpumask_t __first_cpu_map__ = (map) & CPU_MASK_ALL; \ + __first_cpu_map__ ? __ffs(__first_cpu_map__) : NR_CPUS; \ +})#endif /* __ASM_GENERIC_CPUMASK_ARITH_H */
Quoted 1 line
Eh? Why are you now suddenly changing your tune from when I wrote a fix
Rusty said that one fix was causing oopses. And he caught me in a weak moment - so I didn't challenge him on that assertion.
Yes I would prefer to limit changes in this code to what's actually breaking other code here and now.
As soon as I can get the latest 2.6.6-rc2-mm2 to build various arch's and merge with this stuff, I will be ready to request Andrew's attention once again.
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373