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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2020 16:20, Roger Pau Monné wrote:
> 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.

The operations individually being available doesn't mean that
a combination of 32-bit locked accesses and a 16-bit plain
store are going to produce a consistent result. Perhaps I'm
over-cautious here, but I've been bitten by a vaguely similar
issue on x86 back in the ticket lock (in Linux) days.

Jan

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