[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] rwlock: allow recursive read locking when already locked in write mode

On 20.02.20 15:11, Roger Pau Monné wrote:
On Thu, Feb 20, 2020 at 01:48:54PM +0100, Jan Beulich wrote:
On 20.02.2020 13:02, Roger Pau Monne wrote:
I've done some testing and at least the CPU down case is fixed now.
Posting early in order to get feedback on the approach taken.

Looks good, thanks, just a question and two comments:

--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -20,21 +20,30 @@ typedef struct {
  #define DEFINE_RWLOCK(l) rwlock_t l = RW_LOCK_UNLOCKED
  #define rwlock_init(l) (*(l) = (rwlock_t)RW_LOCK_UNLOCKED)
- * Writer states & reader shift and bias.
- *
- * Writer field is 8 bit to allow for potential optimisation, see
- * _write_unlock().
- */
-#define    _QW_WAITING  1               /* A writer is waiting     */
-#define    _QW_LOCKED   0xff            /* A writer holds the lock */
-#define    _QW_WMASK    0xff            /* Writer mask.*/
-#define    _QR_SHIFT    8               /* Reader count shift      */
+/* Writer states & reader shift and bias. */
+#define    _QW_WAITING  1                       /* A writer is waiting */
+#define    _QW_LOCKED   3                       /* A writer holds the lock */

Aiui things would work equally well if 2 was used here?

I think so, I left it as 3 because previously LOCKED would also
include WAITING, and I didn't want to change it in case I've missed
some code path that was relying on that.

+#define    _QW_WMASK    3                       /* Writer mask */
+#define    _QW_CPUSHIFT 2                       /* Writer CPU shift */
+#define    _QW_CPUMASK  0x3ffc                  /* Writer CPU mask */

At least on x86, the shift involved here is quite certainly
more expensive than using wider immediates on AND and CMP
resulting from the _QW_MASK-based comparisons. I'd therefore
like to suggest to put the CPU in the low 12 bits.

Hm right. The LOCKED and WAITING bits don't need shifting anyway.

Another option is to use the recurse_cpu field of the
associated spin lock: The field is used for recursive locks
only, and hence the only conflict would be with
_spin_is_locked(), which we don't (and in the future then
also shouldn't) use on this lock.

I looked into that also, but things get more complicated AFAICT, as it's
not possible to atomically fetch the state of the lock and the owner
CPU at the same time. Neither you could set the LOCKED bit and the CPU
at the same time.

@@ -166,7 +180,8 @@ static inline void _write_unlock(rwlock_t *lock)
       * If the writer field is atomic, it can be cleared directly.
       * Otherwise, an atomic subtraction will be used to clear it.
-    atomic_sub(_QW_LOCKED, &lock->cnts);
+    ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
+    atomic_sub(_write_lock_val(), &lock->cnts);

I think this would be more efficient with atomic_and(), not
the least because of the then avoided smp_processor_id().
Whether to mask off just _QW_WMASK or also the CPU number of
the last write owner would need to be determined. But with
using subtraction, in case of problems it'll likely be
harder to understand what actually went on, from looking at
the resulting state of the lock (this is in part a pre-
existing problem, but gets worse with subtraction of CPU

Right, a mask would be better. Right now both need to be cleared (the
LOCKED and the CPU fields) as there's code that relies on !lock->cnts
as a way to determine that the lock is not read or write locked. If we
left the CPU lying around those checks would need to be adjusted.

In case you make _QR_SHIFT 16 it is possible to just write a 2-byte zero
value for write_unlock() (like its possible to do so today using a
single byte write).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.