mbox series

[v2,00/13] xen/spinlock: make recursive spinlocks a dedicated type

Message ID 20231013094224.7060-1-jgross@suse.com (mailing list archive)
Headers show
Series xen/spinlock: make recursive spinlocks a dedicated type | expand

Message

Jürgen Groß Oct. 13, 2023, 9:42 a.m. UTC
Instead of being able to use normal spinlocks as recursive ones, too,
make recursive spinlocks a special lock type.

This will make the spinlock structure smaller in production builds and
add type-safety.

This allows to increase the maximum number of physical cpus from 8191
to 65535 without increasing the size of the lock structure in production
builds (the size of recursive spinlocks in debug builds will grow to
12 bytes due to that change).

Changes in V2:
- addressed comments by Jan Beulich
- lots of additional cleanups
- reorganized complete series

Juergen Gross (13):
  xen/spinlock: fix coding style issues
  xen/spinlock: reduce lock profile ifdefs
  xen/spinlock: make spinlock initializers more readable
  xen/spinlock: introduce new type for recursive spinlocks
  xen/spinlock: rename recursive lock functions
  xen/spinlock: add rspin_[un]lock_irq[save|restore]()
  xen/spinlock: make struct lock_profile rspinlock_t aware
  xen/spinlock: add explicit non-recursive locking functions
  xen/spinlock: add another function level
  xen/spinlock: add missing rspin_is_locked() and rspin_barrier()
  xen/spinlock: split recursive spinlocks from normal ones
  xen/spinlock: remove indirection through macros for spin_*() functions
  xen/spinlock: support higher number of cpus

 xen/arch/arm/domain.c         |   4 +-
 xen/arch/arm/mm.c             |   4 +-
 xen/arch/x86/domain.c         |  20 +--
 xen/arch/x86/include/asm/mm.h |   2 +-
 xen/arch/x86/mm.c             |  12 +-
 xen/arch/x86/mm/mem_sharing.c |  16 +-
 xen/arch/x86/mm/mm-locks.h    |   6 +-
 xen/arch/x86/mm/p2m-pod.c     |   6 +-
 xen/arch/x86/mm/p2m.c         |   4 +-
 xen/arch/x86/tboot.c          |   4 +-
 xen/arch/x86/traps.c          |  14 +-
 xen/common/domain.c           |   6 +-
 xen/common/domctl.c           |   4 +-
 xen/common/grant_table.c      |  10 +-
 xen/common/ioreq.c            |  54 +++----
 xen/common/memory.c           |   4 +-
 xen/common/numa.c             |   4 +-
 xen/common/page_alloc.c       |  30 ++--
 xen/common/spinlock.c         | 292 +++++++++++++++++++++++-----------
 xen/drivers/char/console.c    |  48 ++----
 xen/drivers/passthrough/pci.c |   8 +-
 xen/include/xen/console.h     |   5 +-
 xen/include/xen/sched.h       |  10 +-
 xen/include/xen/spinlock.h    | 176 ++++++++++++--------
 24 files changed, 447 insertions(+), 296 deletions(-)

Comments

Jan Beulich Oct. 19, 2023, 7:48 a.m. UTC | #1
On 13.10.2023 11:42, Juergen Gross wrote:
> Instead of being able to use normal spinlocks as recursive ones, too,
> make recursive spinlocks a special lock type.
> 
> This will make the spinlock structure smaller in production builds and
> add type-safety.
> 
> This allows to increase the maximum number of physical cpus from 8191
> to 65535 without increasing the size of the lock structure in production
> builds (the size of recursive spinlocks in debug builds will grow to
> 12 bytes due to that change).
> 
> Changes in V2:
> - addressed comments by Jan Beulich
> - lots of additional cleanups
> - reorganized complete series
> 
> Juergen Gross (13):
>   xen/spinlock: fix coding style issues
>   xen/spinlock: reduce lock profile ifdefs
>   xen/spinlock: make spinlock initializers more readable
>   xen/spinlock: introduce new type for recursive spinlocks
>   xen/spinlock: rename recursive lock functions
>   xen/spinlock: add rspin_[un]lock_irq[save|restore]()
>   xen/spinlock: make struct lock_profile rspinlock_t aware
>   xen/spinlock: add explicit non-recursive locking functions
>   xen/spinlock: add another function level
>   xen/spinlock: add missing rspin_is_locked() and rspin_barrier()
>   xen/spinlock: split recursive spinlocks from normal ones
>   xen/spinlock: remove indirection through macros for spin_*() functions
>   xen/spinlock: support higher number of cpus

Before looking at patches 4 and onwards, I'd like us to settle on the future
of recursive locking in Xen, considering in particular Andrew's objections
to their use in the code base. If the plan was to eliminate them, I'd see
little point in reworking the infrastructure. I'd like to suggest that one
of us tries to remember to put this up as an agenda item for the next
Community Call. But of course the discussion can also happen right here; I
merely expect there might not be much of a reaction.

Jan
Andrew Cooper Oct. 19, 2023, 9:35 a.m. UTC | #2
On 19/10/2023 8:48 am, Jan Beulich wrote:
> On 13.10.2023 11:42, Juergen Gross wrote:
>> Instead of being able to use normal spinlocks as recursive ones, too,
>> make recursive spinlocks a special lock type.
>>
>> This will make the spinlock structure smaller in production builds and
>> add type-safety.
>>
>> This allows to increase the maximum number of physical cpus from 8191
>> to 65535 without increasing the size of the lock structure in production
>> builds (the size of recursive spinlocks in debug builds will grow to
>> 12 bytes due to that change).
>>
>> Changes in V2:
>> - addressed comments by Jan Beulich
>> - lots of additional cleanups
>> - reorganized complete series
>>
>> Juergen Gross (13):
>>   xen/spinlock: fix coding style issues
>>   xen/spinlock: reduce lock profile ifdefs
>>   xen/spinlock: make spinlock initializers more readable
>>   xen/spinlock: introduce new type for recursive spinlocks
>>   xen/spinlock: rename recursive lock functions
>>   xen/spinlock: add rspin_[un]lock_irq[save|restore]()
>>   xen/spinlock: make struct lock_profile rspinlock_t aware
>>   xen/spinlock: add explicit non-recursive locking functions
>>   xen/spinlock: add another function level
>>   xen/spinlock: add missing rspin_is_locked() and rspin_barrier()
>>   xen/spinlock: split recursive spinlocks from normal ones
>>   xen/spinlock: remove indirection through macros for spin_*() functions
>>   xen/spinlock: support higher number of cpus
> Before looking at patches 4 and onwards, I'd like us to settle on the future
> of recursive locking in Xen, considering in particular Andrew's objections
> to their use in the code base. If the plan was to eliminate them, I'd see
> little point in reworking the infrastructure. I'd like to suggest that one
> of us tries to remember to put this up as an agenda item for the next
> Community Call. But of course the discussion can also happen right here; I
> merely expect there might not be much of a reaction.

Actually, I consider this series an improvement.  The CPU limit is the
most urgent problem to fix.

XenServer has just jumped to NR_CPUS=2k in order to support 2024's range
of hardware, and it's only going to be a couple of years more before
we're stuck given the current spinlocks.

I do genuinely think the code and logic would be better without
recursive locks, but making that happen is going to be very invasive and
complicated.

But in the meantime with spinlocks properly separated from recursive
locks, it becomes easier IMO to dissuade the introduction of new cases
while we unpick the existing ones.

And so what if we do end up deleting recursive locks in a few years
time?  That's not an argument against doing this untangling now.

~Andrew
Jan Beulich Oct. 19, 2023, 9:39 a.m. UTC | #3
On 19.10.2023 11:35, Andrew Cooper wrote:
> On 19/10/2023 8:48 am, Jan Beulich wrote:
>> On 13.10.2023 11:42, Juergen Gross wrote:
>>> Instead of being able to use normal spinlocks as recursive ones, too,
>>> make recursive spinlocks a special lock type.
>>>
>>> This will make the spinlock structure smaller in production builds and
>>> add type-safety.
>>>
>>> This allows to increase the maximum number of physical cpus from 8191
>>> to 65535 without increasing the size of the lock structure in production
>>> builds (the size of recursive spinlocks in debug builds will grow to
>>> 12 bytes due to that change).
>>>
>>> Changes in V2:
>>> - addressed comments by Jan Beulich
>>> - lots of additional cleanups
>>> - reorganized complete series
>>>
>>> Juergen Gross (13):
>>>   xen/spinlock: fix coding style issues
>>>   xen/spinlock: reduce lock profile ifdefs
>>>   xen/spinlock: make spinlock initializers more readable
>>>   xen/spinlock: introduce new type for recursive spinlocks
>>>   xen/spinlock: rename recursive lock functions
>>>   xen/spinlock: add rspin_[un]lock_irq[save|restore]()
>>>   xen/spinlock: make struct lock_profile rspinlock_t aware
>>>   xen/spinlock: add explicit non-recursive locking functions
>>>   xen/spinlock: add another function level
>>>   xen/spinlock: add missing rspin_is_locked() and rspin_barrier()
>>>   xen/spinlock: split recursive spinlocks from normal ones
>>>   xen/spinlock: remove indirection through macros for spin_*() functions
>>>   xen/spinlock: support higher number of cpus
>> Before looking at patches 4 and onwards, I'd like us to settle on the future
>> of recursive locking in Xen, considering in particular Andrew's objections
>> to their use in the code base. If the plan was to eliminate them, I'd see
>> little point in reworking the infrastructure. I'd like to suggest that one
>> of us tries to remember to put this up as an agenda item for the next
>> Community Call. But of course the discussion can also happen right here; I
>> merely expect there might not be much of a reaction.
> 
> Actually, I consider this series an improvement.  The CPU limit is the
> most urgent problem to fix.
> 
> XenServer has just jumped to NR_CPUS=2k in order to support 2024's range
> of hardware, and it's only going to be a couple of years more before
> we're stuck given the current spinlocks.
> 
> I do genuinely think the code and logic would be better without
> recursive locks, but making that happen is going to be very invasive and
> complicated.
> 
> But in the meantime with spinlocks properly separated from recursive
> locks, it becomes easier IMO to dissuade the introduction of new cases
> while we unpick the existing ones.
> 
> And so what if we do end up deleting recursive locks in a few years
> time?  That's not an argument against doing this untangling now.

