Re: [PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK

Guo Ren
2023-09-14T03:32:18Z

On Thu, Sep 14, 2023 at 2:54 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear@fastmail.com wrote:
> > On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote:
> >> From: Guo Ren <guoren@linux.alibaba.com>
> >>
> >> According to qspinlock requirements, RISC-V gives out a weak LR/SC
> >> forward progress guarantee which does not satisfy qspinlock. But
> >> many vendors could produce stronger forward guarantee LR/SC to
> >> ensure the xchg_tail could be finished in time on any kind of
> >> hart. T-HEAD is the vendor which implements strong forward
> >> guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD
> >> with errata help.
> >>
> >> T-HEAD early version of processors has the merge buffer delay
> >> problem, so we need ERRATA_WRITEONCE to support qspinlock.
> >>
> >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> >> Signed-off-by: Guo Ren <guoren@kernel.org>
> >> ---
> >> arch/riscv/Kconfig.errata | 13 +++++++++++++
> >> arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++
> >> arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++
> >> arch/riscv/include/asm/vendorid_list.h | 3 ++-
> >> arch/riscv/kernel/cpufeature.c | 3 ++-
> >> 5 files changed, 61 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
> >> index 4745a5c57e7c..eb43677b13cc 100644
> >> --- a/arch/riscv/Kconfig.errata
> >> +++ b/arch/riscv/Kconfig.errata
> >> @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE
> >>
> >> If you don't know what to do here, say "Y".
> >>
> >> +config ERRATA_THEAD_QSPINLOCK
> >> + bool "Apply T-Head queued spinlock errata"
> >> + depends on ERRATA_THEAD
> >> + default y
> >> + help
> >> + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to
> >> + match the xchg_tail requirement of qspinlock.
> >> +
> >> + This will apply the QSPINLOCK errata to handle the non-standard
> >> + behavior via using qspinlock instead of ticket_lock.
> >> +
> >> + If you don't know what to do here, say "Y".
> >
> > If this is to be applied, I would like to see a detailed explanation somewhere,
> > preferably with citations, of:
> >
> > (a) The memory model requirements for qspinlock
> > (b) Why, with arguments, RISC-V does not architecturally meet (a)
> > (c) Why, with arguments, T-HEAD C9xx meets (a)
> > (d) Why at least one other architecture which defines ARCH_USE_QUEUED_SPINLOCKS
> > meets (a)
>
> I agree.
>
> Just having a magic fence that makes qspinlocks stop livelocking on some
> processors is going to lead to a mess -- I'd argue this means those
> processors just don't provide the forward progress guarantee, but we'd
> really need something written down about what this new custom
> instruction aliasing as a fence does.
The "magic fence" is not related to the LR/SC forward progress
guarantee, and it's our processors' store buffer hardware problem that
needs to be fixed. Not only is qspinlock suffering on it, but also
kernel/locking/osq_lock.c.

This is about this patch:
https://lore.kernel.org/linux-riscv/20230910082911.3378782-10-guoren@kernel.org/
I've written down the root cause of that patch: The "new custom fence
instruction" triggers the store buffer flush.

Only the normal store instruction has the problem, and atomic stores
(including LR/SC) are okay.

>
> > As far as I can tell, the RISC-V guarantees concerning constrained LR/SC loops
> > (livelock freedom but no starvation freedom) are exactly the same as those in
> > Armv8 (as of 0487F.c) for equivalent loops, and xchg_tail compiles to a
> > constrained LR/SC loop with guaranteed eventual success (with -O1). Clearly you
> > disagree; I would like to see your perspective.
>
> It sounds to me like this processor might be quite broken: if it's
> permanently holding stores in a buffer we're going to have more issues
> than just qspinlock, pretty much anything concurrent is going to have
> issues -- and that's not just in the kernel, there's concurrent
> userspace code as well.
Yes, the userspace scenarios are our worries because modifying the
userspace atomic library is impossible. Now, we are stress-testing
various userspace parallel applications, especially userspace queued
spinlock, on the 128-core hardware platform.
We haven't detected the problem in the userspace, and it could be
because of timer interrupts, which breaks the store buffer waiting.

>
> > -s
> >
> >> +
> >> endmenu # "CPU errata selection"
> >> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> >> index 881729746d2e..d560dc45c0e7 100644
> >> --- a/arch/riscv/errata/thead/errata.c
> >> +++ b/arch/riscv/errata/thead/errata.c
> >> @@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage,
> >> return false;
> >> }
> >>
> >> +static bool errata_probe_qspinlock(unsigned int stage,
> >> + unsigned long arch_id, unsigned long impid)
> >> +{
> >> + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK))
> >> + return false;
> >> +
> >> + /*
> >> + * The queued_spinlock torture would get in livelock without
> >> + * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD
> >> + * processors.
> >> + */
> >> + if (arch_id == 0 && impid == 0 &&
> >> + !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE))
> >> + return false;
> >> +
> >> + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> >> + return true;
> >> +
> >> + return false;
> >> +}
> >> +
> >> static u32 thead_errata_probe(unsigned int stage,
> >> unsigned long archid, unsigned long impid)
> >> {
> >> @@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage,
> >> if (errata_probe_write_once(stage, archid, impid))
> >> cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE);
> >>
> >> + if (errata_probe_qspinlock(stage, archid, impid))
> >> + cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK);
> >> +
> >> return cpu_req_errata;
> >> }
> >>
> >> diff --git a/arch/riscv/include/asm/errata_list.h
> >> b/arch/riscv/include/asm/errata_list.h
> >> index fbb2b8d39321..a696d18d1b0d 100644
> >> --- a/arch/riscv/include/asm/errata_list.h
> >> +++ b/arch/riscv/include/asm/errata_list.h
> >> @@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE( \
> >> : "=r" (__ovl) : \
> >> : "memory")
> >>
> >> +static __always_inline bool
> >> +riscv_has_errata_thead_qspinlock(void)
> >> +{
> >> + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
> >> + asm_volatile_goto(
> >> + ALTERNATIVE(
> >> + "j %l[l_no]", "nop",
> >> + THEAD_VENDOR_ID,
> >> + ERRATA_THEAD_QSPINLOCK,
> >> + CONFIG_ERRATA_THEAD_QSPINLOCK)
> >> + : : : : l_no);
> >> + } else {
> >> + goto l_no;
> >> + }
> >> +
> >> + return true;
> >> +l_no:
> >> + return false;
> >> +}
> >> +
> >> #endif /* __ASSEMBLY__ */
> >>
> >> #endif
> >> diff --git a/arch/riscv/include/asm/vendorid_list.h
> >> b/arch/riscv/include/asm/vendorid_list.h
> >> index 73078cfe4029..1f1d03877f5f 100644
> >> --- a/arch/riscv/include/asm/vendorid_list.h
> >> +++ b/arch/riscv/include/asm/vendorid_list.h
> >> @@ -19,7 +19,8 @@
> >> #define ERRATA_THEAD_CMO 1
> >> #define ERRATA_THEAD_PMU 2
> >> #define ERRATA_THEAD_WRITE_ONCE 3
> >> -#define ERRATA_THEAD_NUMBER 4
> >> +#define ERRATA_THEAD_QSPINLOCK 4
> >> +#define ERRATA_THEAD_NUMBER 5
> >> #endif
> >>
> >> #endif
> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >> index f8dbbe1bbd34..d9694fe40a9a 100644
> >> --- a/arch/riscv/kernel/cpufeature.c
> >> +++ b/arch/riscv/kernel/cpufeature.c
> >> @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void)
> >> * spinlock value, the only way is to change from queued_spinlock to
> >> * ticket_spinlock, but can not be vice.
> >> */
> >> - if (!force_qspinlock) {
> >> + if (!force_qspinlock &&
> >> + !riscv_has_errata_thead_qspinlock()) {
> >> set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa);
> >> }
> >> #endif
> >> --
> >> 2.36.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Best Regards
Guo Ren