Re: [Xen-devel] [PATCH] xen/sched: fix locking in sched_tick_[suspend|resume]()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/4/19 3:43 PM, Jürgen Groß wrote:
> On 04.10.19 16:34, George Dunlap wrote:
>> On 10/4/19 3:24 PM, Jürgen Groß wrote:
>>> On 04.10.19 16:08, George Dunlap wrote:
>>>> On 10/4/19 7:40 AM, Juergen Gross wrote:
>>>>> sched_tick_suspend() and sched_tick_resume() should not call the
>>>>> scheduler specific timer handlers in case the cpu they are running on
>>>>> is just being moved to or from a cpupool.
>>>>>
>>>>> Use a new percpu lock for that purpose.
>>>>
>>>> Is there a reason we can't use the pcpu_schedule_lock() instead of
>>>> introducing a new one?  Sorry if this is obvious, but it's been a while
>>>> since I poked around this code.
>>>
>>> Lock contention would be higher especially with credit2 which is using a
>>> per-core or even per-socket lock. We don't care about other scheduling
>>> activity here, all we need is a guard against our per-cpu scheduler
>>> data being changed beneath our feet.
>>
>> Is this code really being called so often that we need to worry about
>> this level of contention?
> 
> Its called each time idle is entered and left again.
> 
> Especially with core scheduling there is a high probability of multiple
> cpus leaving idle at the same time and the per-scheduler lock being used
> in parallel already.

Hrm, that does sound pretty bad.

>> We already have a *lot* of locks; and in this case you're adding a
>> second lock which interacts with the per-scheduler cpu lock.  This just
>> seems like asking for trouble.
> 
> In which way does it interact with the per-scheduler cpu lock?
> 
>> I won't Nack the patch, but I don't think I would ack it without clear
>> evidence that the extra lock has a performance improvement that's worth
>> the cost of the extra complexity.
> 
> I think complexity is lower this way. Especially considering the per-
> scheduler lock changing with moving a cpu to or from a cpupool.

The key aspect of the per-scheduler lock is that once you hold it, the
pointer to the lock can't change.

After this patch, the fact remains that sometimes you need to grab one
lock, sometimes the other, and sometimes both.

And, tick_suspend() lives in the per-scheduler code.  Each scheduler has
to remember that tick_suspend and tick_resume hold a completely
different lock to the rest of the scheduling functions.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel