Re: [PATCH v6 7/8] xen/rwlock: raise the number of possible cpus

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.04.2024 17:29, Jürgen Groß wrote:
> On 02.04.24 16:52, Jan Beulich wrote:
>> On 27.03.2024 16:22, Juergen Gross wrote:
>>> @@ -36,14 +36,21 @@ 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);
>>> +    /*
>>> +     * If write locked by the caller, no other readers are possible.
>>> +     * Not allowing the lock holder to read_lock() another 32768 times 
>>> ought
>>> +     * to be fine.
>>> +     */
>>> +    return cnts <= INT_MAX &&
>>> +           (!(cnts & _QW_WMASK) || _is_write_locked_by_me(cnts));
>>>   }
>>
>> What is the 32768 in the comment relating to? INT_MAX is quite a bit higher,
>> yet the comparison against it is the only thing you add. Whereas the reader
>> count is, with the sign bit unused, 17 bits, though (bits 14..30). I think
> 
> You missed:
> 
> #define    _QR_SHIFT    (_QW_SHIFT + 2)         /* Reader count shift */

Oops. No I idea how I managed to skip this, when something like this was
exactly what I was expecting to find.

> So the reader's shift is 16, resulting in 15 bits for the reader count.
> 
>> even in such a comment rather than using a literal number the corresponding
>> expression would better be stated.
> 
> Hmm, you mean replacing the 32768 with INT_MAX >> _QR_SHIFT? This would be
> fine with me.

Happy to do so while committing, provided earlier patches get unblocked
first:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan