diff mbox series

[v5,09/13] xen/spinlock: split recursive spinlocks from normal ones

Message ID 20240314072029.16937-10-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen/spinlock: make recursive spinlocks a dedicated type | expand

Commit Message

Jürgen Groß March 14, 2024, 7:20 a.m. UTC
Recursive and normal spinlocks are sharing the same data structure for
representation of the lock. This has two major disadvantages:

- it is not clear from the definition of a lock, whether it is intended
  to be used recursive or not, while a mixture of both usage variants
  needs to be

- in production builds (builds without CONFIG_DEBUG_LOCKS) the needed
  data size of an ordinary spinlock is 8 bytes instead of 4, due to the
  additional recursion data needed (associated with that the rwlock
  data is using 12 instead of only 8 bytes)

Fix that by introducing a struct spinlock_recursive for recursive
spinlocks only, and switch recursive spinlock functions to require
pointers to this new struct.

This allows to check the correct usage at build time.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use shorter names (Jan Beulich)
- don't embed spinlock_t in rspinlock_t (Jan Beulich)
V5:
- some style fixes (Jan Beulich)
- bool instead of int (Jan Beulich)
---
 xen/common/spinlock.c      | 50 ++++++++++++++++++++++++++
 xen/include/xen/spinlock.h | 72 +++++++++++++++++++++++++++++---------
 2 files changed, 105 insertions(+), 17 deletions(-)

Comments

Jan Beulich March 18, 2024, 2:59 p.m. UTC | #1
On 14.03.2024 08:20, Juergen Gross wrote:
> Recursive and normal spinlocks are sharing the same data structure for
> representation of the lock. This has two major disadvantages:
> 
> - it is not clear from the definition of a lock, whether it is intended
>   to be used recursive or not, while a mixture of both usage variants
>   needs to be
> 
> - in production builds (builds without CONFIG_DEBUG_LOCKS) the needed
>   data size of an ordinary spinlock is 8 bytes instead of 4, due to the
>   additional recursion data needed (associated with that the rwlock
>   data is using 12 instead of only 8 bytes)
> 
> Fix that by introducing a struct spinlock_recursive for recursive
> spinlocks only, and switch recursive spinlock functions to require
> pointers to this new struct.
> 
> This allows to check the correct usage at build time.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff mbox series

Patch

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index c3f2f9b209..a88ad9b93c 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -541,6 +541,56 @@  void _rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
     local_irq_restore(flags);
 }
 
