On Fri, Aug 4, 2023 at 6:07 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> On Fri, Aug 04, 2023 at 05:53:35PM +0800, Guo Ren wrote:
> > On Fri, Aug 4, 2023 at 5:06 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
> > > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@xxxxxxxxxx wrote:
> > > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
>
> > > > 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);
> > >
> > > Is this a generic vendor extension (lol @ that misnomer) or is it an
> > > erratum? Make your mind up please. As has been said on other series, NAK
> > > to using march/vendor/imp IDs for feature probing.
> >
> > The RISCV_ISA_EXT_XTICKETLOCK is a feature extension number,
>
> No, that is not what "ISA_EXT" means, nor what the X in "XTICKETLOCK"
> would imply.
>
> The comment above these reads:
> These macros represent the logical IDs of each multi-letter RISC-V ISA
> extension and are used in the ISA bitmap.
>
> > and it's
> > set by default for forward-compatible. We also define a vendor
> > extension (riscv_has_errata_thead_qspinlock) to force all our
> > processors to use qspinlock; others still stay on ticket_lock.
>
> No, "riscv_has_errata_thead_qspinlock()" would be an _erratum_, not a
> vendor extension. We need to have a discussion about how to support
> non-standard extensions etc, not abuse errata. That discussion has been
> started on the v0.7.1 vector patches, but has not made progress yet.
You convinced me, yes, I abuse errata here. I would change to Linux
standard static_key mechanism next.
>
> > The only possible changing direction is from qspinlock to ticket_lock
> > because ticket_lock would dirty the lock value, which prevents
> > changing to qspinlock next. So startup with qspinlock and change to
> > ticket_lock before smp up. You also could use cmdline to try qspinlock
> > (force_qspinlock).
>
> I don't see what the relevance of this is, sorry. I am only commenting
> on how you are deciding that the hardware is capable of using qspinlocks,
> I don't intend getting into the detail unless the powers that be deem
> this series worthwhile, as I mentioned:
> > > I've got some thoughts on other parts of this series too, but I'm not
> > > going to spend time on it unless the locking people and Palmer ascent
> > > to this series.
>
--
Best Regards
Guo Ren
