Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()

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

On 18.03.24 15:43, Jan Beulich wrote:
> On 14.03.2024 08:20, Juergen Gross wrote:
> > Instead of special casing rspin_lock_irqsave() and
> > rspin_unlock_irqrestore() for the console lock, add those functions
> > to spinlock handling and use them where needed.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with two remarks:
>
> > --- a/xen/common/spinlock.c
> > +++ b/xen/common/spinlock.c
> > @@ -475,15 +475,31 @@ void _rspin_lock(rspinlock_t *lock)
> >       lock->recurse_cnt++;
> >   }
> >   
> > +unsigned long _rspin_lock_irqsave(rspinlock_t *lock)
> > +{
> > +    unsigned long flags;
> > +
> > +    local_irq_save(flags);
> > +    _rspin_lock(lock);
> > +
> > +    return flags;
> > +}
> > +
> >   void _rspin_unlock(rspinlock_t *lock)
> >   {
> >       if ( likely(--lock->recurse_cnt == 0) )
> >       {
> >           lock->recurse_cpu = SPINLOCK_NO_CPU;
> > -        spin_unlock(lock);
> > +        _spin_unlock(lock);
> This looks like an unrelated change. I think I can guess the purpose, but
> it would be nice if such along-the-way changes could be mentioned in the
> description.
I think it would be better to move that change to patch 3.

> > --- a/xen/include/xen/spinlock.h
> > +++ b/xen/include/xen/spinlock.h
> > @@ -272,7 +272,15 @@ static always_inline void spin_lock_if(bool condition, 
> > spinlock_t *l)
> >    */
> >   bool _rspin_trylock(rspinlock_t *lock);
> >   void _rspin_lock(rspinlock_t *lock);
> > +#define rspin_lock_irqsave(l, f)                                \
> > +    ({                                                          \
> > +        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
> > +        ((f) = _rspin_lock_irqsave(l));                         \
> Perhaps in the context of another patch in the series I think I had
> pointed out that the outer pair of parentheses is unnecessary in
> constructs like this.
Oh, this one slipped through, sorry for that.

Will fix it in the next iteration.


Juergen