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

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

On 18.03.24 16:44, Jan Beulich wrote:
> On 18.03.2024 16:31, Jürgen Groß wrote:
> > 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.
> Hmm. I guess there is a difference in expectations. To me, these
> functions in principle ought to report whether the lock is "owned",
> not just "locked by some CPU". They don't, hence why they may not be
> used for other than ASSERT()s.
Okay, thanks for clarification. I'll change the comment accordingly.


Juergen