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

[Julien Grall <julien@xxxxxxx>]

Hi,

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.
>
> 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;
This patch is also bumping the number of recursion possible from 16 to 
256. It is not clear to me whether this was intended or you just wanted 
to use uint8_t because it was easy to use.
From above, I also see that we only need 3 bits:

> BUILD_BUG_ON(SPINLOCK_RECURSE_BITS < 3);

So I would consider to ...

>   #define SPINLOCK_MAX_RECURSE   ((1u << SPINLOCK_RECURSE_BITS) - 1)
... update SPINLOCK_MAX_RECURSE to 16 or at least explain why we want to 
allow up to 256 recursion.
Cheers,

--
Julien Grall