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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 14, 2023 at 2:54 AM Palmer Dabbelt <palmer@xxxxxxxxxxxx> wrote:
>
> On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear@xxxxxxxxxxxx wrote:
> > On Wed, Aug 2, 2023, at 12:46 PM, guoren@xxxxxxxxxx wrote:
> >> From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxx/
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@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Best Regards
 Guo Ren




[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux