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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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.

In principle, without any nrspin_*() functions, the result here ought
to be usable generally, not just for ASSERT()s. Whether having its
and _spin_is_locked()'s behavior differ would be a good idea is a
separate question, of course.

Jan