Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

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

On Thu, Feb 20, 2020 at 08:57:33AM +0000, Julien Grall wrote:
> 
> 
> On 20/02/2020 08:36, Jan Beulich wrote:
> > On 20.02.2020 09:27, Jürgen Groß wrote:
> > > On 20.02.20 09:13, Jan Beulich wrote:
> > > > On 13.02.2020 12:32, Roger Pau Monne wrote:
> > > > > Most users of the cpu maps just care about the maps not changing while
> > > > > the lock is being held, but don't actually modify the maps.
> > > > > 
> > > > > Convert the lock into a rw lock, and take the lock in read mode in
> > > > > get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
> > > > > the contention around the lock, since plug and unplug operations that
> > > > > take the lock in write mode are not that common.
> > > > > 
> > > > > Note that the read lock can be taken recursively (as it's a shared
> > > > > lock), and hence will keep the same behavior as the previously used
> > > > > recursive lock. As for the write lock, it's only used by CPU
> > > > > plug/unplug operations, and the lock is never taken recursively in
> > > > > that case.
> > > > > 
> > > > > While there also change get_cpu_maps return type to bool.
> > > > > 
> > > > > Reported-by: Julien Grall <julien@xxxxxxx>
> > > > > Suggested-also-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > 
> > > > I'm afraid I can't see how offlining a CPU would now work.
> > > > Condensed to just the relevant calls, the sequence from
> > > > cpu_down() is
> > > > 
> > > > cpu_hotplug_begin() (i.e. lock taken in write mode)
> > > > stop_machine_run()
> > > > -> get_cpu_maps() (lock unavailable to readers)
> > > 
> > > I've already pointed that out in another thread. :-)
> > 
> > Oh, I didn't recall. Or else I wouldn't have committed the
> > patch in the first place.
> > 
> > > > Other than recursive spin locks, rw locks don't currently
> > > > have a concept of permitting in a reader when this CPU
> > > > already holds the lock in write mode. Hence I can't see
> > > > how the get_cpu_maps() above would now ever succeed. Am I
> > > > missing anything, or does the patch need reverting until
> > > > the read_trylock() got enhanced to cope with this?
> > > 
> > > I think this can be handled locally in get_cpu_maps() and
> > > cpu_hotplug_begin() with the use of a variable holding the cpu (or
> > > NR_CPUS) of the cpu holding the write lock. get_cpu_maps() can just
> > > succeed in case this variable contains smp_processor_id().
> > 
> > It could, yes. But this is a general shortcoming of our rw
> > lock implementation (and imo a trap waiting for others to
> > fall into as well), and hence I think it would better be
> > taken care of in a generic manner.
> I actually did fall into this trap last week when playing with the p2m code
> and the emulation code. The emulation code is grabbing the write lock very
> early (which I didn't initially spot) and I was trying to use the read lock
> in a subsequent caller quite deep in the stack.
> 
> So a generic manner to solve the problem here would be ideal.

Let me take a look, it doesn't seem very convoluted to adapt some of
the recursive logic to be used in a rw lock.

Roger.

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