cpumask: force nr_cpumask_bit to NR_CPUS for CPUMASK_OFFSTACK=n

PATCH In review Mike Travis 2 participants 3 messages Dec 11, 2008 – Dec 13, 2008 3 messages not in the archive

Subsystems (2)

Linux-next tree Bitmap API

lore

Share & tools

lore

Mike Travis

author

2008-12-11 20:44 UTC #3 lore raw

PATCH 1/1 cpumask: force nr_cpumask_bit to NR_CPUS for CPUMASK_OFFSTACK=n

In reply to a message not in the archive

Re: cpumask conversions, or not?

Quoted 16 lines · Rusty Russell

Quoted 8 lines · On Tuesday 09 December 2008 21:26:36 Mike Travis wrote:
Quoted 7 lines · Rusty Russell
Hi all,
The new cpumask conversions are going well, but unfortunately Stephen
uncovered a nasty bug via linux-next: the new cpumask operators only go to nr_cpumask_bits which can be less than NR_CPUS if NR_CPUS > BITS_PER_LONG. The undefined bits confuse the old cpumask operators. We fixed one case, but I am concerned that we will break archs as we convert more core code.
Quoted 6 lines
Hi Rusty,
I think we can avoid this problem if we make cpumask_bits == NR_CPUS iff CONFIG_CPUMASK_OFFSTACK=n. This complies with the current cpumask_t approach and should cause all cpumask operators to always operate on all cpumask bits.

Quoted 4 lines

A very good point. And it's no worse than the old method.
OK, forget about this for now, no urgent conversions needed :) Rusty.

This probably should be submitted through linux-next for wider test coverage?

Thanks,
Mike
---
cpumask: force nr_cpumask_bits to be NR_CPUS when CPUMASK_OFFSTACK is false

This maintains compatibility with the current cpumask_t. Once an architecture is "cpumask clean" [IOW, all references span 0..(nr_cpus_ids-1) only, ignoring any bits >= nr_cpu_ids], then it can set CPUMASK_OFFSTACK=y.

Signed-off-by Mike Travis <[email protected]>

---
 include/linux/cpumask.h |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

include/linux/cpumask.h +12 -4

@@ -510,9 +510,6 @@ extern cpumask_t cpu_active_map;
 	[BITS_TO_LONGS(NR_CPUS)-1] = CPU_MASK_LAST_WORD	\
 }
 
-/* This produces more efficient code. */
-#define nr_cpumask_bits	NR_CPUS
-
 #else /* NR_CPUS > BITS_PER_LONG */
 
 #define CPU_BITS_ALL						\

@@ -521,9 +518,20 @@ extern cpumask_t cpu_active_map;
 	[BITS_TO_LONGS(NR_CPUS)-1] = CPU_MASK_LAST_WORD		\
 }
 
-#define nr_cpumask_bits	nr_cpu_ids
 #endif /* NR_CPUS > BITS_PER_LONG */
 
+#ifndef CONFIG_CPUMASK_OFFSTACK
+
+/* This produces more efficient code. */
+#define nr_cpumask_bits	NR_CPUS
+
+#else
+
+/* This allows for variabled-sized cpumask's */
+#define nr_cpumask_bits	nr_cpu_ids
+
+#endif
+
 /* verify cpu argument to cpumask_* operators */
 static inline unsigned int cpumask_check(unsigned int cpu)
 {

Mike Travis

author

2008-12-11 23:10 UTC #4 lore raw

Quoted 63 lines · Mike Travis

Re: cpumask conversions, or not?
Quoted 11 lines · Rusty Russell
Quoted 10 lines · On Tuesday 09 December 2008 21:26:36 Mike Travis wrote:
Quoted 7 lines · Rusty Russell
Hi all,
The new cpumask conversions are going well, but unfortunately Stephen
uncovered a nasty bug via linux-next: the new cpumask operators only go to nr_cpumask_bits which can be less than NR_CPUS if NR_CPUS > BITS_PER_LONG. The undefined bits confuse the old cpumask operators. We fixed one case, but I am concerned that we will break archs as we convert more core code.
Hi Rusty,
Quoted 8 lines
Quoted 4 lines
I think we can avoid this problem if we make cpumask_bits == NR_CPUS iff CONFIG_CPUMASK_OFFSTACK=n. This complies with the current cpumask_t approach and should cause all cpumask operators to always operate on all cpumask bits.
A very good point. And it's no worse than the old method.
OK, forget about this for now, no urgent conversions needed :) Rusty.
This probably should be submitted through linux-next for wider test coverage?
Thanks,
Mike
---
cpumask: force nr_cpumask_bits to be NR_CPUS when CPUMASK_OFFSTACK is false
This maintains compatibility with the current cpumask_t. Once an architecture is "cpumask clean" [IOW, all references span 0..(nr_cpus_ids-1) only, ignoring any bits >= nr_cpu_ids], then it can set CPUMASK_OFFSTACK=y.
Signed-off-by Mike Travis <[email protected]>
---
 include/linux/cpumask.h |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
include/linux/cpumask.h +5 -4
@@ -510,9 +510,6 @@ extern cpumask_t cpu_active_map;
 	[BITS_TO_LONGS(NR_CPUS)-1] = CPU_MASK_LAST_WORD	\
 }
 
-/* This produces more efficient code. */
-#define nr_cpumask_bits	NR_CPUS
-
 #else /* NR_CPUS > BITS_PER_LONG */
 
 #define CPU_BITS_ALL						\
@@ -521,9 +518,20 @@ extern cpumask_t cpu_active_map;
 	[BITS_TO_LONGS(NR_CPUS)-1] = CPU_MASK_LAST_WORD		\
 }
 
-#define nr_cpumask_bits	nr_cpu_ids
 #endif /* NR_CPUS > BITS_PER_LONG */
 
+#ifndef CONFIG_CPUMASK_OFFSTACK
+
+/* This produces more efficient code. */
+#define nr_cpumask_bits	NR_CPUS
+

include/linux/cpumask.h extern cpumask_t cpu_active_map; · line 537

Quoted 10 lines

include/linux/cpumask.h +7 -0

+#else
+
+/* This allows for variabled-sized cpumask's */
+#define nr_cpumask_bits	nr_cpu_ids
+
+#endif
+
 /* verify cpu argument to cpumask_* operators */
 static inline unsigned int cpumask_check(unsigned int cpu)
 {

I wonder if we should not jump immediately to "variable-sized" cpumasks, even for CPUMASK_OFFSTACK=y? That way we could reap the benefits of allocating the cpumask's on demand, without the restriction that all code that touches the cpumask is "cpumask clean"? In other words, perhaps this would be the right approach:

#ifdef CONFIG_ARCH_HAS_VARIABLE_CPUMASKS
#define nr_cpumask_bits nr_cpu_ids
#else
#define nr_cpumask_bits CONFIG_NR_CPUS
#endif

static inline size_t cpumask_size(void)

{
	return BITS_TO_LONG(nr_cpumask_bits) * sizeof(LONG);
}

Then when the code is verified that the cpumasks are handled correctly by all functions that reference these cpumasks, then ARCH_HAS_VARIABLE_CPUMASKS can be set?

Thanks,
Mike

Rusty Russell

2008-12-13 09:55 UTC #5 lore raw

In reply to Mike Travis

Quoted 19 lines · On Friday 12 December 2008 07:14:09 Mike Travis wrote:

Re: cpumask conversions, or not?
Quoted 16 lines · Rusty Russell
Quoted 14 lines · On Tuesday 09 December 2008 21:26:36 Mike Travis wrote:
Quoted 7 lines · Rusty Russell
Hi all,
The new cpumask conversions are going well, but unfortunately Stephen
uncovered a nasty bug via linux-next: the new cpumask operators only go to nr_cpumask_bits which can be less than NR_CPUS if NR_CPUS > BITS_PER_LONG. The undefined bits confuse the old cpumask operators. We fixed one case, but I am concerned that we will break archs as we convert more core code.
Hi Rusty,
I think we can avoid this problem if we make cpumask_bits == NR_CPUS iff CONFIG_CPUMASK_OFFSTACK=n. This complies with the current cpumask_t approach and should cause all cpumask operators to always operate on all cpumask bits.

Quoted 6 lines

Quoted 4 lines
A very good point. And it's no worse than the old method.
OK, forget about this for now, no urgent conversions needed :) Rusty.
This probably should be submitted through linux-next for wider test coverage?

Identical patch already in series.

Thanks,
Rusty.