Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2020 11:23, Roger Pau Monné wrote:
> On Thu, Feb 20, 2020 at 09:16:48AM +0100, Jan Beulich wrote:
>> On 19.02.2020 18:03, Jan Beulich wrote:
>>> On 19.02.2020 17:08, Roger Pau Monné wrote:
>>>> On Wed, Feb 19, 2020 at 03:07:14PM +0000, Andrew Cooper wrote:
>>>>> On 19/02/2020 14:57, Jan Beulich wrote:
>>>>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>>>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock 
>>>>>>>>>>> into a
>>>>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock 
>>>>>>>>>>> are
>>>>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>>>>>> favor of having the caller retry.
>>>>>>>>> Now that the contention between read-only users is reduced as those
>>>>>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>>>>>> blocking mode.
>>>>>>>> I'd agree if writers couldn't be starved by (many) readers.
>>>>>>> AFAICT from the rw lock implementation readers won't be able to pick
>>>>>>> the lock as soon as there's a writer waiting, which should avoid this
>>>>>>> starvation?
>>>>>>>
>>>>>>> You still need to wait for current readers to drop the lock, but no
>>>>>>> new readers would be able to lock it, which I think should givbe us
>>>>>>> enough fairness.
>>>>>> Ah, right, it was rather the other way around - back-to-back
>>>>>> writers can starve readers with our current implementation.
>>>>>>
>>>>>>> OTOH when using _trylock new readers can still pick
>>>>>>> the lock in read mode, and hence I think using blocking mode for
>>>>>>> writers is actually better, as you can assure that readers won't be
>>>>>>> able to starve writers.
>>>>>> This is a good point. Nevertheless I remain unconvinced that
>>>>>> the change is warranted given the original intentions (as far
>>>>>> as we're able to reconstruct them). If the current behavior
>>>>>> gets in the way of sensible shim operation, perhaps the
>>>>>> behavior should be made dependent upon running in shim mode?
>>>>>
>>>>> Hotplug isn't generally used at all, so there is 0 write pressure on the
>>>>> lock.
>>>>>
>>>>> When it is used, it is all at explicit request from the controlling
>>>>> entity in the system (hardware domain, or singleton shim domain).
>>>>>
>>>>> If that entity is trying to DoS you, you've already lost.
>>>>>
>>>>> There might be attempts to justify why the locking was done like that in
>>>>> the first place, but it doesn't mean they were necessarily correct to
>>>>> being with, and they don't match up with the realistic usage of the lock.
>>>>
>>>> I'm happy to rewrite the commit message in order to include the
>>>> discussion here. What about adding:
>>>>
>>>> Note that when using rw locks a writer wanting to take the lock will
>>>> prevent further reads from locking it, hence preventing readers from
>>>> starving writers. Writers OTOH could starve readers, but since the
>>>> lock is only picked in write mode by actions requested by privileged
>>>> domains such entities already have the ability to DoS the hypervisor
>>>> in many other ways.
>>>
>>> While this sounds fine, my primary request was more towards removing
>>> (or at least making less scary) the part about deadlocks.
>>
>> Actually, having thought about this some more over night, I'm fine
>> with the mentioning of the deadlock scenario as you have it right now.
>> I'm not overly fussed as to the addition (or not) of the above extra
>> paragraph.
> 
> Up to you, I don't have a strong opinion.
> 
> AFAICT there's no need for me to resend then?

Indeed, but I wouldn't want to apply this one until the regression
from patch 1 was fixed, as else that change may also still get
reverted. But I'll keep it queued for committing until such time.

Jan

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