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

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

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.

 -George