Re: [PATCH 1/2] xen/spinlocks: spin_trylock with interrupts off is always fine

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

On 30.10.20 15:59, Jan Beulich wrote:
> On 30.10.2020 15:24, Juergen Gross wrote:
> > Even if a spinlock was taken with interrupts on before calling
> > spin_trylock() with interrupts off is fine, as it can't block.
> >
> > Add a bool parameter "try" to check_lock() for handling this case.
> >
> > Remove the call of check_lock() from _spin_is_locked(), as it really
> > serves no purpose and it can even lead to false crashes, e.g. when
> > a lock was taken correctly with interrupts enabled and the call of
> > _spin_is_locked() happened with interrupts off. In case the lock is
> > taken with wrong interrupt flags this will be catched when taking
> > the lock.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit I guess ...
>
> > @@ -42,7 +42,13 @@ static void check_lock(union lock_debug *debug)
> >        *
> >        * To guard against this subtle bug we latch the IRQ safety of every
> >        * spinlock in the system, on first use.
> > +     *
> > +     * A spin_trylock() or spin_is_locked() with interrupts off is always
> > +     * fine, as those can't block and above deadlock scenario doesn't apply.
> >        */
> > +    if ( try && irq_safe )
> > +        return;
> ... the reference to spin_is_locked() here wants dropping,
> since ...
>
> > @@ -220,8 +226,6 @@ void _spin_unlock_irqrestore(spinlock_t *lock, unsigned 
> > long flags)
> >   
> >   int _spin_is_locked(spinlock_t *lock)
> >   {
> > -    check_lock(&lock->debug);
> ... you drop the call here?
Oh yes, this was a late modification and I didn't adapt the comment
accordingly. Thanks for spotting it.


Juergen