Of course if that was happening only in a few years time, the series
here is still worthwhile to have. My question was rather towards us
possibly eliminating recursive locks in the next release cycle.

Jan
Andrew Cooper Oct. 19, 2023, 11:05 a.m. UTC | #4
On 19/10/2023 10:39 am, Jan Beulich wrote:
> On 19.10.2023 11:35, Andrew Cooper wrote:
>> On 19/10/2023 8:48 am, Jan Beulich wrote:
>>> On 13.10.2023 11:42, Juergen Gross wrote:
>>>> Instead of being able to use normal spinlocks as recursive ones, too,
>>>> make recursive spinlocks a special lock type.
>>>>
>>>> This will make the spinlock structure smaller in production builds and
>>>> add type-safety.
>>>>
>>>> This allows to increase the maximum number of physical cpus from 8191
>>>> to 65535 without increasing the size of the lock structure in production
>>>> builds (the size of recursive spinlocks in debug builds will grow to
>>>> 12 bytes due to that change).
>>>>
>>>> Changes in V2:
>>>> - addressed comments by Jan Beulich
>>>> - lots of additional cleanups
>>>> - reorganized complete series
>>>>
>>>> Juergen Gross (13):
>>>>   xen/spinlock: fix coding style issues
>>>>   xen/spinlock: reduce lock profile ifdefs
>>>>   xen/spinlock: make spinlock initializers more readable
>>>>   xen/spinlock: introduce new type for recursive spinlocks
>>>>   xen/spinlock: rename recursive lock functions
>>>>   xen/spinlock: add rspin_[un]lock_irq[save|restore]()
>>>>   xen/spinlock: make struct lock_profile rspinlock_t aware
>>>>   xen/spinlock: add explicit non-recursive locking functions
>>>>   xen/spinlock: add another function level
>>>>   xen/spinlock: add missing rspin_is_locked() and rspin_barrier()
>>>>   xen/spinlock: split recursive spinlocks from normal ones
>>>>   xen/spinlock: remove indirection through macros for spin_*() functions
>>>>   xen/spinlock: support higher number of cpus
>>> Before looking at patches 4 and onwards, I'd like us to settle on the future
>>> of recursive locking in Xen, considering in particular Andrew's objections
>>> to their use in the code base. If the plan was to eliminate them, I'd see
>>> little point in reworking the infrastructure. I'd like to suggest that one
>>> of us tries to remember to put this up as an agenda item for the next
>>> Community Call. But of course the discussion can also happen right here; I
>>> merely expect there might not be much of a reaction.
>> Actually, I consider this series an improvement.  The CPU limit is the
>> most urgent problem to fix.
>>
>> XenServer has just jumped to NR_CPUS=2k in order to support 2024's range
>> of hardware, and it's only going to be a couple of years more before
>> we're stuck given the current spinlocks.
>>
>> I do genuinely think the code and logic would be better without
>> recursive locks, but making that happen is going to be very invasive and
>> complicated.
>>
>> But in the meantime with spinlocks properly separated from recursive
>> locks, it becomes easier IMO to dissuade the introduction of new cases
>> while we unpick the existing ones.
>>
>> And so what if we do end up deleting recursive locks in a few years
>> time?  That's not an argument against doing this untangling now.
> Of course if that was happening only in a few years time, the series
> here is still worthwhile to have. My question was rather towards us
> possibly eliminating recursive locks in the next release cycle.

I find it unlikely that we'd manage to do it in the next release cycle,
even if we did dedicate a significant portion of effort to it.  There
are more urgent things too.

But the same argument works with several years substituted for months. 

~Andrew