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:26, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 05:06:20PM +0100, Jan Beulich wrote:
>> On 19.02.2020 16:07, 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.
>>
>> But write pressure was never in question. My concern is with
>> how long it might take for all readers to drop their locks.
> 
> The only long running operation that takes the CPU maps read lock is
> microcode updating or livepatching, and since those are also started
> by a privileged domain I think it's safe. Any sane admin wouldn't do a
> CPU plug/unplug while updating microcode or doing a livepatching.

Ah, yes, and perhaps one can even imply that further users of
this lock would also be admin invoked (we'd have to watch out
for future violations of this principle).

Jan

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