Re: [Xen-devel] [PATCH] rwlock: allow recursive read locking when already locked in write mode

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Feb 20, 2020 at 04:11:08PM +0100, Jan Beulich wrote:
> On 20.02.2020 15:38, Roger Pau Monné wrote:
> > On Thu, Feb 20, 2020 at 03:23:38PM +0100, Jürgen Groß wrote:
> >> On 20.02.20 15:11, Roger Pau Monné wrote:
> >>> On Thu, Feb 20, 2020 at 01:48:54PM +0100, Jan Beulich wrote:
> >>>> On 20.02.2020 13:02, Roger Pau Monne wrote:
> >>>>> @@ -166,7 +180,8 @@ static inline void _write_unlock(rwlock_t *lock)
> >>>>>        * If the writer field is atomic, it can be cleared directly.
> >>>>>        * Otherwise, an atomic subtraction will be used to clear it.
> >>>>>        */
> >>>>> -    atomic_sub(_QW_LOCKED, &lock->cnts);
> >>>>> +    ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
> >>>>> +    atomic_sub(_write_lock_val(), &lock->cnts);
> >>>>
> >>>> I think this would be more efficient with atomic_and(), not
> >>>> the least because of the then avoided smp_processor_id().
> >>>> Whether to mask off just _QW_WMASK or also the CPU number of
> >>>> the last write owner would need to be determined. But with
> >>>> using subtraction, in case of problems it'll likely be
> >>>> harder to understand what actually went on, from looking at
> >>>> the resulting state of the lock (this is in part a pre-
> >>>> existing problem, but gets worse with subtraction of CPU
> >>>> numbers).
> >>>
> >>> Right, a mask would be better. Right now both need to be cleared (the
> >>> LOCKED and the CPU fields) as there's code that relies on !lock->cnts
> >>> as a way to determine that the lock is not read or write locked. If we
> >>> left the CPU lying around those checks would need to be adjusted.
> >>
> >> In case you make _QR_SHIFT 16 it is possible to just write a 2-byte zero
> >> value for write_unlock() (like its possible to do so today using a
> >> single byte write).
> > 
> > That would limit the readers count to 65536, what do you think Jan?
> 
> If the recurse_cpu approach is considered bad, I think this would
> be acceptable. But of course you'll need to consult with the Arm
> guys regarding the correctness of such a "half" store in their
> memory model; I would hope this to be universally okay, but I'm
> not entirely certain.

I would like to get confirmation from the Arm folks, but I see Arm has
write_atomic and supports such operation against a uint16_t, so I
don't see why it wouldn't work.

Thanks, Roger.

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