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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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