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

[Jürgen Groß <jgross@xxxxxxxx>]

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 */

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.


Juergen