Re: [RFC] CPUMASK: proposal for replacing cpumask_t

[Peter Zijlstra <peterz@infradead.org>]

On Wed, 2008-09-10 at 15:47 -0700, Mike Travis wrote:
> Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> >>>> NAK
> ....
> > 
> > seconded. Mike, since none of this is v2.6.27 material, lets do it right 
> > with a v2.6.28 target. You know all the cpumask_t using code sites 
> > inside out already, so the know-how is all available already :-) Please 
> > make it finegrained series of patches so that we can resolve conflicts 
> > with other trees more easily.
> > 
> > perhaps propose the new cpumask_t API early (in this thread?), so that 
> > people can comment on it before NAKs come flying against a full patchset 
> > ;-)
> > 
> > 	Ingo
> 
> 
> Here's an initial proposal for abstracting cpumask_t to be either
> an array of 1 or a pointer to an array...   Hopefully this will
> minimize the amount of code changes while providing the capabilities
> this change is attempting to do.
> 
> Comments most welcome. ;-)
> 
> Thanks,
> Mike
> --
> 
> Basically, linux/cpumask.h has the following defines:
> 
> 	typedef struct { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_data;
> 
> 	#if NR_CPUS > BITS_PER_LONG
> 	typedef const cpumask_data	*cpumask_t;
> 	typedef cpumask_data		*cpumask_var;
> 	typedef cpumask_t		cpumask_val;
> 	#define	_NR_CPUS		nr_cpu_ids
> 	#else
> 	typedef const cpumask_data	cpumask_t[1];
> 	typedef cpumask_data		cpumask_var[1];
> 	typedef cpumask_data		cpumask_val;
> 	#define	_NR_CPUS		NR_CPUS
> 	#endif
> 
> So in function prototypes:
> 
> 	cpumask_t function(const cpumask_t *A,
> 			   cpumask_t *B,
> 			   cpumask_t cpumask_C)
> 
> becomes:
> 
> 	cpumask_val function(cpumask_t A,
> 			     cpumask_var B,
> 			     cpumask_t cpumask_C)

I guess we have to stick the const into the typedef otherwise we get a
const pointer instead of a const array member, right?

In which case I much prefer the following names:

 cpumask_data_t  - value

 const_cpumask_t - pointer to constant value
 cpumask_t       - pointer to value

> And in local variables:
> 
> 	cpumask_t ==> cpumask_var  IFF variable is to be written.
> 
> This:
> 
> 	cpumask_t mask = cpu_online_map 
> 	<change mask>
> 
> becomes:
> 
> 	#include <linux/cpumask_alloc.h>
> 
> 	cpumask_var mask;
> 
> 	alloc_cpumask(&mask);

Don't you have to deal with allocation errors?

> 	*mask = *cpu_online_map;
> 	<change mask>
> 	free_cpumask(&mask);
> 
> Currently, alloc_cpumask is:
> 
> 	#define BYTES_PER_CPUMASK (BITS_TO_LONGS(nr_cpu_ids)/sizeof(long))
> 
> 	static inline bool no_cpumask(cpumask_t *m)
> 	{
> 		return (*m == NULL);
> 	}
> 
> 	static inline void alloc_cpumask(cpumask_t *m)
> 	{
> 		cpumask_t d = kmalloc(BYTES_PER_CPUMASK, GFP_KERNEL);
> 		if (no_cpumask(&d))
> 			BUG();

yuckery yuck yuck!

> 		*m = d;
> 	}
> 
> 	static inline void alloc_cpumask_nopanic(cpumask_t *m)
> 	{
> 		cpumask_t d = kmalloc(BYTES_PER_CPUMASK, GFP_KERNEL);
> 
> 		*m = d;
> 	}

gah - at the very least you got the naming wrong, methinks the one
panic-ing should have panic in its name - if you really want to persist
with that variant.

> 	static inline void free_cpumask(cpumask_t *m)
> 	{
> 		kfree(*m);
> 	}
> 
> Other means of obtaining a temporary cpumask_t variable will be provided
> for those cases where kmalloc() is not available.
> 
> Furthermore, system-wide maps become:
> 
> 	extern cpumask_data _cpu_possible_map;			 /* read/write */
> 	extern cpumask_data _cpu_online_map;
> 	extern cpumask_data _cpu_present_map;
> 	extern cpumask_data _cpu_active_map;
> 	#define	cpu_possible_map ((cpumask_t)&_cpu_possible_map) /* read only */
> 	#define	cpu_online_map ((cpumask_t)&_cpu_online_map)
> 	#define	cpu_present_map ((cpumask_t)&_cpu_present_map)
> 	#define	cpu_active_map ((cpumask_t)&_cpu_active_map)
> 
> 
> So code to set these bits would be:
> 
> 	cpu_set(cpu, &_cpu_online_map);
> 	cpu_set(cpu, &_cpu_present_map);
> 	cpu_set(cpu, &_cpu_possible_map);
> 
> Arrays that contain a fixed cpumask would have:
> 
> 	struct xxx {
> 		cpumask_data	cpumask;
> 	};
> 
> .... though we should probably encourage the map to be allocated:
> 
> 	struct xxx {
> 		cpumask_t	readonly_cpumask;
> 		cpumask_var	readwrite_cpumask;
> 	};
> 
> 	alloc_cpumask(&xxx->readonly_cpumask);
> 	alloc_cpumask(&xxx->readwrite_cpumask);
> 
> 
> All the cpu operators become:
> 
> 	#define cpu_XXX(dst, src) _cpu_XXX(dst, src, _NR_CPUS)
> 	static inline void __cpu_XXX(cpumask_var dstp, cpumask_t srcp, int count)
> 	{
> 		XXX_bit(dstp->bits, srcp->bits, count);
> 	}
> 
> (_NR_CPUS being defined to be nr_cpu_ids allows us to allocate variable lengthed arrays.)
> 
> 
> Cpumask initializers become:
> 
> 
> 	#if NR_CPUS <= BITS_PER_LONG
> 
> 	#define INIT_CPU_MASK_ALL					\
> 	(cpumask_t) { {							\
> 		[BITS_TO_LONGS(NR_CPUS)-1] = CPU_MASK_LAST_WORD		\
> 	} }
> 
> 	#else
> 	#define INIT_CPU_MASK_ALL					\
> 	(cpumask_data) { {						\
> 		[0 ... BITS_TO_LONGS(NR_CPUS)-2] = ~0UL,		\
> 		[BITS_TO_LONGS(NR_CPUS)-1] = CPU_MASK_LAST_WORD		\
> 	} }
> 	#endif
> 
> 	#define INIT_CPU_MASK_NONE					\
> 	(cpumask_data) { {						\
> 		[0 ... BITS_TO_LONGS(NR_CPUS)-1] =  0UL			\
> 	} }
> 
> 	#define INIT_CPU_MASK_CPU0					\
> 	(cpumask_data) { {						\
> 		[0] =  1UL						\
> 	} }
> 
> 	#if NR_CPUS > BITS_PER_LONG
> 	extern cpumask_t cpu_mask_all, cpu_mask_none, cpu_mask_cpu0;
> 	#define CPU_MASK_ALL		(cpu_mask_all)
> 	#define CPU_MASK_NONE		(cpu_mask_none)
> 	#define CPU_MASK_CPU0		(cpu_mask_cpu0)
> 	#else
> 	#define CPU_MASK_ALL		((cpumask_t)&INIT_CPU_MASK_ALL)
> 	#define CPU_MASK_NONE		((cpumask_t)&INIT_CPU_MASK_NONE)
> 	#define CPU_MASK_CPU0		((cpumask_t)&INIT_CPU_MASK_CPU0)
> 	#endif
> 
> And in kernel/cpu.c:
> 
> 	/*
> 	 * provide const cpumask_t's
> 	 */
> 	#if NR_CPUS > BITS_PER_LONG
> 	cpumask_data cpu_mask_all __read_mostly = INIT_CPU_MASK_ALL;
> 	EXPORT_SYMBOL(cpu_mask_all);
> 
> 	cpumask_data cpu_mask_none __read_mostly = INIT_CPU_MASK_NONE;
> 	EXPORT_SYMBOL(cpu_mask_none);
> 
> 	cpumask_data cpu_mask_cpu0 __read_mostly = INIT_CPU_MASK_CPU0;
> 	EXPORT_SYMBOL(cpu_mask_cpu0);
> 	#endif
>