Re: [PATCH 3/9] xen: sched: make locking for {insert, remove}_vcpu consistent

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/10/15 15:58, George Dunlap wrote:
> On 29/09/15 18:31, Andrew Cooper wrote:
>> On 29/09/15 17:55, Dario Faggioli wrote:
>>> The insert_vcpu() scheduler hook is called with an
>>> inconsistent locking strategy. In fact, it is sometimes
>>> invoked while holding the runqueue lock and sometimes
>>> when that is not the case.
>>>
>>> In other words, some call sites seems to imply that
>>> locking should be handled in the callers, in schedule.c
>>> --e.g., in schedule_cpu_switch(), which acquires the
>>> runqueue lock before calling the hook; others that
>>> specific schedulers should be responsible for locking
>>> themselves --e.g., in sched_move_domain(), which does
>>> not acquire any lock for calling the hook.
>>>
>>> The right thing to do seems to always defer locking to
>>> the specific schedulers, as it's them that know what, how
>>> and when it is best to lock (as in: runqueue locks, vs.
>>> private scheduler locks, vs. both, etc.)
>>>
>>> This patch, therefore:
>>>  - removes any locking around insert_vcpu() from
>>>    generic code (schedule.c);
>>>  - add the _proper_ locking in the hook implementations,
>>>    depending on the scheduler (for instance, credit2
>>>    does that already, credit1 and RTDS need to grab
>>>    the runqueue lock while manipulating runqueues).
>>>
>>> In case of credit1, remove_vcpu() handling needs some
>>> fixing remove_vcpu() too, i.e.:
>>>  - it manipulates runqueues, so the runqueue lock must
>>>    be acquired;
>>>  - *_lock_irq() is enough, there is no need to do
>>>    _irqsave()
>> Nothing in any of generic scheduling code should need interrupts
>> disabled at all.
>>
>> One of the problem-areas identified by Jenny during the ticketlock
>> performance work was that the SCHEDULE_SOFTIRQ was a large consumer of
>> time with interrupts disabled.  (The other large one being the time
>> calibration rendezvous, but that is a wildly different can of worms to fix.)
> Generic scheduling code is called from interrupt contexts -- namely,
> vcpu_wake()

There are a lot of codepaths, but I cant see one which is definitely
called with interrupts disables.  (OTOH, I can see several where
interrupts are definitely enabled).

> , which for the credit scheduler wants to put things on the
> runqueue.  Lock taken in interrupt context => interrupts must be
> disabled whenever taking the lock, yes?

Correct, which is the purpose of the ASSERT()s in the _irq() and
_irqsave() variants.

> There may be a way we can revise the whole scheduling system to separate
> locks taken in an interrupt context from other locks (for instance,
> having a special "wake" queue which is drained in schedule() or
> something) but it's not as simple as just switching all the locks over
> to non-irq.

I did not wish to imply that this is trivial to achieve.  I am entirely
willing to believe that it might involve changes of logic to achieve.

~Andrew