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

[Jürgen Groß <jgross@xxxxxxxx>]

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