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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

> -           : 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)?

> --- 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?

Also - do the two padding fields really need names?

Jan