Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Thomas Gleixner]

On Thu, 9 Feb 2017, Ingo Molnar wrote:
> And -rt would do something like this in migration_disable()/enable():
>  
>   t->cpus_ptr = _of(task_cpu(p));
>   t->nr_cpus = 1;
> 
>   ...
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = cpumask_weight(t->cpus_mask);
> 
> In addition to that we could cache the weight of the cpumask as an additional 
> optimization:
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = t->cpus_mask_weight;
> 
> It all looks like a pretty natural construct to me. The migration_disabled() 
> flag 
> spreads almost a hundred branches all across the scheduler.

I'm fine with that. Making the pointer const is clever and prevents people
from manipulating the wrong thing. If would be great if you could rework it
that way instead of just ripping all out.

Thanks,

tglx

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Thomas Gleixner]

On Thu, 9 Feb 2017, Ingo Molnar wrote:
> And -rt would do something like this in migration_disable()/enable():
>  
>   t->cpus_ptr = _of(task_cpu(p));
>   t->nr_cpus = 1;
> 
>   ...
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = cpumask_weight(t->cpus_mask);
> 
> In addition to that we could cache the weight of the cpumask as an additional 
> optimization:
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = t->cpus_mask_weight;
> 
> It all looks like a pretty natural construct to me. The migration_disabled() 
> flag 
> spreads almost a hundred branches all across the scheduler.

I'm fine with that. Making the pointer const is clever and prevents people
from manipulating the wrong thing. If would be great if you could rework it
that way instead of just ripping all out.

Thanks,

tglx

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Peter Zijlstra]

On Thu, Feb 09, 2017 at 07:57:27AM +0100, Ingo Molnar wrote:
> And -rt would do something like this in migration_disable()/enable():
>  
>   t->cpus_ptr = _of(task_cpu(p));
>   t->nr_cpus = 1;
> 
>   ...
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = cpumask_weight(t->cpus_mask);
> 
> In addition to that we could cache the weight of the cpumask as an additional 
> optimization:
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = t->cpus_mask_weight;
> 
> It all looks like a pretty natural construct to me. The migration_disabled() 
> flag 
> spreads almost a hundred branches all across the scheduler.

Could work I suppose. But please then implement this instead of ripping
out the current thing, because taking out the accessors leaves RT in a
bind without recourse.

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Peter Zijlstra]

On Thu, Feb 09, 2017 at 07:57:27AM +0100, Ingo Molnar wrote:
> And -rt would do something like this in migration_disable()/enable():
>  
>   t->cpus_ptr = _of(task_cpu(p));
>   t->nr_cpus = 1;
> 
>   ...
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = cpumask_weight(t->cpus_mask);
> 
> In addition to that we could cache the weight of the cpumask as an additional 
> optimization:
> 
>   t->cpus_ptr = >cpus_mask;
>   t->nr_cpus = t->cpus_mask_weight;
> 
> It all looks like a pretty natural construct to me. The migration_disabled() 
> flag 
> spreads almost a hundred branches all across the scheduler.

Could work I suppose. But please then implement this instead of ripping
out the current thing, because taking out the accessors leaves RT in a
bind without recourse.

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> > On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > > * Peter Zijlstra  wrote:
> > > >
> > > > cpumasks are a pain, the above avoids allocating more of them.
> > 
> > Indeed.
> > 
> > > Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> > > robust 
> > > than the wrappery,
> > 
> > You mean:
> > 
> > struct task_struct {
> >cpumask_tcpus_allowed;
> >cpumask_t*effective_cpus_allowed;
> > };

Yeah. I'd name it a bit differently and constify the pointer to get type 
safety and to make sure the mask is never modified through the pointer:

struct task_struct {
const cpumask_t *cpus_ptr;
cpumask_t   cpus_mask;
};

( I'd drop the 'allowed' part, it's obvious enough what task->cpus_mask does, 
  right? )

and upstream would essentially just do:

t->cpus_allowed_ptr = >cpus_allowed;

And -rt, when it wants to pin a task, would do:

t->cpus_allowed_ptr = _of(task_cpu(p));

The rules are:

 - Code that 'uses' ->cpus_allowed would use the pointer.

 - Code that 'modifies' ->cpus_allowed would use the direct mask.

The upstream advantages are:

 - The type separation of modifications from usage.

 - Removal of wrappery.

 - Maybe sometime in the future upstream would want to disable migration too ...

