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

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

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?

Thanks, Roger.

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