Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 12/12/2023 09:47, Juergen Gross wrote:
> Allow 16 bits per cpu number, which is the limit imposed by
> spinlock_tickets_t.
>
> This will allow up to 65535 cpus, while increasing only the size of
> recursive spinlocks in debug builds from 8 to 12 bytes.
Looking at arch/Kconfig, it looks like we are limiting NR_CPUS to 
maximum 4096. So can you outline why we need this?
Just to be clear is I am not against this change, but alone it seems a 
little bit odd to increase the size in debug when that limit can never 
be reached (at least today).
Cheers,

> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>   xen/common/spinlock.c      |  1 +
>   xen/include/xen/spinlock.h | 18 +++++++++---------
>   2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 296bcf33e6..ae7c7c2086 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -481,6 +481,7 @@ int rspin_trylock(rspinlock_t *lock)
>   
>       /* Don't allow overflow of recurse_cpu field. */
>       BUILD_BUG_ON(NR_CPUS > SPINLOCK_NO_CPU);
> +    BUILD_BUG_ON(SPINLOCK_CPU_BITS > sizeof(lock->recurse_cpu) * 8);
>       BUILD_BUG_ON(SPINLOCK_RECURSE_BITS < 3);
>   
>       check_lock(&lock->debug, true);
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index 87946965b2..d720778cc1 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -7,16 +7,16 @@
>   #include <asm/system.h>
>   #include <asm/spinlock.h>
>   
> -#define SPINLOCK_CPU_BITS  12
> +#define SPINLOCK_CPU_BITS  16
>   
>   #ifdef CONFIG_DEBUG_LOCKS
>   union lock_debug {
> -    uint16_t val;
> -#define LOCK_DEBUG_INITVAL 0xffff
> +    uint32_t val;
> +#define LOCK_DEBUG_INITVAL 0xffffffff
>       struct {
> -        uint16_t cpu:SPINLOCK_CPU_BITS;
> -#define LOCK_DEBUG_PAD_BITS (14 - SPINLOCK_CPU_BITS)
> -        uint16_t :LOCK_DEBUG_PAD_BITS;
> +        uint32_t cpu:SPINLOCK_CPU_BITS;
> +#define LOCK_DEBUG_PAD_BITS (30 - SPINLOCK_CPU_BITS)
> +        uint32_t :LOCK_DEBUG_PAD_BITS;
>           bool irq_safe:1;
>           bool unseen:1;
>       };
> @@ -210,10 +210,10 @@ typedef struct spinlock {
>   
>   typedef struct rspinlock {
>       spinlock_tickets_t tickets;
> -    uint16_t recurse_cpu:SPINLOCK_CPU_BITS;
> +    uint16_t recurse_cpu;
>   #define SPINLOCK_NO_CPU        ((1u << SPINLOCK_CPU_BITS) - 1)
> -#define SPINLOCK_RECURSE_BITS  (16 - SPINLOCK_CPU_BITS)
> -    uint16_t recurse_cnt:SPINLOCK_RECURSE_BITS;
> +#define SPINLOCK_RECURSE_BITS  8
> +    uint8_t recurse_cnt;
>   #define SPINLOCK_MAX_RECURSE   ((1u << SPINLOCK_RECURSE_BITS) - 1)
>       union lock_debug debug;
>   #ifdef CONFIG_DEBUG_LOCK_PROFILE
--
Julien Grall