Re: [PATCH 05/31] cpumask: Provide new cpumask API

Previous thread: [PATCH 00/31] cpumask: Provide new cpumask API by Mike Travis on Monday, September 29, 2008 - 11:02 am. (1 message)

Next thread: [PATCH 06/31] cpumask: new lib/cpumask.c by Mike Travis on Monday, September 29, 2008 - 11:02 am. (1 message)
From: Mike Travis
Date: Monday, September 29, 2008 - 11:02 am

Provide new cpumask interface API.  The relevant change is basically
cpumask_t becomes an opaque object.  I believe this results in the
minimum amount of editing while still allowing the inline cpumask
functions, and the ability to declare static cpumask objects.


    /* raw declaration */
    struct __cpumask_data_s { DECLARE_BITMAP(bits, NR_CPUS); };

    /* cpumask_map_t used for declaring static cpumask maps */
    typedef struct __cpumask_data_s cpumask_map_t[1];

    /* cpumask_t used for function args and return pointers */
    typedef struct __cpumask_data_s *cpumask_t;
    typedef const struct __cpumask_data_s *const_cpumask_t;

    /* cpumask_var_t used for local variable, definition follows */
    typedef struct __cpumask_data_s	cpumask_var_t[1]; /* SMALL NR_CPUS */
    typedef struct __cpumask_data_s	*cpumask_var_t;	  /* LARGE NR_CPUS */

    /* replaces cpumask_t dst = (cpumask_t)src */
    void cpus_copy(cpumask_t dst, const cpumask_t src);

Remove the '*' indirection in all references to cpumask_t objects.  You can
change the reference to the cpumask object but not the cpumask object itself
without using the functions that operate on cpumask objects (f.e. the cpu_*
operators).  Functions can return a cpumask_t (which is a pointer to the
cpumask object) and only be passed a cpumask_t.

All uses of cpumask_t on the stack are changed to be cpumask_var_t except
for pointers to static cpumask objects.  Allocation of local (temp) cpumask
objects will follow...

All cpumask operators now operate using nr_cpu_ids instead of NR_CPUS.  All
variants of the cpumask operators which used nr_cpu_ids instead of NR_CPUS
are deleted.

All variants of functions which use the (old cpumask_t *) pointer are deleted
(f.e. set_cpus_allowed_ptr()).

Based on code from Rusty Russell <rusty@rustcorp.com.au> (THANKS!!)

Signed-of-by: Mike Travis <travis@sgi.com>

---
 include/linux/cpumask.h       |  445 +++++++++++++++++++++++-------------------
 ...
From: Ingo Molnar
Date: Tuesday, September 30, 2008 - 2:11 am

minor namespace nit i noticed while looking at actual usage of 
cpus_copy(): could you please rename it cpumask_set(dst, src)?

That streamlines it to have the same naming concept as atomic_set(), 
node_set(), zero_fd_set(), etc.

the patch-set looks quite nice otherwise already, the changes are 
straightforward and the end result already looks a lot more maintainable 
and not fragile at all.

In what way will Rusty's changes differ? Since you incorporate some of 
Rusty's changes already, could you please iterate towards a single 
patchset which we could then start testing?

	Ingo
--

From: Mike Travis
Date: Tuesday, September 30, 2008 - 8:42 am

Cpus_copy came from it's underlying function: bits_copy().  Cpumask_set
would deviate from the current naming convention of cpu_XXX for single

I was hoping for a stronger compiler error to indicate incorrect usage,
it currently just says "may be used before it's initialized" if you mistakenly

Our timezones are not very conducive to a lot of email exchanges (and he's moving.)
From what I've seen I believe he's leaning towards using struct cpumask * and
less trickery than I have.

The other alternative that is very easy to implement with the new code is
using a simple unsigned long list for cpumask_t (as Linus first suggested):

	typedef unsigned long cpumask_var_t[1];	/* small NR_CPUS */
	typedef unsigned long *cpumask_var_t;	/* large NR_CPUS */

This simplifies things quite a bit and I can get rid of some trickery (and
it's a pointer already without having to invent a pointer to a struct.)  The
downside is arrays of cpumask_t's are less clean, but doable.  The best thing
about the changes in this patchset is the context of the cpumask is more well
known, and changes to the underlying type for cpumask are confined to only a
few files (cpumask.h, lib/cpumask.c, etc.)

Thanks,
Mike
--

From: Mike Travis
Date: Tuesday, September 30, 2008 - 9:17 am

Oh yeah, I forgot the other major point of Rusty's approach.  He wants the
patchset to be completely bisectable.  That's far from true in my version.  

Thanks,
Mike
--

From: Ingo Molnar
Date: Wednesday, October 1, 2008 - 2:08 am

actually, that's quite sane to do. const_cpumask_t looked a bit weird to 
me.

the extra indirection to a cpumask_t is not a big issue IMO, so in that 
sense whether we pass by value or pass by reference is not a _big_ 
performance item.

The complications (both present and expected ones) all come from the 

well, it should be a smooth transition and completely bisectable, 
there's hundreds of usages of cpumask_t and quite many in the pipeline. 
It's far easier to _you_ to get this stuff to work if it's all gradual 
and is expected to work all across. Have a default-off debug mode that 
turns off compatible cpumask_t perhaps - we can remove that later on.

with 'struct cpumask' we could keep cpumask_t as the compatible API, and 
could see the impact of these changes in a very finegrained and gradual 
way. Seems like a fundamentally sane approach to me ...

	Ingo
--

Previous thread: [PATCH 00/31] cpumask: Provide new cpumask API by Mike Travis on Monday, September 29, 2008 - 11:02 am. (1 message)

Next thread: [PATCH 06/31] cpumask: new lib/cpumask.c by Mike Travis on Monday, September 29, 2008 - 11:02 am. (1 message)