Re: [PATCH v5 12/13] xen/rwlock: raise the number of possible cpus

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 17:00, Jürgen Groß wrote:
> On 18.03.24 16:39, Jan Beulich wrote:
>> On 14.03.2024 08:20, Juergen Gross wrote:
>>> @@ -36,14 +36,16 @@ void queue_write_lock_slowpath(rwlock_t *lock);
>>>   
>>>   static inline bool _is_write_locked_by_me(unsigned int cnts)
>>>   {
>>> -    BUILD_BUG_ON(_QW_CPUMASK < NR_CPUS);
>>> +    BUILD_BUG_ON((_QW_CPUMASK + 1) < NR_CPUS);
>>> +    BUILD_BUG_ON(NR_CPUS * _QR_BIAS > INT_MAX);
>>>       return (cnts & _QW_WMASK) == _QW_LOCKED &&
>>>              (cnts & _QW_CPUMASK) == smp_processor_id();
>>>   }
>>>   
>>>   static inline bool _can_read_lock(unsigned int cnts)
>>>   {
>>> -    return !(cnts & _QW_WMASK) || _is_write_locked_by_me(cnts);
>>> +    return cnts <= INT_MAX &&
>>> +           (!(cnts & _QW_WMASK) || _is_write_locked_by_me(cnts));
>>>   }
>>
>> I view this as problematic: Code knowing that a write lock is being held
>> may invoke a function using read_trylock() and expect the lock to be
>> available there.
> 
> So you expect it to be fine that someone is using read_trylock() 32768 times
> recursively while holding a lock as a writer? Sure, I can change the 
> condition,
> but OTOH ...

Hmm, yes, the reader count (leaving aside nested read_trylock()) is zero
when the lock is held for writing. So yes, I agree the condition is fine,
but may I ask for a brief comment to this effect, for blind people like
me?

Jan