Re: [patch 09/10] sched/core: Add migrate_disable/enable()

From: Thomas Gleixner
Date: Fri Sep 18 2020 - 03:01:19 EST


On Thu, Sep 17 2020 at 16:24, peterz wrote:
> On Thu, Sep 17, 2020 at 11:42:11AM +0200, Thomas Gleixner wrote:
>
>> +static inline void update_nr_migratory(struct task_struct *p, long delta)
>> +{
>> + if (p->nr_cpus_allowed > 1 && p->sched_class->update_migratory)
>> + p->sched_class->update_migratory(p, delta);
>> +}
>
> Right, so as you know, I totally hate this thing :-) It adds a second
> (and radically different) version of changing affinity. I'm working on a
> version that uses the normal *set_cpus_allowed*() interface.

Tried that back and forth and ended either up in locking hell or with
race conditions of sorts, but my scheduler foo is rusty.

>> +static inline void sched_migration_ctrl(struct task_struct *prev, int cpu)
>> +{
>> + if (!prev->migration_ctrl.disable_cnt ||
>> + prev->cpus_ptr != &prev->cpus_mask)
>> + return;
>> +
>> + prev->cpus_ptr = cpumask_of(cpu);
>> + update_nr_migratory(prev, -1);
>> + prev->nr_cpus_allowed = 1;
>> +}
>
> So this thing is called from schedule(), with only rq->lock held, and
> that violates the locking rules for changing the affinity.
>
> I have a comment that explains how it's broken and why it's sort-of
> working.

Yeah :(

>> +void migrate_disable(void)
>> +{
>> + unsigned long flags;
>> +
>> + if (!current->migration_ctrl.disable_cnt) {
>> + raw_spin_lock_irqsave(&current->pi_lock, flags);
>> + current->migration_ctrl.disable_cnt++;
>> + raw_spin_unlock_irqrestore(&current->pi_lock, flags);
>> + } else {
>> + current->migration_ctrl.disable_cnt++;
>> + }
>> +}
>
> That pi_lock seems unfortunate, and it isn't obvious what the point of
> it is.

Indeed. That obviously lacks a big fat comment.

current->migration_ctrl.disable_cnt++ is obviously a RMW operation. So
you end up with the following:

CPU0 CPU1
migrate_disable()
R = current->migration_ctrl.disable_cnt;
set_cpus_allowed_ptr()
task_rq_lock();
if
(!p->migration_ctrl.disable_cnt) {
current->migration_ctrl.disable_cnt = R + 1;
stop_one_cpu();
---> stopper_thread()
BUG_ON(task->migration_ctrl.disable_cnt);

I tried to back out from that instead of BUG(), but that ended up being
even more of a hacky trainwreck than just biting the bullet and taking
pi_lock.

>
> So, what I'm missing with all this are the design contraints for this
> trainwreck. Because the 'sane' solution was having migrate_disable()
> imply cpus_read_lock(). But that didn't fly because we can't have
> migrate_disable() / migrate_enable() schedule for raisins.

Yeah. The original code had some magic

if (preemptible())
cpus_read_lock();
else
p->atomic_migrate_disable++;

but that caused another set of horrors with asymetric code like the
below and stuff like try_lock().

> And if I'm not mistaken, the above migrate_enable() *does* require being
> able to schedule, and our favourite piece of futex:
>
> raw_spin_lock_irq(&q.pi_state->pi_mutex.wait_lock);
> spin_unlock(q.lock_ptr);
>
> is broken. Consider that spin_unlock() doing migrate_enable() with a
> pending sched_setaffinity().

Yes, we have the extra migrate_disable()/enable() pair around that.

The other way I solved that was to have a spin_[un]lock() variant which
does not have a migrate disable/enable. That works in that code because
there is no per CPUness requirement. Not pretty either...

Thanks,

tglx