In fact -rt gains something too:

 - With this scheme we would AFAICS get slightly more optimal code on -rt.
   (Because there's no __migration_disabled() branching anymore.)

 - Plus all new code is automatically -rt ready - no need to maintain the 
wrappery 
   space. Much less code path forking.

So as I see it it's win-win for both upstream and for -rt!

> > and make the scheduler use effective_cpus_allowed instead of cpus_allowed? 
> > Or 
> > what do you have in mind?
> 
> That scheme is weird for nr_cpus_allowed. Not to mention that the
> pointer to the integer is larger than the integer itself.

So in the new scheme I don't think nr_cpus_allowed would have to be wrapped
at all: whenever the pointer (or mask) is changed in set_cpus_allowed_common() 
nr_cpus_allowed is recalculated as well - like today.

It should be self-maintaining. Am I missing something?

> I really prefer the current wrappers, they're trivial and consistent
> with one another.

I think it's ugly wrappery and we can do better! ;-)

But of course if I cannot suggest a better alternative then it stands.

Thanks,

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> > On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > > * Peter Zijlstra  wrote:
> > > >
> > > > cpumasks are a pain, the above avoids allocating more of them.
> > 
> > Indeed.
> > 
> > > Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> > > robust 
> > > than the wrappery,
> > 
> > You mean:
> > 
> > struct task_struct {
> >cpumask_tcpus_allowed;
> >cpumask_t*effective_cpus_allowed;
> > };

Yeah. I'd name it a bit differently and constify the pointer to get type 
safety and to make sure the mask is never modified through the pointer:

struct task_struct {
const cpumask_t *cpus_ptr;
cpumask_t   cpus_mask;
};

( I'd drop the 'allowed' part, it's obvious enough what task->cpus_mask does, 
  right? )

and upstream would essentially just do:

t->cpus_allowed_ptr = >cpus_allowed;

And -rt, when it wants to pin a task, would do:

t->cpus_allowed_ptr = _of(task_cpu(p));

The rules are:

 - Code that 'uses' ->cpus_allowed would use the pointer.

 - Code that 'modifies' ->cpus_allowed would use the direct mask.

The upstream advantages are:

 - The type separation of modifications from usage.

 - Removal of wrappery.

 - Maybe sometime in the future upstream would want to disable migration too ...

In fact -rt gains something too:

 - With this scheme we would AFAICS get slightly more optimal code on -rt.
   (Because there's no __migration_disabled() branching anymore.)

 - Plus all new code is automatically -rt ready - no need to maintain the 
wrappery 
   space. Much less code path forking.

So as I see it it's win-win for both upstream and for -rt!

> > and make the scheduler use effective_cpus_allowed instead of cpus_allowed? 
> > Or 
> > what do you have in mind?
> 
> That scheme is weird for nr_cpus_allowed. Not to mention that the
> pointer to the integer is larger than the integer itself.

So in the new scheme I don't think nr_cpus_allowed would have to be wrapped
at all: whenever the pointer (or mask) is changed in set_cpus_allowed_common() 
nr_cpus_allowed is recalculated as well - like today.

It should be self-maintaining. Am I missing something?

> I really prefer the current wrappers, they're trivial and consistent
> with one another.

I think it's ugly wrappery and we can do better! ;-)

But of course if I cannot suggest a better alternative then it stands.

Thanks,

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Ingo Molnar  wrote:

> 
> * Peter Zijlstra  wrote:
> 
> > On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> > > On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > > > * Peter Zijlstra  wrote:
> > > > >
> > > > > cpumasks are a pain, the above avoids allocating more of them.
> > > 
> > > Indeed.
> > > 
> > > > Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> > > > robust 
> > > > than the wrappery,
> > > 
> > > You mean:
> > > 
> > > struct task_struct {
> > >cpumask_t  cpus_allowed;
> > >cpumask_t  *effective_cpus_allowed;
> > > };
> 
> Yeah. I'd name it a bit differently and constify the pointer to get type 
> safety and to make sure the mask is never modified through the pointer:
> 
>   struct task_struct {
>   const cpumask_t *cpus_ptr;
>   cpumask_t   cpus_mask;
>   };
> 
> ( I'd drop the 'allowed' part, it's obvious enough what task->cpus_mask does, 
>   right? )
> 
> and upstream would essentially just do:
> 
>   t->cpus_allowed_ptr = >cpus_allowed;
> 
> And -rt, when it wants to pin a task, would do:
> 
>   t->cpus_allowed_ptr = _of(task_cpu(p));
> 
> The rules are:
> 
>  - Code that 'uses' ->cpus_allowed would use the pointer.
> 
>  - Code that 'modifies' ->cpus_allowed would use the direct mask.
> 
> The upstream advantages are:
> 
>  - The type separation of modifications from usage.
> 
>  - Removal of wrappery.
> 
>  - Maybe sometime in the future upstream would want to disable migration too 
> ...
> 
> In fact -rt gains something too:
> 
>  - With this scheme we would AFAICS get slightly more optimal code on -rt.
>(Because there's no __migration_disabled() branching anymore.)
> 
>  - Plus all new code is automatically -rt ready - no need to maintain the 
> wrappery 
>space. Much less code path forking.
> 
> So as I see it it's win-win for both upstream and for -rt!
> 
> > > and make the scheduler use effective_cpus_allowed instead of 
> > > cpus_allowed? Or 
> > > what do you have in mind?
> > 
> > That scheme is weird for nr_cpus_allowed. Not to mention that the
> > pointer to the integer is larger than the integer itself.
> 
> So in the new scheme I don't think nr_cpus_allowed would have to be wrapped
> at all: whenever the pointer (or mask) is changed in 
> set_cpus_allowed_common() 
> nr_cpus_allowed is recalculated as well - like today.
> 
> It should be self-maintaining. Am I missing something?

And -rt would do something like this in migration_disable()/enable():
 
t->cpus_ptr = _of(task_cpu(p));
t->nr_cpus = 1;

...

t->cpus_ptr = >cpus_mask;
t->nr_cpus = cpumask_weight(t->cpus_mask);

In addition to that we could cache the weight of the cpumask as an additional 
optimization:

t->cpus_ptr = >cpus_mask;
t->nr_cpus = t->cpus_mask_weight;

It all looks like a pretty natural construct to me. The migration_disabled() 
flag 
spreads almost a hundred branches all across the scheduler.

Thanks,

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Ingo Molnar  wrote:

> 
> * Peter Zijlstra  wrote:
> 
> > On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> > > On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > > > * Peter Zijlstra  wrote:
> > > > >
> > > > > cpumasks are a pain, the above avoids allocating more of them.
> > > 
> > > Indeed.
> > > 
> > > > Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> > > > robust 
> > > > than the wrappery,
> > > 
> > > You mean:
> > > 
> > > struct task_struct {
> > >cpumask_t  cpus_allowed;
> > >cpumask_t  *effective_cpus_allowed;
> > > };
> 
> Yeah. I'd name it a bit differently and constify the pointer to get type 
> safety and to make sure the mask is never modified through the pointer:
> 
>   struct task_struct {
>   const cpumask_t *cpus_ptr;
>   cpumask_t   cpus_mask;
>   };
> 
> ( I'd drop the 'allowed' part, it's obvious enough what task->cpus_mask does, 
>   right? )
> 
> and upstream would essentially just do:
> 
>   t->cpus_allowed_ptr = >cpus_allowed;
> 
> And -rt, when it wants to pin a task, would do:
> 
>   t->cpus_allowed_ptr = _of(task_cpu(p));
> 
> The rules are:
> 
>  - Code that 'uses' ->cpus_allowed would use the pointer.
> 
>  - Code that 'modifies' ->cpus_allowed would use the direct mask.
> 
> The upstream advantages are:
> 
>  - The type separation of modifications from usage.
> 
>  - Removal of wrappery.
> 
>  - Maybe sometime in the future upstream would want to disable migration too 
> ...
> 
> In fact -rt gains something too:
> 
>  - With this scheme we would AFAICS get slightly more optimal code on -rt.
>(Because there's no __migration_disabled() branching anymore.)
> 
>  - Plus all new code is automatically -rt ready - no need to maintain the 
> wrappery 
>space. Much less code path forking.
> 
> So as I see it it's win-win for both upstream and for -rt!
> 
> > > and make the scheduler use effective_cpus_allowed instead of 
> > > cpus_allowed? Or 
> > > what do you have in mind?
> > 
> > That scheme is weird for nr_cpus_allowed. Not to mention that the
> > pointer to the integer is larger than the integer itself.
> 
> So in the new scheme I don't think nr_cpus_allowed would have to be wrapped
> at all: whenever the pointer (or mask) is changed in 
> set_cpus_allowed_common() 
> nr_cpus_allowed is recalculated as well - like today.
> 
> It should be self-maintaining. Am I missing something?

And -rt would do something like this in migration_disable()/enable():
 
t->cpus_ptr = _of(task_cpu(p));
t->nr_cpus = 1;

...

t->cpus_ptr = >cpus_mask;
t->nr_cpus = cpumask_weight(t->cpus_mask);

In addition to that we could cache the weight of the cpumask as an additional 
optimization:

t->cpus_ptr = >cpus_mask;
t->nr_cpus = t->cpus_mask_weight;

It all looks like a pretty natural construct to me. The migration_disabled() 
flag 
spreads almost a hundred branches all across the scheduler.

Thanks,

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Peter Zijlstra]

On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > * Peter Zijlstra  wrote:
> > >
> > > cpumasks are a pain, the above avoids allocating more of them.
> 
> Indeed.
> 
> > Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> > robust 
> > than the wrappery,
> 
> You mean:
> 
> struct task_struct {
>cpumask_t  cpus_allowed;
>cpumask_t  *effective_cpus_allowed;
> };
> 
> and make the scheduler use effective_cpus_allowed instead of cpus_allowed?
> Or what do you have in mind?

That scheme is weird for nr_cpus_allowed. Not to mention that the
pointer to the integer is larger than the integer itself.

I really prefer the current wrappers, they're trivial and consistent
with one another.

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Peter Zijlstra]

On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > * Peter Zijlstra  wrote:
> > >
> > > cpumasks are a pain, the above avoids allocating more of them.
> 
> Indeed.
> 
> > Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> > robust 
> > than the wrappery,
> 
> You mean:
> 
> struct task_struct {
>cpumask_t  cpus_allowed;
>cpumask_t  *effective_cpus_allowed;
> };
> 
> and make the scheduler use effective_cpus_allowed instead of cpus_allowed?
> Or what do you have in mind?

That scheme is weird for nr_cpus_allowed. Not to mention that the
pointer to the integer is larger than the integer itself.

I really prefer the current wrappers, they're trivial and consistent
with one another.

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Thomas Gleixner]

On Mon, 6 Feb 2017, Ingo Molnar wrote:
> * Peter Zijlstra  wrote:
> >
> > cpumasks are a pain, the above avoids allocating more of them.

Indeed.

> Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> robust 
> than the wrappery,

You mean:

struct task_struct {
   cpumask_tcpus_allowed;
   cpumask_t*effective_cpus_allowed;
};

and make the scheduler use effective_cpus_allowed instead of cpus_allowed?
Or what do you have in mind?

> because as I've noted in the changelog there's a large body of 
> upstream code that does not use the wrappers but uses ->cpus_allowed directly:

Right and we really should audit those places. I bet that half of them are
just broken and evil hacks.

The wrapper we added is just covering the core scheduler code where the
information really matters for decision, but leaves the other oddball cases
alone.

The extra pointer might be a nicer concept, but it still has the same issue
as the wrapper. How do we enforce that random code accesses the right
thing?

Thanks,

tglx

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Thomas Gleixner]

