Re: [PATCH v4 10/12] xen/spinlock: split recursive spinlocks from normal ones

[Jürgen Groß]

On 29.02.24 16:32, Jan Beulich wrote:

On 12.12.2023 10:47, Juergen Gross wrote:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -541,6 +541,55 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
long flags)
      local_irq_restore(flags);
  }

+int nrspin_trylock(rspinlock_t *lock)

+{
+    check_lock(&lock->debug, true);
+
+    if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
+        return 0;
+
+    return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}

I wonder if we shouldn't take the opportunity and stop having trylock
functions have "int" return type. Perhaps already spin_trylock_common()
when introduced could use "bool" instead, then here following suit.

Fine with me.

+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;

Nit: Strictly speaking we want a blank line ahead of this "return".

Okay, will add it.

@@ -166,11 +172,15 @@ struct lock_profile_qhead { };
  struct lock_profile { };

#define SPIN_LOCK_UNLOCKED { \

+    .debug =_LOCK_DEBUG,                                                      \
+}
+#define RSPIN_LOCK_UNLOCKED {                                                 \
+    .debug =_LOCK_DEBUG,                                                      \
      .recurse_cpu = SPINLOCK_NO_CPU,                                           
\
      .debug =_LOCK_DEBUG,                                                      
\
  }

Initializing .debug twice?

Oh, right. Will drop one.

@@ -180,7 +190,6 @@ struct lock_profile { };

#endif -

  typedef union {
      uint32_t head_tail;
      struct {

Looks like this might be undoing what the earlier patch isn't going to
do anymore?

Yes, seen it already.

@@ -242,6 +257,19 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
long flags);
  int rspin_is_locked(const rspinlock_t *lock);
  void rspin_barrier(rspinlock_t *lock);

+int nrspin_trylock(rspinlock_t *lock);

+void nrspin_lock(rspinlock_t *lock);
+void nrspin_unlock(rspinlock_t *lock);
+void nrspin_lock_irq(rspinlock_t *lock);
+void nrspin_unlock_irq(rspinlock_t *lock);
+#define nrspin_lock_irqsave(l, f)                               \
+    ({                                                          \
+        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
+        ((f) = __nrspin_lock_irqsave(l));                       \

I don't think the outer pair of parentheses is needed here. As to the
leading double underscores, see comments elsewhere.

Okay.


Juergen