On 14.12.2022 12:10, Juergen Gross wrote:
> On 14.12.22 11:39, Jan Beulich wrote:
>> On 10.09.2022 17:49, Juergen Gross wrote:
>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>> @@ -397,7 +397,7 @@ int p2m_pod_empty_cache(struct domain *d)
>>>
>>> /* After this barrier no new PoD activities can happen. */
>>> BUG_ON(!d->is_dying);
>>> - spin_barrier(&p2m->pod.lock.lock);
>>> + spin_barrier(&p2m->pod.lock.lock.lock);
>>
>> This is getting unwieldy as well, and ...
>>
>>> @@ -160,21 +165,30 @@ typedef union {
>>>
>>> typedef struct spinlock {
>>> spinlock_tickets_t tickets;
>>> - u16 recurse_cpu:SPINLOCK_CPU_BITS;
>>> -#define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
>>> -#define SPINLOCK_RECURSE_BITS (16 - SPINLOCK_CPU_BITS)
>>> - u16 recurse_cnt:SPINLOCK_RECURSE_BITS;
>>> -#define SPINLOCK_MAX_RECURSE ((1u << SPINLOCK_RECURSE_BITS) - 1)
>>> union lock_debug debug;
>>> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>>> struct lock_profile *profile;
>>> #endif
>>> } spinlock_t;
>>>
>>> +struct spinlock_recursive {
>>> + struct spinlock lock;
>>> + u16 recurse_cpu:SPINLOCK_CPU_BITS;
>>> +#define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
>>> +#define SPINLOCK_RECURSE_BITS (16 - SPINLOCK_CPU_BITS)
>>> + u16 recurse_cnt:SPINLOCK_RECURSE_BITS;
>>> +#define SPINLOCK_MAX_RECURSE ((1u << SPINLOCK_RECURSE_BITS) - 1)
>>> +};
>>
>> ... I'm not sure anyway it's a good idea to embed spinlock_t inside the
>> new struct. I'd prefer if non-optional fields were always at the same
>> position, and there's not going to be that much duplication if we went
>> with
>>
>> typedef struct spinlock {
>> spinlock_tickets_t tickets;
>> union lock_debug debug;
>> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>> struct lock_profile *profile;
>> #endif
>> } spinlock_t;
>>
>> typedef struct rspinlock {
>> spinlock_tickets_t tickets;
>> u16 recurse_cpu:SPINLOCK_CPU_BITS;
>> #define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
>> #define SPINLOCK_RECURSE_BITS (16 - SPINLOCK_CPU_BITS)
>> u16 recurse_cnt:SPINLOCK_RECURSE_BITS;
>> #define SPINLOCK_MAX_RECURSE ((1u << SPINLOCK_RECURSE_BITS) - 1)
>> union lock_debug debug;
>> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>> struct lock_profile *profile;
>> #endif
>> } rspinlock_t;
>>
>> This would also eliminate the size increase of recursive locks in
>> debug builds. And functions like spin_barrier() then also would
>> (have to) properly indicate what kind of lock they act on.
>
> You are aware that this would require to duplicate all the spinlock
> functions for the recursive spinlocks?
Well, to be honest I didn't really consider this aspect, but I think
that's a reasonable price to pay (with some de-duplication potential
if we wanted to), provided we want to go this route in the first place.
The latest with this aspect in mind I wonder whether we aren't better
off with the current state (the more that iirc Andrew thinks that we
should get rid of recursive locking altogether).
Jan
> I'm not strictly against this, but before going this route I think I
> should mention the implications.
>
>
> Juergen