On Mon, 6 Feb 2017, Ingo Molnar wrote:
> * Peter Zijlstra  wrote:
> >
> > cpumasks are a pain, the above avoids allocating more of them.

Indeed.

> Yeah, so this could then be done by pointerifying ->cpus_allowed - more 
> robust 
> than the wrappery,

You mean:

struct task_struct {
   cpumask_tcpus_allowed;
   cpumask_t*effective_cpus_allowed;
};

and make the scheduler use effective_cpus_allowed instead of cpus_allowed?
Or what do you have in mind?

> because as I've noted in the changelog there's a large body of 
> upstream code that does not use the wrappers but uses ->cpus_allowed directly:

Right and we really should audit those places. I bet that half of them are
just broken and evil hacks.

The wrapper we added is just covering the core scheduler code where the
information really matters for decision, but leaves the other oddball cases
alone.

The extra pointer might be a nicer concept, but it still has the same issue
as the wrapper. How do we enforce that random code accesses the right
thing?

Thanks,

tglx

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Mon, Feb 06, 2017 at 01:29:28PM +0100, Ingo Molnar wrote:
> > > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct 
> > > *p)
> > > +{
> > > +   if (__migrate_disabled(p))
> > > +   return cpumask_of(task_cpu(p));
> > > +
> > > +   return >cpus_allowed;
> > > +}
> > > +
> > > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > > +{
> > > +   if (__migrate_disabled(p))
> > > +   return 1;
> > > +   return p->nr_cpus_allowed;
> > > +}
> > 
> > So ... I think the cleaner approach in -rt would be to introduce 
> > ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> > current mask there and changing ->cpus_allowed - and then restoring it when 
> > re-enabling migration.
> > 
> > This means ->cpus_allowed could be used by the scheduler directly, no 
> > wrappery 
> > would be required, AFAICS.
> > 
> > ( Some extra care would be required in places that change ->cpus_allowed 
> > because 
> >   they'd now have to be aware of ->cpus_allowed_saved. )
> > 
> > Am I missing something?
> 
> cpumasks are a pain, the above avoids allocating more of them.

Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
than the wrappery, because as I've noted in the changelog there's a large body 
of 
upstream code that does not use the wrappers but uses ->cpus_allowed directly:

arch/ia64/kernel/mca.c: cpumask_set_cpu(cpu, >cpus_allowed);
arch/ia64/kernel/salinfo.c: cpumask_t save_cpus_allowed = 
current->cpus_allowed;
arch/ia64/kernel/topology.c:oldmask = current->cpus_allowed;
arch/ia64/sn/kernel/sn2/sn_hwperf.c:save_allowed = 
current->cpus_allowed;
arch/mips/include/asm/switch_to.h: * isn't set), we undo the restriction on 
cpus_allowed.
arch/mips/include/asm/switch_to.h:  prev->cpus_allowed = 
prev->thread.user_cpus_allowed;\
arch/mips/kernel/mips-mt-fpaff.c:   cpumask_var_t cpus_allowed, new_mask, 
effective_mask;
arch/mips/kernel/mips-mt-fpaff.c:   if (!alloc_cpumask_var(_allowed, 
GFP_KERNEL)) {
arch/mips/kernel/mips-mt-fpaff.c:   cpuset_cpus_allowed(p, 
cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:   if 
(!cpumask_subset(effective_mask, cpus_allowed)) {
arch/mips/kernel/mips-mt-fpaff.c:* update. Just reset 
the cpus_allowed to the
arch/mips/kernel/mips-mt-fpaff.c:* cpuset's cpus_allowed
arch/mips/kernel/mips-mt-fpaff.c:   cpumask_copy(new_mask, 
cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:   free_cpumask_var(cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:   cpumask_or(, 
>thread.user_cpus_allowed, >cpus_allowed);
arch/mips/kernel/traps.c:   if 
(cpumask_intersects(>cpus_allowed, _fpu_cpumask)) {
arch/mips/kernel/traps.c:   = current->cpus_allowed;
arch/mips/kernel/traps.c:   cpumask_and(, 
>cpus_allowed,
arch/powerpc/platforms/cell/spufs/sched.c:  
cpumask_copy(>cpus_allowed, tsk_cpus_allowed(current));
arch/powerpc/platforms/cell/spufs/sched.c:  if 
(cpumask_intersects(mask, >cpus_allowed))
arch/powerpc/platforms/cell/spufs/spufs.h:  cpumask_t cpus_allowed;
arch/tile/include/asm/setup.h:  if (!cpumask_equal(>cpus_allowed, new_mask)) 
\
arch/tile/kernel/hardwall.c:if (cpumask_weight(>cpus_allowed) != 1)
arch/tile/kernel/hardwall.c:BUG_ON(cpumask_first(>cpus_allowed) != cpu);
arch/tile/kernel/hardwall.c: * We assume the cpus_allowed, pid, and comm fields 
are still valid.
arch/tile/kernel/hardwall.c:if (cpumask_weight(>cpus_allowed) != 1) {
arch/tile/kernel/hardwall.c:   
cpumask_weight(>cpus_allowed));
drivers/acpi/processor_throttling.c:cpumask_copy(saved_mask, 
>cpus_allowed);
drivers/cpufreq/ia64-acpi-cpufreq.c:saved_mask = current->cpus_allowed;
drivers/cpufreq/ia64-acpi-cpufreq.c:saved_mask = current->cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:cpumask_t cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:cpus_allowed = current->cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:set_cpus_allowed_ptr(current, 
_allowed);
drivers/cpufreq/sh-cpufreq.c:   cpumask_t cpus_allowed;
drivers/cpufreq/sh-cpufreq.c:   cpus_allowed = current->cpus_allowed;
drivers/cpufreq/sh-cpufreq.c:   set_cpus_allowed_ptr(current, _allowed);
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_copy(_allowed, 
tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us2e-cpufreq.c:   set_cpus_allowed_ptr(current, 
_allowed);
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_copy(_allowed, 
tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us2e-cpufreq.c:   set_cpus_allowed_ptr(current, 

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Mon, Feb 06, 2017 at 01:29:28PM +0100, Ingo Molnar wrote:
> > > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct 
> > > *p)
> > > +{
> > > +   if (__migrate_disabled(p))
> > > +   return cpumask_of(task_cpu(p));
> > > +
> > > +   return >cpus_allowed;
> > > +}
> > > +
> > > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > > +{
> > > +   if (__migrate_disabled(p))
> > > +   return 1;
> > > +   return p->nr_cpus_allowed;
> > > +}
> > 
> > So ... I think the cleaner approach in -rt would be to introduce 
> > ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> > current mask there and changing ->cpus_allowed - and then restoring it when 
> > re-enabling migration.
> > 
> > This means ->cpus_allowed could be used by the scheduler directly, no 
> > wrappery 
> > would be required, AFAICS.
> > 
> > ( Some extra care would be required in places that change ->cpus_allowed 
> > because 
> >   they'd now have to be aware of ->cpus_allowed_saved. )
> > 
> > Am I missing something?
> 
> cpumasks are a pain, the above avoids allocating more of them.

Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
than the wrappery, because as I've noted in the changelog there's a large body 
of 
upstream code that does not use the wrappers but uses ->cpus_allowed directly:

arch/ia64/kernel/mca.c: cpumask_set_cpu(cpu, >cpus_allowed);
arch/ia64/kernel/salinfo.c: cpumask_t save_cpus_allowed = 
current->cpus_allowed;
arch/ia64/kernel/topology.c:oldmask = current->cpus_allowed;
arch/ia64/sn/kernel/sn2/sn_hwperf.c:save_allowed = 
current->cpus_allowed;
arch/mips/include/asm/switch_to.h: * isn't set), we undo the restriction on 
cpus_allowed.
arch/mips/include/asm/switch_to.h:  prev->cpus_allowed = 
prev->thread.user_cpus_allowed;\
arch/mips/kernel/mips-mt-fpaff.c:   cpumask_var_t cpus_allowed, new_mask, 
effective_mask;
arch/mips/kernel/mips-mt-fpaff.c:   if (!alloc_cpumask_var(_allowed, 
GFP_KERNEL)) {
arch/mips/kernel/mips-mt-fpaff.c:   cpuset_cpus_allowed(p, 
cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:   if 
(!cpumask_subset(effective_mask, cpus_allowed)) {
arch/mips/kernel/mips-mt-fpaff.c:* update. Just reset 
the cpus_allowed to the
arch/mips/kernel/mips-mt-fpaff.c:* cpuset's cpus_allowed
arch/mips/kernel/mips-mt-fpaff.c:   cpumask_copy(new_mask, 
cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:   free_cpumask_var(cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:   cpumask_or(, 
>thread.user_cpus_allowed, >cpus_allowed);
arch/mips/kernel/traps.c:   if 
(cpumask_intersects(>cpus_allowed, _fpu_cpumask)) {
arch/mips/kernel/traps.c:   = current->cpus_allowed;
arch/mips/kernel/traps.c:   cpumask_and(, 
>cpus_allowed,
arch/powerpc/platforms/cell/spufs/sched.c:  
cpumask_copy(>cpus_allowed, tsk_cpus_allowed(current));
arch/powerpc/platforms/cell/spufs/sched.c:  if 
(cpumask_intersects(mask, >cpus_allowed))
arch/powerpc/platforms/cell/spufs/spufs.h:  cpumask_t cpus_allowed;
arch/tile/include/asm/setup.h:  if (!cpumask_equal(>cpus_allowed, new_mask)) 
\
arch/tile/kernel/hardwall.c:if (cpumask_weight(>cpus_allowed) != 1)
arch/tile/kernel/hardwall.c:BUG_ON(cpumask_first(>cpus_allowed) != cpu);
arch/tile/kernel/hardwall.c: * We assume the cpus_allowed, pid, and comm fields 
are still valid.
arch/tile/kernel/hardwall.c:if (cpumask_weight(>cpus_allowed) != 1) {
arch/tile/kernel/hardwall.c:   
cpumask_weight(>cpus_allowed));
drivers/acpi/processor_throttling.c:cpumask_copy(saved_mask, 
>cpus_allowed);
drivers/cpufreq/ia64-acpi-cpufreq.c:saved_mask = current->cpus_allowed;
drivers/cpufreq/ia64-acpi-cpufreq.c:saved_mask = current->cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:cpumask_t cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:cpus_allowed = current->cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:set_cpus_allowed_ptr(current, 
_allowed);
drivers/cpufreq/sh-cpufreq.c:   cpumask_t cpus_allowed;
drivers/cpufreq/sh-cpufreq.c:   cpus_allowed = current->cpus_allowed;
drivers/cpufreq/sh-cpufreq.c:   set_cpus_allowed_ptr(current, _allowed);
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_copy(_allowed, 
tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us2e-cpufreq.c:   set_cpus_allowed_ptr(current, 
_allowed);
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us2e-cpufreq.c:   cpumask_copy(_allowed, 
tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us2e-cpufreq.c:   set_cpus_allowed_ptr(current, 
_allowed);

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Peter Zijlstra]

On Mon, Feb 06, 2017 at 01:29:28PM +0100, Ingo Molnar wrote:
> > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return cpumask_of(task_cpu(p));
> > +
> > +   return >cpus_allowed;
> > +}
> > +
> > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return 1;
> > +   return p->nr_cpus_allowed;
> > +}
> 
> So ... I think the cleaner approach in -rt would be to introduce 
> ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> current mask there and changing ->cpus_allowed - and then restoring it when 
> re-enabling migration.
> 
> This means ->cpus_allowed could be used by the scheduler directly, no 
> wrappery 
> would be required, AFAICS.
> 
> ( Some extra care would be required in places that change ->cpus_allowed 
> because 
>   they'd now have to be aware of ->cpus_allowed_saved. )
> 
> Am I missing something?

cpumasks are a pain, the above avoids allocating more of them.

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Peter Zijlstra]

On Mon, Feb 06, 2017 at 01:29:28PM +0100, Ingo Molnar wrote:
> > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return cpumask_of(task_cpu(p));
> > +
> > +   return >cpus_allowed;
> > +}
> > +
> > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return 1;
> > +   return p->nr_cpus_allowed;
> > +}
> 
> So ... I think the cleaner approach in -rt would be to introduce 
> ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> current mask there and changing ->cpus_allowed - and then restoring it when 
> re-enabling migration.
> 
> This means ->cpus_allowed could be used by the scheduler directly, no 
> wrappery 
> would be required, AFAICS.
> 
> ( Some extra care would be required in places that change ->cpus_allowed 
> because 
>   they'd now have to be aware of ->cpus_allowed_saved. )
> 
> Am I missing something?

cpumasks are a pain, the above avoids allocating more of them.

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Mike Galbraith]

On Mon, 2017-02-06 at 13:29 +0100, Ingo Molnar wrote:
> * Mike Galbraith  wrote:
> 
> > On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> > > * Mike Galbraith  wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > > > they grow more functionality in -rt, which is allegedly slowly but
> > > > surely headed toward merge.  I don't suppose they could be left intact?
> > > >  I can easily restore them in my local tree, but it seems a bit of a
> > > > shame to whack these integration friendly bits.
> > > 
> > > Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?
> > 
> > RT extends them to reflect whether migration is disabled or not.
> > 
> > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return cpumask_of(task_cpu(p));
> > +
> > +   return >cpus_allowed;
> > +}
> > +
> > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return 1;
> > +   return p->nr_cpus_allowed;
> > +}
> 
> So ... I think the cleaner approach in -rt would be to introduce 
> ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> current mask there and changing ->cpus_allowed - and then restoring it when 
> re-enabling migration.
> 
> This means ->cpus_allowed could be used by the scheduler directly, no 
> wrappery 
> would be required, AFAICS.
> 
> ( Some extra care would be required in places that change ->cpus_allowed 
> because 
>   they'd now have to be aware of ->cpus_allowed_saved. )
> 
> Am I missing something?

I suppose it's a matter of personal preference.  I prefer the above,
looks nice and clean to me.  Hohum, I'll just put them back locally for
the nonce.  My trees are only place holders until official releases
catch up anyway.

-Mike

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Mike Galbraith]

On Mon, 2017-02-06 at 13:29 +0100, Ingo Molnar wrote:
> * Mike Galbraith  wrote:
> 
> > On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> > > * Mike Galbraith  wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > > > they grow more functionality in -rt, which is allegedly slowly but
> > > > surely headed toward merge.  I don't suppose they could be left intact?
> > > >  I can easily restore them in my local tree, but it seems a bit of a
> > > > shame to whack these integration friendly bits.
> > > 
> > > Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?
> > 
> > RT extends them to reflect whether migration is disabled or not.
> > 
> > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return cpumask_of(task_cpu(p));
> > +
> > +   return >cpus_allowed;
> > +}
> > +
> > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > +{
> > +   if (__migrate_disabled(p))
> > +   return 1;
> > +   return p->nr_cpus_allowed;
> > +}
> 
> So ... I think the cleaner approach in -rt would be to introduce 
> ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> current mask there and changing ->cpus_allowed - and then restoring it when 
> re-enabling migration.
> 
> This means ->cpus_allowed could be used by the scheduler directly, no 
> wrappery 
> would be required, AFAICS.
> 
> ( Some extra care would be required in places that change ->cpus_allowed 
> because 
>   they'd now have to be aware of ->cpus_allowed_saved. )
> 
> Am I missing something?

I suppose it's a matter of personal preference.  I prefer the above,
looks nice and clean to me.  Hohum, I'll just put them back locally for
the nonce.  My trees are only place holders until official releases
catch up anyway.

-Mike

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Mike Galbraith  wrote:

> On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> > * Mike Galbraith  wrote:
> > 
> > > Hi Ingo,
> > > 
> > > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > > they grow more functionality in -rt, which is allegedly slowly but
> > > surely headed toward merge.  I don't suppose they could be left intact?
> > >  I can easily restore them in my local tree, but it seems a bit of a
> > > shame to whack these integration friendly bits.
> > 
> > Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?
> 
> RT extends them to reflect whether migration is disabled or not.
> 
> +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> +{
> +   if (__migrate_disabled(p))
> +   return cpumask_of(task_cpu(p));
> +
> +   return >cpus_allowed;
> +}
> +
> +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> +{
> +   if (__migrate_disabled(p))
> +   return 1;
> +   return p->nr_cpus_allowed;
> +}

So ... I think the cleaner approach in -rt would be to introduce 
->cpus_allowed_saved, and when disabling/enabling migration then saving the 
current mask there and changing ->cpus_allowed - and then restoring it when 
re-enabling migration.

This means ->cpus_allowed could be used by the scheduler directly, no wrappery 
would be required, AFAICS.

( Some extra care would be required in places that change ->cpus_allowed 
because 
  they'd now have to be aware of ->cpus_allowed_saved. )

Am I missing something?

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Mike Galbraith  wrote:

> On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> > * Mike Galbraith  wrote:
> > 
> > > Hi Ingo,
> > > 
> > > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > > they grow more functionality in -rt, which is allegedly slowly but
> > > surely headed toward merge.  I don't suppose they could be left intact?
> > >  I can easily restore them in my local tree, but it seems a bit of a
> > > shame to whack these integration friendly bits.
> > 
> > Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?
> 
> RT extends them to reflect whether migration is disabled or not.
> 
> +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> +{
> +   if (__migrate_disabled(p))
> +   return cpumask_of(task_cpu(p));
> +
> +   return >cpus_allowed;
> +}
> +
> +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> +{
> +   if (__migrate_disabled(p))
> +   return 1;
> +   return p->nr_cpus_allowed;
> +}

So ... I think the cleaner approach in -rt would be to introduce 
->cpus_allowed_saved, and when disabling/enabling migration then saving the 
current mask there and changing ->cpus_allowed - and then restoring it when 
re-enabling migration.

This means ->cpus_allowed could be used by the scheduler directly, no wrappery 
would be required, AFAICS.

( Some extra care would be required in places that change ->cpus_allowed 
because 
  they'd now have to be aware of ->cpus_allowed_saved. )

Am I missing something?

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Mike Galbraith]

On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> * Mike Galbraith  wrote:
> 
> > Hi Ingo,
> > 
> > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > they grow more functionality in -rt, which is allegedly slowly but
> > surely headed toward merge.  I don't suppose they could be left intact?
> >  I can easily restore them in my local tree, but it seems a bit of a
> > shame to whack these integration friendly bits.
> 
> Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?

RT extends them to reflect whether migration is disabled or not.

+/* Future-safe accessor for struct task_struct's cpus_allowed. */
+static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
+{
+   if (__migrate_disabled(p))
+   return cpumask_of(task_cpu(p));
+
+   return >cpus_allowed;
+}
+
+static inline int tsk_nr_cpus_allowed(struct task_struct *p)
+{
+   if (__migrate_disabled(p))
+   return 1;
+   return p->nr_cpus_allowed;
+}

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Mike Galbraith]

On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> * Mike Galbraith  wrote:
> 
> > Hi Ingo,
> > 
> > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > they grow more functionality in -rt, which is allegedly slowly but
> > surely headed toward merge.  I don't suppose they could be left intact?
> >  I can easily restore them in my local tree, but it seems a bit of a
> > shame to whack these integration friendly bits.
> 
> Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?

RT extends them to reflect whether migration is disabled or not.

+/* Future-safe accessor for struct task_struct's cpus_allowed. */
+static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
+{
+   if (__migrate_disabled(p))
+   return cpumask_of(task_cpu(p));
+
+   return >cpus_allowed;
+}
+
+static inline int tsk_nr_cpus_allowed(struct task_struct *p)
+{
+   if (__migrate_disabled(p))
+   return 1;
+   return p->nr_cpus_allowed;
+}

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Mike Galbraith  wrote:

> Hi Ingo,
> 
> Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> they grow more functionality in -rt, which is allegedly slowly but
> surely headed toward merge.  I don't suppose they could be left intact?
>  I can easily restore them in my local tree, but it seems a bit of a
> shame to whack these integration friendly bits.

Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?

Thanks,

Ingo

Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Ingo Molnar]

* Mike Galbraith  wrote:

> Hi Ingo,
> 
> Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> they grow more functionality in -rt, which is allegedly slowly but
> surely headed toward merge.  I don't suppose they could be left intact?
>  I can easily restore them in my local tree, but it seems a bit of a
> shame to whack these integration friendly bits.

Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?

Thanks,

Ingo

tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Mike Galbraith]

Hi Ingo,

Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
they grow more functionality in -rt, which is allegedly slowly but
surely headed toward merge.  I don't suppose they could be left intact?
 I can easily restore them in my local tree, but it seems a bit of a
shame to whack these integration friendly bits.

-Mike

tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

[Mike Galbraith]

Hi Ingo,

Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
they grow more functionality in -rt, which is allegedly slowly but
surely headed toward merge.  I don't suppose they could be left intact?
 I can easily restore them in my local tree, but it seems a bit of a
shame to whack these integration friendly bits.

-Mike