[PATCH v4 13/13] riscv: Add qspinlock support
Alexandre Ghiti
alex at ghiti.fr
Fri Aug 2 01:31:12 PDT 2024
Hi Drew,
On 01/08/2024 09:48, Andrew Jones wrote:
> On Thu, Aug 01, 2024 at 08:53:06AM GMT, Alexandre Ghiti wrote:
>> On Wed, Jul 31, 2024 at 5:29 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>>> On Wed, Jul 31, 2024 at 09:24:05AM GMT, Alexandre Ghiti wrote:
>>>> In order to produce a generic kernel, a user can select
>>>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
>>>> spinlock implementation if Zabha or Ziccrse are not present.
>>>>
>>>> Note that we can't use alternatives here because the discovery of
>>>> extensions is done too late and we need to start with the qspinlock
>>>> implementation because the ticket spinlock implementation would pollute
>>>> the spinlock value, so let's use static keys.
>>>>
>>>> This is largely based on Guo's work and Leonardo reviews at [1].
>>>>
>>>> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
>>>> Signed-off-by: Guo Ren <guoren at kernel.org>
>>>> Signed-off-by: Alexandre Ghiti <alexghiti at rivosinc.com>
>>>> ---
>>>> .../locking/queued-spinlocks/arch-support.txt | 2 +-
>>>> arch/riscv/Kconfig | 29 +++++++++++++
>>>> arch/riscv/include/asm/Kbuild | 4 +-
>>>> arch/riscv/include/asm/spinlock.h | 43 +++++++++++++++++++
>>>> arch/riscv/kernel/setup.c | 38 ++++++++++++++++
>>>> include/asm-generic/qspinlock.h | 2 +
>>>> include/asm-generic/ticket_spinlock.h | 2 +
>>>> 7 files changed, 118 insertions(+), 2 deletions(-)
>>>> create mode 100644 arch/riscv/include/asm/spinlock.h
>>>>
>>>> diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
>>>> index 22f2990392ff..cf26042480e2 100644
>>>> --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
>>>> +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
>>>> @@ -20,7 +20,7 @@
>>>> | openrisc: | ok |
>>>> | parisc: | TODO |
>>>> | powerpc: | ok |
>>>> - | riscv: | TODO |
>>>> + | riscv: | ok |
>>>> | s390: | TODO |
>>>> | sh: | TODO |
>>>> | sparc: | ok |
>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>> index ef55ab94027e..c9ff8081efc1 100644
>>>> --- a/arch/riscv/Kconfig
>>>> +++ b/arch/riscv/Kconfig
>>>> @@ -79,6 +79,7 @@ config RISCV
>>>> select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
>>>> select ARCH_WANTS_NO_INSTR
>>>> select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
>>>> + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
>>> Why do we need this? Also, we presumably would prefer not to have it
>>> when we end up using ticket spinlocks when combo spinlocks is selected.
>>> Is there no way to avoid it?
>> I'll let Andrea answer this as he asked for it.
>>
>>>> select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
>>>> select BUILDTIME_TABLE_SORT if MMU
>>>> select CLINT_TIMER if RISCV_M_MODE
>>>> @@ -488,6 +489,34 @@ config NODES_SHIFT
>>>> Specify the maximum number of NUMA Nodes available on the target
>>>> system. Increases memory reserved to accommodate various tables.
>>>>
>>>> +choice
>>>> + prompt "RISC-V spinlock type"
>>>> + default RISCV_COMBO_SPINLOCKS
>>>> +
>>>> +config RISCV_TICKET_SPINLOCKS
>>>> + bool "Using ticket spinlock"
>>>> +
>>>> +config RISCV_QUEUED_SPINLOCKS
>>>> + bool "Using queued spinlock"
>>>> + depends on SMP && MMU && NONPORTABLE
>>>> + select ARCH_USE_QUEUED_SPINLOCKS
>>>> + help
>>>> + The queued spinlock implementation requires the forward progress
>>>> + guarantee of cmpxchg()/xchg() atomic operations: CAS with Zabha or
>>>> + LR/SC with Ziccrse provide such guarantee.
>>>> +
>>>> + Select this if and only if Zabha or Ziccrse is available on your
>>>> + platform.
>>> Maybe some text recommending combo spinlocks here? As it stands it sounds
>>> like enabling queued spinlocks is a bad idea for anybody that doesn't know
>>> what platforms will run the kernel they're building, which is all distros.
>> That's NONPORTABLE, so people enabling this config are supposed to
>> know that right?
> Yes, both the NONPORTABLE and the scary text will imply that qspinlocks
> shouldn't be selected. I'm asking for text which points people configuring
> kernels to COMBO. Something like
>
> qspinlocks provides performance enhancements on platforms which support
> Zabha or Ziccrse. RISCV_QUEUED_SPINLOCKS should not be selected for
> platforms without one of those extensions. If unsure, select
> RISCV_COMBO_SPINLOCKS, which will use qspinlocks when supported and
> otherwise ticket spinlocks.
Ok I'll add that.
>
>>>> +
>>>> +config RISCV_COMBO_SPINLOCKS
>>>> + bool "Using combo spinlock"
>>>> + depends on SMP && MMU
>>>> + select ARCH_USE_QUEUED_SPINLOCKS
>>>> + help
>>>> + Embed both queued spinlock and ticket lock so that the spinlock
>>>> + implementation can be chosen at runtime.
>>> nit: Add a blank line here
>> Done
>>
>>>> +endchoice
>>>> +
>>>> config RISCV_ALTERNATIVE
>>>> bool
>>>> depends on !XIP_KERNEL
>>>> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
>>>> index 5c589770f2a8..1c2618c964f0 100644
>>>> --- a/arch/riscv/include/asm/Kbuild
>>>> +++ b/arch/riscv/include/asm/Kbuild
>>>> @@ -5,10 +5,12 @@ syscall-y += syscall_table_64.h
>>>> generic-y += early_ioremap.h
>>>> generic-y += flat.h
>>>> generic-y += kvm_para.h
>>>> +generic-y += mcs_spinlock.h
>>>> generic-y += parport.h
>>>> -generic-y += spinlock.h
>>>> generic-y += spinlock_types.h
>>>> +generic-y += ticket_spinlock.h
>>>> generic-y += qrwlock.h
>>>> generic-y += qrwlock_types.h
>>>> +generic-y += qspinlock.h
>>>> generic-y += user.h
>>>> generic-y += vmlinux.lds.h
>>>> diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
>>>> new file mode 100644
>>>> index 000000000000..503aef31db83
>>>> --- /dev/null
>>>> +++ b/arch/riscv/include/asm/spinlock.h
>>>> @@ -0,0 +1,43 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +
>>>> +#ifndef __ASM_RISCV_SPINLOCK_H
>>>> +#define __ASM_RISCV_SPINLOCK_H
>>>> +
>>>> +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
>>>> +#define _Q_PENDING_LOOPS (1 << 9)
>>>> +
>>>> +#define __no_arch_spinlock_redefine
>>>> +#include <asm/ticket_spinlock.h>
>>>> +#include <asm/qspinlock.h>
>>>> +#include <asm/alternative.h>
>>> We need asm/jump_label.h instead of asm/alternative.h, but...
>>>
>>>> +
>>>> +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
>>>> +
>>>> +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
>>>> +static __always_inline type arch_spin_##op(type_lock lock) \
>>>> +{ \
>>>> + if (static_branch_unlikely(&qspinlock_key)) \
>>>> + return queued_spin_##op(lock); \
>>>> + return ticket_spin_##op(lock); \
>>>> +}
>>> ...do you know what impact this inlined static key check has on the
>>> kernel size?
>> No, I'll check, thanks.
>>
>>> Actually, why not use ALTERNATIVE with any nonzero cpufeature value.
>>> Then add code to riscv_cpufeature_patch_check() to return true when
>>> qspinlocks should be enabled (based on the value of some global set
>>> during riscv_spinlock_init)?
>> As discussed with Guo in the previous iteration, the patching of the
>> alternatives intervenes far after the first use of the spinlocks and
>> the ticket spinlock implementation pollutes the spinlock value, so
>> we'd have to unconditionally start with the qspinlock implementation
>> and after switch to the ticket implementation if not supported by the
>> platform. It works but it's dirty, I really don't like this hack.
>>
>> We could though:
>> - add an initial value to the alternatives (not sure it's feasible though)
>> - make the patching of alternatives happen sooner by parsing the isa
>> string sooner, either in DT or ACPI (I have a working PoC for very
>> early parsing of ACPI).
>>
>> I intend to do the latter as I think we should be aware of the
>> extensions sooner in the boot process, so I'll change that to the
>> alternatives when it's done. WDYT, any other idea?
> Yes, we'll likely want early patching for other extensions as well,
> so that's a good idea in general. Putting a TODO on this static key
> to be changed to an ALTERNATIVE later when possible sounds reasonable
> to me.
Great, I'll add a TODO.
Thanks,
Alex
>
> Thanks,
> drew
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
More information about the linux-riscv
mailing list