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

[Juergen Gross <jgross@xxxxxxxx>]

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.

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).


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature