Re: [PATCH 3/6] cpumask: truncate task_struct.cpus_allowed for CONFIG_CPUMASK_OFFSTACK

[Rusty Russell <rusty@rustcorp.com.au>]

On Tue, 24 Nov 2009 04:53:07 am Ingo Molnar wrote:
> 
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > Turns cpus_allowed into a bitmap, and truncate it to nr_cpu_ids if
> > CONFIG_CPUMASK_OFFSTACK is set.
> > 
> > I do this rather than the classic [0] dangling array trick, because of
> > INIT_TASK and references to sizeof(struct task_struct).
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > ---
> >  include/linux/init_task.h |    2 +-
> >  include/linux/sched.h     |    7 +++++--
> >  kernel/fork.c             |   19 ++++++++++++++++++-
> >  3 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> > --- a/include/linux/init_task.h
> > +++ b/include/linux/init_task.h
> > @@ -130,7 +130,7 @@ extern struct cred init_cred;
> >  	.static_prio	= MAX_PRIO-20,					\
> >  	.normal_prio	= MAX_PRIO-20,					\
> >  	.policy		= SCHED_NORMAL,					\
> > -	.cpus_allowed	= CPU_MASK_ALL,					\
> > +	.cpus_allowed	= CPU_BITS_ALL,					\
> >  	.mm		= NULL,						\
> >  	.active_mm	= &init_mm,					\
> >  	.se		= {						\
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1256,7 +1256,6 @@ struct task_struct {
> >  #endif
> >  
> >  	unsigned int policy;
> > -	cpumask_t cpus_allowed;
> >  
> >  #ifdef CONFIG_TREE_PREEMPT_RCU
> >  	int rcu_read_lock_nesting;
> > @@ -1544,10 +1543,14 @@ struct task_struct {
> >  	unsigned long trace_recursion;
> >  #endif /* CONFIG_TRACING */
> >  	unsigned long stack_start;
> > +
> > +	/* This has to go at the end: if CONFIG_CPUMASK_OFFSTACK=y, only
> > +	 * nr_cpu_ids bits will actually be allocated. */
> > +	DECLARE_BITMAP(cpus_allowed, CONFIG_NR_CPUS);
> 
> (nit: please use the comment style you see elsewhere in the file.)

Sure, my mistake.

> >  };
> >  
> >  /* Future-safe accessor for struct task_struct's cpus_allowed. */
> > -#define tsk_cpumask(tsk) (&(tsk)->cpus_allowed)
> > +#define tsk_cpumask(tsk) (to_cpumask((tsk)->cpus_allowed))
> 
> Please use tsk_cpus_allowed() throughout - so that people who knew what 
> p->cpus_allowed did know what this new thing does.

OK.  I chose this because it's shorter and consistent with other uses
(esp. mm_cpumask).  It's been in Linus' tree for a while, but noone uses
it so it's easy to rename.
 
> Also, i'm still having second thoughts about it all - could we somehow 
> avoid all this wrappery of commonly used fields? (My main and pretty 
> much only worry is that struct field members look so much cleaner than 
> some wrapper intermediary.)

I'm not a fan either, but it does avoid a flag-day transition.  And I was
pleasantly surprised how few places access ->cpus_allowed.

If we use a cpumask_var_t, we still need to change all the callers
(since it'll now look like a ptr), but we get a gratuitous ptr deref
for CONFIG_CPUMASK_OFFSTACK=y.

INIT_TASK needs some tweaking, but it's not a showstopper AFAICT.

Want me to create a cpumask_var_t version for comparison?

Thanks,
Rusty.