Re: [PATCH v4 10/12] xen/spinlock: split recursive spinlocks from normal ones

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

On 29.02.24 16:32, Jan Beulich wrote:
> On 12.12.2023 10:47, Juergen Gross wrote:
> > --- a/xen/common/spinlock.c
> > +++ b/xen/common/spinlock.c
> > @@ -541,6 +541,55 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
> > long flags)
> >       local_irq_restore(flags);
> >   }
> >   
> > +int nrspin_trylock(rspinlock_t *lock)
> > +{
> > +    check_lock(&lock->debug, true);
> > +
> > +    if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
> > +        return 0;
> > +
> > +    return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
> > +}
> I wonder if we shouldn't take the opportunity and stop having trylock
> functions have "int" return type. Perhaps already spin_trylock_common()
> when introduced could use "bool" instead, then here following suit.
Fine with me.

> > +void nrspin_lock(rspinlock_t *lock)
> > +{
> > +    spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
> > +                     NULL);
> > +}
> > +
> > +void nrspin_unlock(rspinlock_t *lock)
> > +{
> > +    spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
> > +}
> > +
> > +void nrspin_lock_irq(rspinlock_t *lock)
> > +{
> > +    ASSERT(local_irq_is_enabled());
> > +    local_irq_disable();
> > +    nrspin_lock(lock);
> > +}
> > +
> > +void nrspin_unlock_irq(rspinlock_t *lock)
> > +{
> > +    nrspin_unlock(lock);
> > +    local_irq_enable();
> > +}
> > +
> > +unsigned long __nrspin_lock_irqsave(rspinlock_t *lock)
> > +{
> > +    unsigned long flags;
> > +
> > +    local_irq_save(flags);
> > +    nrspin_lock(lock);
> > +    return flags;
> Nit: Strictly speaking we want a blank line ahead of this "return".
Okay, will add it.

> > @@ -166,11 +172,15 @@ struct lock_profile_qhead { };
> >   struct lock_profile { };
> >   
> >   #define SPIN_LOCK_UNLOCKED {                                                  \
> > +    .debug =_LOCK_DEBUG,                                                      \
> > +}
> > +#define RSPIN_LOCK_UNLOCKED {                                                 \
> > +    .debug =_LOCK_DEBUG,                                                      \
> >       .recurse_cpu = SPINLOCK_NO_CPU,                                           
> > \
> >       .debug =_LOCK_DEBUG,                                                      
> > \
> >   }
> Initializing .debug twice?
Oh, right. Will drop one.

> > @@ -180,7 +190,6 @@ struct lock_profile { };
> >   
> >   #endif
> >   
> > -
> >   typedef union {
> >       uint32_t head_tail;
> >       struct {
> Looks like this might be undoing what the earlier patch isn't going to
> do anymore?
Yes, seen it already.

> > @@ -242,6 +257,19 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
> > long flags);
> >   int rspin_is_locked(const rspinlock_t *lock);
> >   void rspin_barrier(rspinlock_t *lock);
> >   
> > +int nrspin_trylock(rspinlock_t *lock);
> > +void nrspin_lock(rspinlock_t *lock);
> > +void nrspin_unlock(rspinlock_t *lock);
> > +void nrspin_lock_irq(rspinlock_t *lock);
> > +void nrspin_unlock_irq(rspinlock_t *lock);
> > +#define nrspin_lock_irqsave(l, f)                               \
> > +    ({                                                          \
> > +        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
> > +        ((f) = __nrspin_lock_irqsave(l));                       \
> I don't think the outer pair of parentheses is needed here. As to the
> leading double underscores, see comments elsewhere.
Okay.


Juergen