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

From: Julien Grall <julien@xen.org>
To: Juergen Gross <jgross@suse.com>, xen-devel@lists.xenproject.org
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v4 12/12] xen/spinlock: support higher number of cpus
Date: Tue, 12 Dec 2023 14:04:48 +0000	[thread overview]
Message-ID: <4afe81fd-f5b7-4ddb-8782-38c98d8b3076@xen.org> (raw)
In-Reply-To: <9f1f73e6-2ade-440f-aed4-df46be62f3a5@suse.com>

On 12/12/2023 13:08, Juergen Gross wrote:
> 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@suse.com>
>>> ---
>>>   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));

It sounds good to me.

Cheers,

-- 
Julien Grall