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

[Juergen Gross <jgross@xxxxxxxx>]

On 12.12.23 13:39, Julien Grall wrote:
> 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.
That was the case indeed.

>  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.
I think updating SPINLOCK_MAX_RECURSE to 15 (the current value) is fine,
probably with an additional

BUILD_BUG_ON(SPINLOCK_MAX_RECURSE > ((1u << SPINLOCK_RECURSE_BITS) - 1));


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature