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

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

[Rusty Russell]

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);
 };
 
 /* 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))
 
 /*
  * Priority of a process goes from 0..MAX_PRIO-1, valid RT
diff --git a/kernel/fork.c b/kernel/fork.c
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -183,9 +183,26 @@ void __init fork_init(unsigned long memp
 #ifndef ARCH_MIN_TASKALIGN
 #define ARCH_MIN_TASKALIGN	L1_CACHE_BYTES
 #endif
+	unsigned int task_size;
+
+#ifdef CONFIG_CPUMASK_OFFSTACK
+	/*
+	 * Restrict task_struct allocations so cpus_allowed is only
+	 * nr_cpu_ids long.  cpus_allowed must be a NR_CPUS bitmap at
+	 * end for this to work.
+	 */
+	BUILD_BUG_ON(offsetof(struct task_struct, cpus_allowed)
+		     + BITS_TO_LONGS(CONFIG_NR_CPUS)*sizeof(long)
+		     != sizeof(struct task_struct));
+	task_size = offsetof(struct task_struct, cpus_allowed) +
+		BITS_TO_LONGS(nr_cpu_ids) * sizeof(long);
+#else
+	task_size = sizeof(struct task_struct);
+#endif /* CONFIG_CPUMASK_OFFSTACK */
+
 	/* create a slab on which task_structs can be allocated */
 	task_struct_cachep =
-		kmem_cache_create("task_struct", sizeof(struct task_struct),
+		kmem_cache_create("task_struct", task_size,
 			ARCH_MIN_TASKALIGN, SLAB_PANIC | SLAB_NOTRACK, NULL);
 #endif
 

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

[Ingo Molnar]

* 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.)

>  };
>  
>  /* 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.

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.)

	Ingo

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

[Rusty Russell]

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.

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

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> 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.

Thanks!

> > 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?

There's also the problem of cache footprint - we really want to freedom 
to place the masks (which are not off-stack in the regular case) 
anywhere in task struct - and task-struct is huge, so cache placement 
matters.

Is the ptr deref really a big problem for off-stack?

Changing all users isnt a problem as long as it still looks like a clean 
data structure with clean usage. Those end-of-struct tricks we play with 
cpumasks make me really a bit uneasy.

	Ingo

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

[Rusty Russell]

On Tue, 24 Nov 2009 09:44:40 am Ingo Molnar wrote:
> There's also the problem of cache footprint - we really want to freedom 
> to place the masks (which are not off-stack in the regular case) 
> anywhere in task struct - and task-struct is huge, so cache placement 
> matters.

My initial patch changed it to always be a const cpumask *, with it
pointing to the standard all or single-cpu masks in the normal cases: it
only got its own allocated if we changed it to something unusual.

It was more invasive though, and really needs to be thought about separately.
I'd want to measure how often it is accessed, for example.

> Is the ptr deref really a big problem for off-stack?

I don't think so; slightly suboptimal codegen but it'll still just point to
the end of the struct.

> Changing all users isnt a problem as long as it still looks like a clean 
> data structure with clean usage.

We're still supposed to use a wrapper to avoid a flag day.  It's gratuitous
if we're going to do this, but that's modern kernel life AFAICT.

> Those end-of-struct tricks we play with cpumasks make me really a bit uneasy.

Fair enough.

I don't know when I'll get more cycles on this.  A few more months I expect.

Rusty.