From: Guo Ren <guoren@kernel.org>
To: Palmer Dabbelt <palmer@rivosinc.com>
Cc: sorear@fastmail.com, Paul Walmsley <paul.walmsley@sifive.com>,
anup@brainfault.org, peterz@infradead.org, mingo@redhat.com,
Will Deacon <will@kernel.org>,
longman@redhat.com, boqun.feng@gmail.com, tglx@linutronix.de,
paulmck@kernel.org, rostedt@goodmis.org, rdunlap@infradead.org,
Catalin Marinas <catalin.marinas@arm.com>,
Conor Dooley <conor.dooley@microchip.com>,
xiaoguang.xing@sophgo.com, Bjorn Topel <bjorn@rivosinc.com>,
alexghiti@rivosinc.com, keescook@chromium.org,
greentime.hu@sifive.com, ajones@ventanamicro.com,
jszhang@kernel.org, wefu@redhat.com, wuwei2016@iscas.ac.cn,
linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org,
linux-doc@vger.kernel.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
linux-csky@vger.kernel.org, guoren@linux.alibaba.com
Subject: Re: [PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK
Date: Thu, 14 Sep 2023 11:31:45 +0800 [thread overview]
Message-ID: <CAJF2gTRF5t3cYy_Y2hHLBKjx2arsu+gugDb_g4EH9M2oyKK1vQ@mail.gmail.com> (raw)
In-Reply-To: <mhng-ee184bd2-7666-402d-b0df-d484ed6d8236@palmer-ri-x1c9>
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
next prev parent reply other threads:[~2023-09-14 3:32 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-02 16:46 [PATCH V10 00/19] riscv: Add Native/Paravirt/CNA qspinlock support guoren
2023-08-02 16:46 ` [PATCH V10 01/19] asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock guoren
2023-08-02 16:46 ` [PATCH V10 02/19] asm-generic: ticket-lock: Move into ticket_spinlock.h guoren
2023-08-02 16:46 ` [PATCH V10 03/19] riscv: qspinlock: errata: Add ERRATA_THEAD_WRITE_ONCE fixup guoren
2023-08-02 16:46 ` [PATCH V10 04/19] riscv: qspinlock: Add basic queued_spinlock support guoren
2023-08-11 19:34 ` Waiman Long
2023-08-12 0:18 ` Guo Ren
2023-08-02 16:46 ` [PATCH V10 05/19] riscv: qspinlock: Introduce combo spinlock guoren
2023-08-11 19:51 ` Waiman Long
2023-08-12 0:22 ` Guo Ren
2023-08-02 16:46 ` [PATCH V10 06/19] riscv: qspinlock: Allow force qspinlock from the command line guoren
2023-08-02 16:46 ` [PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK guoren
2023-08-04 9:05 ` Conor Dooley
2023-08-04 9:53 ` Guo Ren
2023-08-04 10:06 ` Conor Dooley
2023-08-05 1:28 ` Guo Ren
2023-08-07 5:23 ` Stefan O'Rear
2023-08-08 2:12 ` Guo Ren
2023-09-13 18:54 ` Palmer Dabbelt
2023-09-13 19:32 ` Waiman Long
2023-09-14 3:31 ` Guo Ren [this message]
2023-08-02 16:46 ` [PATCH V10 08/19] riscv: qspinlock: Use new static key for controlling call of virt_spin_lock() guoren
2023-08-02 16:46 ` [PATCH V10 09/19] RISC-V: paravirt: pvqspinlock: Add paravirt qspinlock skeleton guoren
2023-08-02 16:46 ` [PATCH V10 10/19] RISC-V: paravirt: pvqspinlock: KVM: " guoren
2023-08-02 16:46 ` [PATCH V10 11/19] RISC-V: paravirt: pvqspinlock: KVM: Implement kvm_sbi_ext_pvlock_kick_cpu() guoren
2023-08-02 16:46 ` [PATCH V10 12/19] RISC-V: paravirt: pvqspinlock: Add nopvspin kernel parameter guoren
2023-08-02 16:46 ` [PATCH V10 13/19] RISC-V: paravirt: pvqspinlock: Remove unnecessary definitions of cmpxchg & xchg guoren
2023-08-02 16:46 ` [PATCH V10 14/19] RISC-V: paravirt: pvqspinlock: Add xchg8 & cmpxchg_small support guoren
2023-08-02 16:46 ` [PATCH V10 15/19] RISC-V: paravirt: pvqspinlock: Add SBI implementation guoren
2023-08-02 16:46 ` [PATCH V10 16/19] RISC-V: paravirt: pvqspinlock: Add kconfig entry guoren
2023-08-02 16:46 ` [PATCH V10 17/19] RISC-V: paravirt: pvqspinlock: Add trace point for pv_kick/wait guoren
2023-08-02 16:47 ` [PATCH V10 18/19] locking/qspinlock: Move pv_ops into x86 directory guoren
2023-08-11 20:42 ` Waiman Long
2023-08-12 0:24 ` Guo Ren
2023-08-12 0:47 ` Waiman Long
2023-08-02 16:47 ` [PATCH V10 19/19] locking/qspinlock: riscv: Add Compact NUMA-aware lock support guoren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJF2gTRF5t3cYy_Y2hHLBKjx2arsu+gugDb_g4EH9M2oyKK1vQ@mail.gmail.com \
--to=guoren@kernel.org \
--cc=ajones@ventanamicro.com \
--cc=alexghiti@rivosinc.com \
--cc=anup@brainfault.org \
--cc=bjorn@rivosinc.com \
--cc=boqun.feng@gmail.com \
--cc=catalin.marinas@arm.com \
--cc=conor.dooley@microchip.com \
--cc=greentime.hu@sifive.com \
--cc=guoren@linux.alibaba.com \
--cc=jszhang@kernel.org \
--cc=keescook@chromium.org \
--cc=kvm@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-csky@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=longman@redhat.com \
--cc=mingo@redhat.com \
--cc=palmer@rivosinc.com \
--cc=paul.walmsley@sifive.com \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rdunlap@infradead.org \
--cc=rostedt@goodmis.org \
--cc=sorear@fastmail.com \
--cc=tglx@linutronix.de \
--cc=virtualization@lists.linux-foundation.org \
--cc=wefu@redhat.com \
--cc=will@kernel.org \
--cc=wuwei2016@iscas.ac.cn \
--cc=xiaoguang.xing@sophgo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).