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

[George Dunlap <george.dunlap@citrix.com>]

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(), 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?

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.

 -George