+bool _nrspin_trylock(rspinlock_t *lock)
+{
+    check_lock(&lock->debug, true);
+
+    if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
+        return false;
+
+    return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
+void _nrspin_lock(rspinlock_t *lock)
+{
+    spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
+                     NULL);
+}
+
+void _nrspin_unlock(rspinlock_t *lock)
+{
+    spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
+void _nrspin_lock_irq(rspinlock_t *lock)
+{
+    ASSERT(local_irq_is_enabled());
+    local_irq_disable();
+    _nrspin_lock(lock);
+}
+
+void _nrspin_unlock_irq(rspinlock_t *lock)
+{
+    _nrspin_unlock(lock);
+    local_irq_enable();
+}
+
+unsigned long _nrspin_lock_irqsave(rspinlock_t *lock)
+{
+    unsigned long flags;
+
+    local_irq_save(flags);
+    _nrspin_lock(lock);
+
+    return flags;
+}
+
+void _nrspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
+{
+    _nrspin_unlock(lock);
+    local_irq_restore(flags);
+}
+
 #ifdef CONFIG_DEBUG_LOCK_PROFILE
 
 struct lock_profile_anc {
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 7dd11faab3..181e5c7d35 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -77,8 +77,6 @@  union lock_debug { };
 */
 
 struct spinlock;
-/* Temporary hack until a dedicated struct rspinlock is existing. */
-#define rspinlock spinlock
 
 struct lock_profile {
     struct lock_profile *next;       /* forward link */
@@ -110,6 +108,10 @@  struct lock_profile_qhead {
     __used_section(".lockprofile.data") =                                     \
     &lock_profile_data__##name
 #define SPIN_LOCK_UNLOCKED_(x) {                                              \
+    .debug = LOCK_DEBUG_,                                                     \
+    .profile = x,                                                             \
+}
+#define RSPIN_LOCK_UNLOCKED_(x) {                                             \
     .recurse_cpu = SPINLOCK_NO_CPU,                                           \
     .debug = LOCK_DEBUG_,                                                     \
     .profile = x,                                                             \
@@ -119,8 +121,9 @@  struct lock_profile_qhead {
     spinlock_t l = SPIN_LOCK_UNLOCKED_(NULL);                                 \
     static struct lock_profile lock_profile_data__##l = LOCK_PROFILE_(l);     \
     LOCK_PROFILE_PTR_(l)
+#define RSPIN_LOCK_UNLOCKED RSPIN_LOCK_UNLOCKED_(NULL)
 #define DEFINE_RSPINLOCK(l)                                                   \
-    rspinlock_t l = SPIN_LOCK_UNLOCKED_(NULL);                                \
+    rspinlock_t l = RSPIN_LOCK_UNLOCKED_(NULL);                               \
     static struct lock_profile lock_profile_data__##l = RLOCK_PROFILE_(l);    \
     LOCK_PROFILE_PTR_(l)
 
@@ -145,8 +148,11 @@  struct lock_profile_qhead {
 
 #define spin_lock_init_prof(s, l)                                             \
     spin_lock_init_prof__(s, l, lock, spinlock_t, false)
-#define rspin_lock_init_prof(s, l)                                            \
-    spin_lock_init_prof__(s, l, rlock, rspinlock_t, true)
+#define rspin_lock_init_prof(s, l) do {                                       \
+        spin_lock_init_prof__(s, l, rlock, rspinlock_t, true);                \
+        (s)->l.recurse_cpu = SPINLOCK_NO_CPU;                                 \
+        (s)->l.recurse_cnt = 0;                                               \
+    } while (0)
 
 void _lock_profile_register_struct(
     int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
@@ -168,11 +174,14 @@  struct lock_profile_qhead { };
 struct lock_profile { };
 
 #define SPIN_LOCK_UNLOCKED {                                                  \
+    .debug = LOCK_DEBUG_,                                                     \
+}
+#define RSPIN_LOCK_UNLOCKED {                                                 \
     .recurse_cpu = SPINLOCK_NO_CPU,                                           \
     .debug = LOCK_DEBUG_,                                                     \
 }
 #define DEFINE_SPINLOCK(l) spinlock_t l = SPIN_LOCK_UNLOCKED
-#define DEFINE_RSPINLOCK(l) rspinlock_t l = SPIN_LOCK_UNLOCKED
+#define DEFINE_RSPINLOCK(l) rspinlock_t l = RSPIN_LOCK_UNLOCKED
 
 #define spin_lock_init_prof(s, l) spin_lock_init(&((s)->l))
 #define rspin_lock_init_prof(s, l) rspin_lock_init(&((s)->l))
@@ -193,6 +202,14 @@  typedef union {
 #define SPINLOCK_TICKET_INC { .head_tail = 0x10000, }
 
 typedef struct spinlock {
+    spinlock_tickets_t tickets;
+    union lock_debug debug;
+#ifdef CONFIG_DEBUG_LOCK_PROFILE
+    struct lock_profile *profile;
+#endif
+} spinlock_t;
+
+typedef struct rspinlock {
     spinlock_tickets_t tickets;
     uint16_t recurse_cpu:SPINLOCK_CPU_BITS;
 #define SPINLOCK_NO_CPU        ((1u << SPINLOCK_CPU_BITS) - 1)
@@ -203,12 +220,10 @@  typedef struct spinlock {
 #ifdef CONFIG_DEBUG_LOCK_PROFILE
     struct lock_profile *profile;
 #endif
-} spinlock_t;
-
-typedef spinlock_t rspinlock_t;
+} rspinlock_t;
 
 #define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
-#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
+#define rspin_lock_init(l) (*(l) = (rspinlock_t)RSPIN_LOCK_UNLOCKED)
 
 void _spin_lock(spinlock_t *lock);
 void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *data), void *data);
@@ -312,12 +327,35 @@  static always_inline void rspin_lock(rspinlock_t *lock)
 #define rspin_barrier(l)              _rspin_barrier(l)
 #define rspin_is_locked(l)            _rspin_is_locked(l)
 
-#define nrspin_trylock(l)    spin_trylock(l)
-#define nrspin_lock(l)       spin_lock(l)
-#define nrspin_unlock(l)     spin_unlock(l)
-#define nrspin_lock_irq(l)   spin_lock_irq(l)
-#define nrspin_unlock_irq(l) spin_unlock_irq(l)
-#define nrspin_lock_irqsave(l, f)      spin_lock_irqsave(l, f)
-#define nrspin_unlock_irqrestore(l, f) spin_unlock_irqrestore(l, f)
+bool _nrspin_trylock(rspinlock_t *lock);
+void _nrspin_lock(rspinlock_t *lock);
+#define nrspin_lock_irqsave(l, f)                               \
+    ({                                                          \
+        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
+        (f) = _nrspin_lock_irqsave(l);                         \
+        block_lock_speculation();                               \
+    })
+unsigned long _nrspin_lock_irqsave(rspinlock_t *lock);
+void _nrspin_unlock(rspinlock_t *lock);
+void _nrspin_lock_irq(rspinlock_t *lock);
+void _nrspin_unlock_irq(rspinlock_t *lock);
+void _nrspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
+
+static always_inline void nrspin_lock(rspinlock_t *lock)
+{
+    _nrspin_lock(lock);
+    block_lock_speculation();
+}
+
+static always_inline void nrspin_lock_irq(rspinlock_t *l)
+{
+    _nrspin_lock_irq(l);
+    block_lock_speculation();
+}
+
+#define nrspin_trylock(l)              lock_evaluate_nospec(_nrspin_trylock(l))
+#define nrspin_unlock(l)               _nrspin_unlock(l)
+#define nrspin_unlock_irqrestore(l, f) _nrspin_unlock_irqrestore(l, f)
+#define nrspin_unlock_irq(l)           _nrspin_unlock_irq(l)
 
 #endif /* __SPINLOCK_H__ */