[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.