On 18.03.24 15:43, Jan Beulich wrote:
On 14.03.2024 08:20, Juergen Gross wrote:
Instead of special casing rspin_lock_irqsave() and
rspin_unlock_irqrestore() for the console lock, add those functions
to spinlock handling and use them where needed.

Signed-off-by: Juergen Gross <[email protected]>

Reviewed-by: Jan Beulich <[email protected]>
with two remarks:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -475,15 +475,31 @@ void _rspin_lock(rspinlock_t *lock)
      lock->recurse_cnt++;
  }
+unsigned long _rspin_lock_irqsave(rspinlock_t *lock)
+{
+    unsigned long flags;
+
+    local_irq_save(flags);
+    _rspin_lock(lock);
+
+    return flags;
+}
+
  void _rspin_unlock(rspinlock_t *lock)
  {
      if ( likely(--lock->recurse_cnt == 0) )
      {
          lock->recurse_cpu = SPINLOCK_NO_CPU;
-        spin_unlock(lock);
+        _spin_unlock(lock);

This looks like an unrelated change. I think I can guess the purpose, but
it would be nice if such along-the-way changes could be mentioned in the
description.

I think it would be better to move that change to patch 3.


--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -272,7 +272,15 @@ static always_inline void spin_lock_if(bool condition, 
spinlock_t *l)
   */
  bool _rspin_trylock(rspinlock_t *lock);
  void _rspin_lock(rspinlock_t *lock);
+#define rspin_lock_irqsave(l, f)                                \
+    ({                                                          \
+        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
+        ((f) = _rspin_lock_irqsave(l));                         \

Perhaps in the context of another patch in the series I think I had
pointed out that the outer pair of parentheses is unnecessary in
constructs like this.

Oh, this one slipped through, sorry for that.

Will fix it in the next iteration.


Juergen

Reply via email to