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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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