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

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

On 08/10/15 18:23, Andrew Cooper wrote:
> On 08/10/15 17:46, George Dunlap wrote:
>> On 08/10/15 16:20, Andrew Cooper wrote:
>>> 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).
>> Oh, I think I misunderstood you.  You meant, "No codepaths *calling
>> into* generic scheduling code should need interrupts disabled at all".
>> I can certainly believe that to be true in most cases; there's no sense
>> in saving the flags if we don't need to.
> 
> My original statement came from the observation that schedule() runs
> with interrupts disabled, and takes between 2.2 and 4 microseconds to
> run (as measured during the ticketlock performance analysis).
> 
> It is the biggest consumer of time with interrupts disabled, next being
> the time calibration rendezvous.
> 
> I am going to go out on a limb and say that the majority of that time
> does not need to be spent with interrupts disabled.  I might easily be
> wrong, but I suspect I am not.

It's certainly worth taking a look at -- in particular, as (if I recall
correctly) we grab the schedule lock, then release it briefly, then grab
it again for the context switch.

Two things related to irqs and the schedule / context-switch path.  One
we've already covered: one is calling vcpu_wake from within an interrupt
context.  The second is what might be called the "idle race": we need
interrupts disabled from the time we last check for softirqs until we
actually return to user mode.  But that's only a few dozen instructions
in most cases.

It might be possible to break things down into two locks -- one for
general schedule data structures, which would not be allowed to be
called from within an interrupt context, and one specifically to be used
for vcpu_wake (i.e., protecting manipulations to the actual runqueue)
which would have to be called with interrupts off.  But the generic
scheduling framework might make that a bit more tricky to get right.

 -George