Re: [RFC PATCH] kernel: sched: Provide a pointer to the valid CPU mask

[Ingo Molnar]

* Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> On Thu, 6 Apr 2017, Ingo Molnar wrote:
>
> > Sorry if this is a back and forth - I was somehow convinced that we do need to 
> > frob the cpus_allowed mask to get this functionality - but in hindsight I 
> > think the counter should be enough.
> > 
> > I.e. just have a counter and these two APIs:
> > 
> >         static inline void migrate_disable(void)
> >         {
> >                 current->migration_disabled++;
> >         }
> > 
> >         ...
> > 
> >         static inline void migrate_enable(void)
> >         {
> >                 current->migration_disabled--;
> >         }
> > 
> > ... and make sure the scheduler migration code plus the CPU hotplug code considers 
> > the counter.
> 
> We tried that some time ago, but there are a lot of places in the scheduler 
> which just rely on the cpus_allowed_mask, so we have to chase all of them and 
> make sure that new users do not ignore that counter. That's why we chose the 
> cpus_allowed_mask approach. And I still think that's the proper thing to do.

But but ...

The number of places in the scheduler where we actually end up migrating a task is 
pretty limited:

	try_to_wake_up():
		- main wakeup code

	migrate_swap():
		- active NUMA-balancing feature

	move_queued_task():
		- hotplug CPU-down migration
		- changing the affinity mask

The wakeup and NUMA balancing case is trivial to solve: it's an optimization and 
we can skip the migration if migration is disabled.

CPU hotplug and changing the affinity mask are the more complex cases, because 
there migrating or not migrating is a correctness issue:

 - CPU hotplug has to be aware of this anyway, regardless of whether it's solved 
   via a counter of the affinity mask.

 - Changing the affinity mask (set_cpus_allowed()) has two main cases:
   the synchronous and asynchronous case:

     - synchronous is when the current task changes its own affinity mask, this 
       should work fine mostly out of box, as we don't call set_cpus_allowed() 
       from inside migration disabled regions. (We can enforce this via a 
       debugging check.)

     - The asynchronous case is when the affinity task of some other task is 
       changed - this would not have an immediate effect with migration-disabled 
       logic, the migration would be delayed to when migration is re-enabled 
       again.

As for general fragility, is there any reason why a simple debugging check in 
set_task_cpu() would not catch most mishaps:

	WARN_ON_ONCE(p->state != TASK_RUNNING && p->migration_disabled);

... or something like that?

I.e. my point is that I think using a counter would be much simpler, yet still as 
robust and maintainable. We could in fact move migrate_disable()/enable() upstream 
straight away and eliminate this small fork of functionality between mainline and 
-rt.

Thanks,

	Ingo