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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.

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