Re: [PATCH] mask ADT: replace cpumask_t implementation [3/22]

[Matthew Dobson <colpatch@us.ibm.com>]

On Mon, 2004-03-29 at 04:12, Paul Jackson wrote:
> Patch_3_of_22 - Rework cpumasks to use new mask ADT
> 	Removes many old include/asm-<arch> and asm-generic cpumask files
> 	Add intersects, subset, xor and andnot operators.
> 	Provides temporary emulators for obsolete const, promote, coerce
> 	Presents entire cpumask API clearly in single cpumask.h file

<snip>

> + * int num_online_cpus()		Number of online CPUs
> + * int num_possible_cpus()		Number of all possible CPUs
> + * int cpu_online(cpu)			Is some cpu < NR_CPUS online?
> + * int cpu_possible(cpu)		Is some cpu < NR_CPUS possible?
> + * int any_online_cpu(mask)		First online cpu in mask

This made me do a double-take.  I don't think it's necessary to specify
the passed in cpu number is < NR_CPUS, because that is true for the
whole API.  This comment seemed to imply at first reading that the call
would return true if any CPU with a number less than 'cpu' was
online/possible.


> +#define cpu_set(cpu, mask) mask_setbit((cpu), (mask))
> +#define cpu_clear(cpu, mask) mask_clearbit((cpu), (mask))
> +#define cpus_setall(mask) mask_setall(mask, NR_CPUS)
> +#define cpus_clear(mask) mask_clearall(mask)
> +#define cpu_isset(cpu, mask) mask_isset((cpu), (mask))
> +#define cpu_test_and_set(cpu, mask) mask_test_and_set((cpu), (mask))
> +#define cpus_and(dst, src1, src2) mask_and((dst), (src1), (src2))
> +#define cpus_or(dst, src1, src2) mask_or((dst), (src1), (src2))
> +#define cpus_xor(dst, src1, src2) mask_xor((dst), (src1), (src2))
> +#define cpus_andnot(dst, src1, src2) mask_andnot((dst), (src1), (src2))
> +#define cpus_complement(dst, src) mask_complement((dst), (src), NR_CPUS)
> +#define cpus_equal(mask1, mask2) mask_equal((mask1), (mask2))
> +#define cpus_intersects(mask1, mask2) mask_intersects(mask1, mask2)
> +#define cpus_subset(mask1, mask2) mask_subset(mask1, mask2)
> +#define cpus_empty(mask) mask_empty(mask)
> +#define cpus_full(mask) mask_full(mask, NR_CPUS)
> +#define cpus_weight(mask) mask_weight(mask, NR_CPUS)
> +#define cpus_shift_right(dst, src, n) \
> +			mask_shift_right((dst), (src), (n), NR_CPUS)
> +#define cpus_shift_left(dst, src, n) \
> +			mask_shift_left((dst), (src), (n), NR_CPUS)
> +#define first_cpu(mask) mask_first(mask, NR_CPUS)
> +#define next_cpu(cpu, mask) mask_next(cpu, mask, NR_CPUS)
> +#define cpumask_of_cpu(cpu) mask_of_bit((cpu), _unused_cpumask_arg_)
> +#define CPU_MASK_ALL MASK_ALL(NR_CPUS)
> +#define CPU_MASK_NONE MASK_NONE(NR_CPUS)
> +#define cpus_raw(mask) mask_raw(mask)
> +#define cpumask_scnprintf(buf, len, mask) \
> +			mask_scnprintf(buf, len, mask, NR_CPUS)
> +#define cpumask_parse(ubuf, ulen, mask) \
> +			mask_parse(ubuf, ulen, mask, NR_CPUS)

What would you think about creating two versions of some of the mask_*
functions, one that takes a nrbits argument and one that doesn't.  Many
(all?) of the bitmap_* functions take a number of bits to operate on,
and for the mask_* functions we simply compute the size of the mask and
pass that along.  We can be smarter in the cpumask/nodemask cases
because we *know* how many bits we're really using, and we can pass that
information along.  Basically, what I'm suggesting is:

mask_empty(mask) -> bitmap_empty(mask, sizeof(mask))
mask_empty_bits(mask, nrbits) -> bitmap_empty(mask, nrbits)
cpus_empty(mask) -> mask_empty_bits(mask, NR_CPUS)

This allows anyone using the mask API to specify the exact number of
bits if they care to, and it buys us a little protection from non-word
sized cpumask/nodemask errors.


>  #else
> +
>  #define	cpu_online_map			cpumask_of_cpu(0)
>  #define	cpu_possible_map		cpumask_of_cpu(0)
> +
>  #define num_online_cpus()		1
>  #define num_possible_cpus()		1
>  #define cpu_online(cpu)			({ BUG_ON((cpu) != 0); 1; })
>  #define cpu_possible(cpu)		({ BUG_ON((cpu) != 0); 1; })

Might it make more sense to actually define a cpu_online_map &
cpu_possible_map for UP, rather than generating this code:

#define mask_of_bit(bit, T)                                            \
({                                                                     \
       typeof(T) m;                                                    \
       mask_clearall(m);                                               \
       mask_setbit((bit), m);                                          \
       m;                                                              \
})

every time some code references cpu_online_map?  It'll only cost us 2
unsigned longs on 32-bit == 8 bytes...

-Matt