Re: [PATCH] switch rangeset's lock to rwlock

[Keir Fraser <keir.xen@gmail.com>]

On 28/03/2011 09:23, "Jan Beulich" <JBeulich@novell.com> wrote:

> As I said in the description, the rangeset code is of general library
> kind, and hence shouldn't make assumptions on the sort of data
> stored in rangesets. While I agree that in many cases the read
> side critical section would be small, there can be exceptions.

Because of rangeset_report_ranges()? Well, we could equally say that it is
not nice to be executing a callback in a spinlock critical section. For
example, the __copy_to_guest_offset() in the current sole callback handler
will be invalid for any guest using xenpaging (when that works properly)
since paging work could be done in there. Not a problem right now of course,
not least because the callback is executed only for dom0.

For that one iterator function I'm sure we could devise a method for
executing callbacks with no lock held, without resorting to RCU. Again, this
would be preferable to using rwlocks.

> Using RCU in this kind of a leaf, independent of almost anything
> else library routine would seem odd.

Callers wouldn't see the RCU-ness.

Not that I'm arguing to make rangesets use RCU, or avoid holding a lock
during report_ranges callbacks. I think concurrency bottlenecks in that code
are a non-issue.

> The only reason to stay with spinlocks was imo if you indeed
> wanted to knock off rwlocks altogether, which would new seem
> contrary to your c/s 23099:612171ff82ea.

Well they're only used now in code that is outside my personal areas of
interest. I'd prefer more sensible locking strategies to be used, but I'm
not going to remove code from Xen as an alternative, nor am I going to do
the work myself. So rwlocks remain in their current limited uses.

>> I need to double check, but I believe we have only a couple of rwlock users
>> now, and none of the read-side critical sections are large, so in that case
>> I suggest we switch them to use spinlocks and kill our rwlock
>> implementation.
> 
> Indeed, there's only tmem without the two patches I had sent. One
> of the reasons for putting them together was to actually have some
> *active* users of rwlocks again, so the code wouldn't have too good
> chances to bit-rot.
> 
> Further, I've taken note of a few other locks that may be
> candidates for conversion to rwlocks (pcidevs_lock being the
> most prominent example), requiring closer inspection and possibly
> code adjustments other than the mere lock kind conversion.

There'd be a bigger win in fragmenting pcidevs_lock, I'm sure. It looks like
it would be easy to do, it covers all sorts of stuff at the moment.

 -- Keir

> Jan
>