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 <jbeul...@suse.com>
Jan
