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

[Jürgen Groß]

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