Re: [PATCH 2/2] xen/spinlock: merge recurse_cpu and debug.cpu fields in struct spinlock

[Juergen Gross <jgross@xxxxxxxx>]

On 24.02.22 17:11, Jan Beulich wrote:
> On 24.02.2022 11:54, Juergen Gross wrote:
> > --- a/xen/arch/x86/mm/mm-locks.h
> > +++ b/xen/arch/x86/mm/mm-locks.h
> > @@ -42,7 +42,7 @@ static inline void mm_lock_init(mm_lock_t *l)
> >   
> >   static inline bool mm_locked_by_me(const mm_lock_t *l)
> >   {
> > -    return (l->lock.recurse_cpu == current->processor);
> > +    return (l->lock.data.cpu == current->processor);
> >   }
> I see a fair risk with this: Behavior will now differ between debug and
> non-debug builds. E.g. a livelock because of trying to acquire the same
> lock again would not be noticed in a debug build if the acquire is
> conditional upon this function's return value. I think this is the main
> reason behind having two separate field, despite the apparent redundancy.
You are aware that mm_locked_by_me() is used for recursive spinlocks
only?

> Nevertheless a few more comments in case I'm missing something.
>
> > @@ -81,19 +79,19 @@ static void check_barrier(spinlock_t *lock)
> >        * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
> >        * is clearly wrong, for the same reason outlined in check_lock() above.
> >        */
> > -    BUG_ON(!local_irq_is_enabled() && !lock->debug.irq_safe);
> > +    BUG_ON(!local_irq_is_enabled() && !lock->data.irq_safe);
> >   }
> >   
> >   static void got_lock(spinlock_t *lock)
> >   {
> > -    lock->debug.cpu = smp_processor_id();
> > +    lock->data.cpu = smp_processor_id();
> This assignment breaks ...
>
> > @@ -230,9 +228,9 @@ int _spin_is_locked(spinlock_t *lock)
> >        * "false" here, making this function suitable only for use in
> >        * ASSERT()s and alike.
> >        */
> > -    return lock->recurse_cpu == SPINLOCK_NO_CPU
> > +    return lock->data.cpu == SPINLOCK_NO_CPU
> ... the use of this field to distinguish recursively locked locks
> from "simple" ones. At the very least the comment needs updating,
> but ...
>
> >              ? lock->tickets.head != lock->tickets.tail
> ... in how far this is still a sensible check in debug builds is
> also questionable. The effect here certainly also deserves pointing
> out in the description.
Okay, will add something.

> > -           : lock->recurse_cpu == smp_processor_id();
> > +           : lock->data.cpu == smp_processor_id();
> >   }
> >   
> >   int _spin_trylock(spinlock_t *lock)
> > @@ -296,22 +294,24 @@ int _spin_trylock_recursive(spinlock_t *lock)
> >   {
> >       unsigned int cpu = smp_processor_id();
> >   
> > -    /* Don't allow overflow of recurse_cpu field. */
> > +    /* Don't allow overflow of cpu field. */
> >       BUILD_BUG_ON(NR_CPUS > SPINLOCK_NO_CPU);
> >       BUILD_BUG_ON(SPINLOCK_RECURSE_BITS < 3);
> >   
> >       check_lock(lock, true);
> >   
> > -    if ( likely(lock->recurse_cpu != cpu) )
> > +    if ( likely(lock->data.cpu != cpu) )
> >       {
> >           if ( !spin_trylock(lock) )
> >               return 0;
> > -        lock->recurse_cpu = cpu;
> > +#ifndef CONFIG_DEBUG_LOCKS
> > +        lock->data.cpu = cpu;
> > +#endif
> Maybe worth an ASSERT() in the #else case (and elsewhere as applicable)?
Okay.

> > --- a/xen/include/xen/spinlock.h
> > +++ b/xen/include/xen/spinlock.h
> > @@ -6,26 +6,34 @@
> >   #include <asm/spinlock.h>
> >   #include <asm/types.h>
> >   
> > -#define SPINLOCK_CPU_BITS  12
> > +#define SPINLOCK_CPU_BITS      12
> > +#define SPINLOCK_NO_CPU        ((1u << SPINLOCK_CPU_BITS) - 1)
> > +#define SPINLOCK_RECURSE_BITS  (16 - SPINLOCK_CPU_BITS)
> > +#define SPINLOCK_MAX_RECURSE   ((1u << SPINLOCK_RECURSE_BITS) - 1)
> > +#define SPINLOCK_PAD_BITS      (30 - SPINLOCK_CPU_BITS - SPINLOCK_RECURSE_BITS)
> >   
> > -#ifdef CONFIG_DEBUG_LOCKS
> > -union lock_debug {
> > -    uint16_t val;
> > -#define LOCK_DEBUG_INITVAL 0xffff
> > +typedef union {
> > +    u32 val;
> >       struct {
> > -        uint16_t cpu:SPINLOCK_CPU_BITS;
> > -#define LOCK_DEBUG_PAD_BITS (14 - SPINLOCK_CPU_BITS)
> > -        uint16_t :LOCK_DEBUG_PAD_BITS;
> > +        u32 cpu:SPINLOCK_CPU_BITS;
> > +        u32 recurse_cnt:SPINLOCK_RECURSE_BITS;
> > +        u32 pad:SPINLOCK_PAD_BITS;
> > +#ifdef CONFIG_DEBUG_LOCKS
> >           bool irq_safe:1;
> >           bool unseen:1;
> > +#define SPINLOCK_DEBUG_INITVAL 0xc0000000
> > +#else
> > +        u32 debug_pad:2;
> Prior to your change we had two well-formed uint16_t. You replace them by
> five new instances of the being-phased-out u32?
Oh, sorry. Will change to uint32_t.

> Also - do the two padding fields really need names?
I'm fine to drop them.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature