2009-03-12 04:07:14

by Rusty Russell

[permalink] [raw]
Subject: [PULL] Wrapper macros for struct task_struct and struct mm_struct cpumask transition

The following changes since commit 99adcd9d67aaf04e28f5ae96df280f236bde4b66:
Linus Torvalds (1):
Merge branch 'fixes' of git://git.kernel.org/.../davej/cpufreq

are available in the git repository at:

ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master

Rusty Russell (2):
cpumask: tsk_cpumask for accessing the struct task_struct's cpus_allowed.
cpumask: mm_cpumask for accessing the struct mm_struct's cpu_vm_mask.

include/linux/mm_types.h | 3 +++
include/linux/sched.h | 3 +++
2 files changed, 6 insertions(+), 0 deletions(-)


2009-03-12 04:20:42

by Nick Piggin

[permalink] [raw]
Subject: Re: [PULL] Wrapper macros for struct task_struct and struct mm_struct cpumask transition

On Thursday 12 March 2009 15:06:52 Rusty Russell wrote:
> The following changes since commit
> 99adcd9d67aaf04e28f5ae96df280f236bde4b66: Linus Torvalds (1):
> Merge branch 'fixes' of git://git.kernel.org/.../davej/cpufreq
>
> are available in the git repository at:
>
>
> ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.
>git master
>
> Rusty Russell (2):
> cpumask: tsk_cpumask for accessing the struct task_struct's
> cpus_allowed. cpumask: mm_cpumask for accessing the struct mm_struct's
> cpu_vm_mask.
>
> include/linux/mm_types.h | 3 +++
> include/linux/sched.h | 3 +++

What's the transition?

2009-03-12 07:15:46

by Rusty Russell

[permalink] [raw]
Subject: Re: [PULL] Wrapper macros for struct task_struct and struct mm_struct cpumask transition

On Thursday 12 March 2009 14:50:11 Nick Piggin wrote:
> On Thursday 12 March 2009 15:06:52 Rusty Russell wrote:
> > Rusty Russell (2):
> > cpumask: tsk_cpumask for accessing the struct task_struct's cpus_allowed.
> > cpumask: mm_cpumask for accessing the struct mm_struct's cpu_vm_mask.
> >
> > include/linux/mm_types.h | 3 +++
> > include/linux/sched.h | 3 +++
>
> What's the transition?

Generally, cpumask_t to cpumask_var_t, ie. a struct cpumask [1] normally,
or a struct cpumask * for CONFIG_CPUMASK_OFFSTACK=y (currently x86 only).

In these cases though, we're allocating them anyway so it makes more sense to
do a dangling bitmap at the end of the struct (and only allocate nr_cpu_ids
bits when CONFIG_CPUMASK_OFFSTACK=y).

I had a patch which used a const struct cpumask * for task_struct: we only
ever replace the whole thing, so we can often use the standard cpu_mask_all
or cpumask_of() and only allocate when it's set to something else. But
the code was pretty ugly.

Anyway, it's a trivial to change if people use the wrappers. And if the
wrappers are in Linus' tree, it's easy to get the conversions into linux-next.

Cheers,
Rusty.

2009-03-12 07:39:37

by Andrew Morton

[permalink] [raw]
Subject: Re: [PULL] Wrapper macros for struct task_struct and struct mm_struct cpumask transition

On Thu, 12 Mar 2009 17:45:22 +1030 Rusty Russell <[email protected]> wrote:

> On Thursday 12 March 2009 14:50:11 Nick Piggin wrote:
> > On Thursday 12 March 2009 15:06:52 Rusty Russell wrote:
> > > Rusty Russell (2):
> > > cpumask: tsk_cpumask for accessing the struct task_struct's cpus_allowed.
> > > cpumask: mm_cpumask for accessing the struct mm_struct's cpu_vm_mask.
> > >
> > > include/linux/mm_types.h | 3 +++
> > > include/linux/sched.h | 3 +++

Please prefer to put the patches in the email if practical? More
eyeballs and all that.

<goes fishing in linux-next>


: commit ccbc7af2a675bce8a9d4d3ab8e9c8a71a39d9158
: Author: Rusty Russell <[email protected]>
: AuthorDate: Tue Mar 10 10:03:44 2009 +1100
: Commit: Stephen Rothwell <[email protected]>
: CommitDate: Tue Mar 10 10:03:44 2009 +1100
:
: cpumask:task_cpumask
:
: This allows us to change the representation (to a dangling bitmap)
: without breaking all the callers: they can use tsk_cpumask() now and
: won't see a difference as the changes roll into linux-next.
:
: Signed-off-by: Rusty Russell <[email protected]>
:
: diff --git a/include/linux/sched.h b/include/linux/sched.h
: index 8c216e0..011db2f 100644
: --- a/include/linux/sched.h
: +++ b/include/linux/sched.h
: @@ -1419,6 +1419,9 @@ struct task_struct {
: #endif
: };
:
: +/* Future-safe accessor for struct task_struct's cpus_allowed. */
: +#define tsk_cpumask(tsk) (&(tsk)->cpus_allowed)
: +
: /*
: * Priority of a process goes from 0..MAX_PRIO-1, valid RT
: * priority is 0..MAX_RT_PRIO-1, and SCHED_NORMAL/SCHED_BATCH
:
:
:
: commit 614e0176340928efb92cba676126048fdd466f0f
: Author: Rusty Russell <[email protected]>
: AuthorDate: Tue Mar 10 10:03:44 2009 +1100
: Commit: Stephen Rothwell <[email protected]>
: CommitDate: Tue Mar 10 10:03:44 2009 +1100
:
: cpumask:mm_cpumask
:
: This allows us to change the representation (to a dangling bitmap)
: without breaking all the callers: they can use mm_cpumask() now and
: won't see a difference as the changes roll into linux-next.
:
: Signed-off-by: Rusty Russell <[email protected]>
:
: diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
: index 92915e8..d84feb7 100644
: --- a/include/linux/mm_types.h
: +++ b/include/linux/mm_types.h
: @@ -276,4 +276,7 @@ struct mm_struct {
: #endif
: };
:
: +/* Future-safe accessor for struct mm_struct's cpu_vm_mask. */
: +#define mm_cpumask(mm) (&(mm)->cpu_vm_mask)
: +
: #endif /* _LINUX_MM_TYPES_H */
:


It would be pretty perverse to run tsk_cpumask() against a `struct
cpuset*', but your proposed implementation would merrily permit that
mistake.

Can we write the kernel in C please??


> And if the
> wrappers are in Linus' tree, it's easy to get the conversions into linux-next.

yup, the patches are quite safe and mergeable. And if they'd been in the
email body, Linus might have seen that and pulled them ;)

2009-03-12 09:13:25

by Rusty Russell

[permalink] [raw]
Subject: Re: [PULL] Wrapper macros for struct task_struct and struct mm_struct cpumask transition

On Thursday 12 March 2009 18:07:09 Andrew Morton wrote:
> On Thu, 12 Mar 2009 17:45:22 +1030 Rusty Russell <[email protected]> wrote:
...
> Please prefer to put the patches in the email if practical? More
> eyeballs and all that.

Hmm, yes. Is there a preferred way of doing that with git request-pull?
Or just append the diff?

> : +/* Future-safe accessor for struct mm_struct's cpu_vm_mask. */
> : +#define mm_cpumask(mm) (&(mm)->cpu_vm_mask)
> : +
...
> It would be pretty perverse to run tsk_cpumask() against a `struct
> cpuset*', but your proposed implementation would merrily permit that
> mistake.
>
> Can we write the kernel in C please??

You mean use an inline? No, it's a bad idea for two reasons. The minor
reason is that this macro is a temporary so it makes more sense to fix the
final version.

But the major one is const correctness. Macros give a const value for
const input. We can enhance the macro to check, but that's even uglier:

static inline int check_is_task_struct(const struct task_struct *t) { }
#define tsk_cpumask(t) (sizeof(check_is_task_struct(t)), &(t)->cpu_vm_mask)

> yup, the patches are quite safe and mergeable. And if they'd been in the
> email body, Linus might have seen that and pulled them ;)

Probably not, as that's not what he complained about last time. He might have
been enlightened by the commit messages tho.

Cheers,
Rusty.

2009-03-12 09:26:57

by Andrew Morton

[permalink] [raw]
Subject: Re: [PULL] Wrapper macros for struct task_struct and struct mm_struct cpumask transition

On Thu, 12 Mar 2009 19:43:02 +1030 Rusty Russell <[email protected]> wrote:

> On Thursday 12 March 2009 18:07:09 Andrew Morton wrote:
> > On Thu, 12 Mar 2009 17:45:22 +1030 Rusty Russell <[email protected]> wrote:
> ...
> > Please prefer to put the patches in the email if practical? More
> > eyeballs and all that.
>
> Hmm, yes. Is there a preferred way of doing that with git request-pull?
> Or just append the diff?

You're asking me about git?

> > : +/* Future-safe accessor for struct mm_struct's cpu_vm_mask. */
> > : +#define mm_cpumask(mm) (&(mm)->cpu_vm_mask)
> > : +
> ...
> > It would be pretty perverse to run tsk_cpumask() against a `struct
> > cpuset*', but your proposed implementation would merrily permit that
> > mistake.
> >
> > Can we write the kernel in C please??
>
> You mean use an inline? No, it's a bad idea for two reasons. The minor
> reason is that this macro is a temporary so it makes more sense to fix the
> final version.
>
> But the major one is const correctness. Macros give a const value for
> const input.

hm. Sounds weaselly.

> We can enhance the macro to check, but that's even uglier:
>
> static inline int check_is_task_struct(const struct task_struct *t) { }
> #define tsk_cpumask(t) (sizeof(check_is_task_struct(t)), &(t)->cpu_vm_mask)

include/linux/typecheck.h has help?

> > yup, the patches are quite safe and mergeable. And if they'd been in the
> > email body, Linus might have seen that and pulled them ;)
>
> Probably not, as that's not what he complained about last time.

Linus? Complain?

> He might have
> been enlightened by the commit messages tho.