Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH 3/6] cpumask: truncate task_struct.cpus_allowed for CONFIG_CPUMASK_OFFSTACK | Date | Tue, 24 Nov 2009 09:18:39 +1030 |
| |
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.
| |