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

Re: [PATCH v5 08/13] xen/spinlock: add missing rspin_is_locked() and rspin_barrier()



On 18.03.24 15:57, Jan Beulich wrote:
On 14.03.2024 08:20, Juergen Gross wrote:
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -395,14 +395,7 @@ static bool always_inline spin_is_locked_common(const 
spinlock_tickets_t *t)
int _spin_is_locked(const spinlock_t *lock)
  {
-    /*
-     * Recursive locks may be locked by another CPU, yet we return
-     * "false" here, making this function suitable only for use in
-     * ASSERT()s and alike.
-     */
-    return lock->recurse_cpu == SPINLOCK_NO_CPU
-           ? spin_is_locked_common(&lock->tickets)
-           : lock->recurse_cpu == smp_processor_id();
+    return spin_is_locked_common(&lock->tickets);
  }

The "only suitable for ASSERT()s and alike" part of the comment wants
to survive here, I think.

Why?

I could understand you asking for putting such a comment to spinlock.h
mentioning that any *_is_locked() variant isn't safe, but with
_spin_is_locked() no longer covering recursive locks the comment's reasoning
is no longer true.


@@ -465,6 +458,23 @@ void _spin_barrier(spinlock_t *lock)
      spin_barrier_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
  }
+bool _rspin_is_locked(const rspinlock_t *lock)
+{
+    /*
+     * Recursive locks may be locked by another CPU, yet we return
+     * "false" here, making this function suitable only for use in
+     * ASSERT()s and alike.
+     */
+    return lock->recurse_cpu == SPINLOCK_NO_CPU
+           ? spin_is_locked_common(&lock->tickets)
+           : lock->recurse_cpu == smp_processor_id();
+}

Here otoh I wonder if both the comment and the spin_is_locked_common()
part of the condition are actually correct. Oh, the latter needs
retaining as long as we have nrspin_*() functions, I suppose. But the
comment could surely do with improving a little - at the very least
"yet we return "false"" isn't quite right; minimally there's a "may"
missing.

If anything I guess the comment shouldn't gain a "may", but rather say
"Recursive locks may be locked by another CPU via rspin_lock() ..."


Juergen



 


Rackspace

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