Re: [PATCH v4 13/13] riscv: Add qspinlock support

[Andrew Jones <ajones@ventanamicro.com>]

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@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@kernel.org>
> > > Signed-off-by: Alexandre Ghiti <alexghiti@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.

> 
> >
> > > +
> > > +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.

Thanks,
drew