lkml.org 
[lkml]   [2008]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC] CPUMASK: proposal for replacing cpumask_t
From
Date
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
>



\
 
 \ /
  Last update: 2008-09-11 11:03    [W:0.210 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site