[v4,6/6] xen/rcu: add per-lock counter in debug builds

Commit Message

Jürgen Groß March 10, 2020, 7:28 a.m. UTC

Add a lock specific counter to rcu read locks in debug builds. This
allows to test for matching lock/unlock calls.

Signed-off-by: Juergen Gross <[e-mail]>
---
 xen/include/xen/rcupdate.h | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

Comments

Jan Beulich March 11, 2020, 12:14 p.m. UTC | #1

On 10.03.2020 08:28, Juergen Gross wrote:
> Add a lock specific counter to rcu read locks in debug builds. This
> allows to test for matching lock/unlock calls.

Similar checking doesn't exist for e.g. spin locks iirc, and hence
I think you want to spend the word on the "why" here.

Jan

Jürgen Groß March 11, 2020, 12:28 p.m. UTC | #2

On 11.03.20 13:14, Jan Beulich wrote:
> On 10.03.2020 08:28, Juergen Gross wrote:
>> Add a lock specific counter to rcu read locks in debug builds. This
>> allows to test for matching lock/unlock calls.
> 
> Similar checking doesn't exist for e.g. spin locks iirc, and hence
> I think you want to spend the word on the "why" here.

With spinlock debugging turned on there is such a check in rel_lock():
The locking cpu has to match and on unlock the locking cpu is set to
SPINLOCK_NO_CPU.

I can add something like:

"This will help top avoid cases like the one fixed by commit
  98ed1f43cc2c89 where different rcu read locks were referenced in the
  lock and unlock calls."


Juergen

Patch

diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
index 0f32b3c7d8..602b819d75 100644
--- a/xen/include/xen/rcupdate.h
+++ b/xen/include/xen/rcupdate.h
@@ -36,21 +36,33 @@ 
 #include <xen/spinlock.h>
 #include <xen/cpumask.h>
 #include <xen/percpu.h>
+#include <asm/atomic.h>
 
 #define __rcu
 
 #ifndef NDEBUG
+/* * Lock type for passing to rcu_read_{lock,unlock}. */
+struct _rcu_read_lock {
+    atomic_t cnt;
+};
+typedef struct _rcu_read_lock rcu_read_lock_t;
+#define DEFINE_RCU_READ_LOCK(x) rcu_read_lock_t x = { .cnt = ATOMIC_INIT(0) }
+#define RCU_READ_LOCK_INIT(x)   atomic_set(&(x)->cnt, 0)
+
 DECLARE_PER_CPU(unsigned int, rcu_lock_cnt);
 
-static inline void rcu_quiesce_disable(void)
+static inline void rcu_quiesce_disable(rcu_read_lock_t *lock)
 {
     this_cpu(rcu_lock_cnt)++;
+    atomic_inc(&lock->cnt);
     barrier();
 }
 
-static inline void rcu_quiesce_enable(void)
+static inline void rcu_quiesce_enable(rcu_read_lock_t *lock)
 {
     barrier();
+    ASSERT(atomic_read(&lock->cnt));
+    atomic_dec(&lock->cnt);
     this_cpu(rcu_lock_cnt)--;
 }
 
@@ -60,8 +72,17 @@  static inline bool rcu_quiesce_allowed(void)
 }
 
 #else
-static inline void rcu_quiesce_disable(void) { }
-static inline void rcu_quiesce_enable(void) { }
+/*
+ * Dummy lock type for passing to rcu_read_{lock,unlock}. Currently exists
+ * only to document the reason for rcu_read_lock() critical sections.
+ */
+struct _rcu_read_lock {};
+typedef struct _rcu_read_lock rcu_read_lock_t;
+#define DEFINE_RCU_READ_LOCK(x) rcu_read_lock_t x
+#define RCU_READ_LOCK_INIT(x)
+
+static inline void rcu_quiesce_disable(rcu_read_lock_t *lock) { }
+static inline void rcu_quiesce_enable(rcu_read_lock_t *lock) { }
 static inline bool rcu_quiesce_allowed(void)
 {
     return true;
@@ -88,15 +109,6 @@  struct rcu_head {
 int rcu_pending(int cpu);
 int rcu_needs_cpu(int cpu);
 
-/*
- * Dummy lock type for passing to rcu_read_{lock,unlock}. Currently exists
- * only to document the reason for rcu_read_lock() critical sections.
- */
-struct _rcu_read_lock {};
-typedef struct _rcu_read_lock rcu_read_lock_t;
-#define DEFINE_RCU_READ_LOCK(x) rcu_read_lock_t x
-#define RCU_READ_LOCK_INIT(x)
-
 /**
  * rcu_read_lock - mark the beginning of an RCU read-side critical section.
  *
@@ -125,7 +137,7 @@  typedef struct _rcu_read_lock rcu_read_lock_t;
  */
 static inline void rcu_read_lock(rcu_read_lock_t *lock)
 {
-    rcu_quiesce_disable();
+    rcu_quiesce_disable(lock);
 }
 
 /**
@@ -136,7 +148,7 @@  static inline void rcu_read_lock(rcu_read_lock_t *lock)
 static inline void rcu_read_unlock(rcu_read_lock_t *lock)
 {
     ASSERT(!rcu_quiesce_allowed());
-    rcu_quiesce_enable();
+    rcu_quiesce_enable(lock);
 }
 
 /*