Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions

[Julien Grall <julien@xxxxxxx>]

On 13/12/2023 06:17, Juergen Gross wrote:
> On 12.12.23 19:49, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 12/12/2023 09:47, Juergen Gross wrote:
> > > -#define spin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, 
> > > spinlock_t)
> > > -#define rspin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, 
> > > rspinlock_t)
> > > +#define spin_lock_init_prof(s, 
> > > l)                                             \
> > > +    __spin_lock_init_prof(s, l, lock, spinlock_t, 0)
> > > +#define rspin_lock_init_prof(s, 
> > > l)                                            \
> > > +    __spin_lock_init_prof(s, l, rlock, rspinlock_t, 1)
> > >   void _lock_profile_register_struct(
> > >       int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
> > > @@ -174,6 +179,7 @@ struct lock_profile_qhead { };
> > >   #endif
> > > +
> > Spurious change?
> Indeed, will remove it.
>
> > >   typedef union {
> > >       uint32_t head_tail;
> > >       struct {
> > > @@ -261,4 +267,12 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, 
> > > unsigned long flags);
> > >   /* Ensure a lock is quiescent between two critical operations. */
> > >   #define spin_barrier(l)               _spin_barrier(l)
> > > +#define nrspin_trylock(l)    spin_trylock(l)
> > > +#define nrspin_lock(l)       spin_lock(l)
> > > +#define nrspin_unlock(l)     spin_unlock(l)
> > > +#define nrspin_lock_irq(l)   spin_lock_irq(l)
> > > +#define nrspin_unlock_irq(l) spin_unlock_irq(l)
> > > +#define nrspin_lock_irqsave(l, f)      spin_lock_irqsave(l, f)
> > > +#define nrspin_unlock_irqrestore(l, f) spin_unlock_irqrestore(l, f)
> > There is a comment describing [r]spinlock but not this new variant. 
> > Can you add one?
> Okay.
>
> > That said, I know this is existing code, but I have to admit this is a 
> > bit unclear why we are allowing an recursive spinlock to be 
> > non-recursive. To me it sounds like we are making the typesafe not 
> > very safe because it doesn't guarantee that we are not mixing the call 
> > nrspin_* with rspin_*.
> This is the current API.
I know. This is why I wrote "I know this is existing code".

> If you have locked with nrspin_*, any rspin_* attempt on the same lock will
> spin until rspin_unlock (nrspin_* will not set recurse_cpu, but take the
> lock).
>
> If you have locked with rspin_*, any nrspin_* attempt on the same lock will
> spin, too.
>
> So I don't see any major problem regarding accidental misuse, which 
> wouldn't
> be visible by deadlocks (there is no silent misbehavior).
Right, so this will lead to a deadlock, which is my concern. If we were 
using rspinlock_* everywhere then the deadlock (at least in the case 
when you recurse) would in theory not be possible (unless you recurse 
too much).
My point here is to simplify the interface rather than providing because 
I don't really see the benefits of providing a non-recursive version for 
recursive spinlock.
Anyway as I said this is nothing new.

Cheers,

--
Julien Grall