Message-ID: <CAJF2gTRpXBS-YotgEO=cPGnwjYiUyda_wk_fQfA6j_QqdYHoQw@mail.gmail.com>
Date: Tue, 16 Jul 2024 16:31:23 +0800
From: Guo Ren <guoren@...nel.org>
To: Alexandre Ghiti <alexghiti@...osinc.com>
Cc: Waiman Long <longman@...hat.com>, Jonathan Corbet <corbet@....net>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Andrea Parri <parri.andrea@...il.com>, 
	Nathan Chancellor <nathan@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Will Deacon <will@...nel.org>, Boqun Feng <boqun.feng@...il.com>, Arnd Bergmann <arnd@...db.de>, 
	Leonardo Bras <leobras@...hat.com>, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-arch@...r.kernel.org
Subject: Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension

On Tue, Jul 16, 2024 at 2:43 PM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
>
> Hi Guo, Waiman,
>
> On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@...nel.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@...hat.com> wrote:
> > >
> > > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > > Hi Guo,
> > > >
> > > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@...nel.org> wrote:
> > > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@...osinc.com> 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 is 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
> > > >> That's not true; we treat spinlock as qspinlock at first.
> > > > That's what I said: we have to use the qspinlock implementation at
> > > > first *because* we can't discover the extensions soon enough to use
> > > > the right spinlock implementation before the kernel uses a spinlock.
> > > > And since the spinlocks are used *before* the discovery of the
> > > > extensions, we cannot use the current alternative mechanism or we'd
> > > > need to extend it to add an "initial" value which does not depend on
> > > > the available extensions.
> > >
> > > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > > is not the case with ticket lock. Assuming that all the discovery will
> > > be done before SMP boot, the qspinlock slowpath won't be activated and
> > > so we don't need the presence of any extension. I believe that is the
> > > main reason why qspinlock is used as the initial default and not ticket
> > > lock.
> > Thx Waiman,
> > Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
>
> Guys, we all agree on that, that's why I kept this behaviour in this patchset.
>
> >
> > Hi Alexandre,
> > Therefore, the switch point(before reset_init()) is late enough to
> > change the lock mechanism, and this satisfies the requirements of
> > apply_boot_alternatives(), apply_early_boot_alternatives(), and
> > apply_module_alternatives().
>
> I can't find reset_init().
Sorry for the typo, rest_init()
>
> The discovery of the extensions is done in riscv_fill_hwcap() called
> from setup_arch()
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
> So *before* this point, we have no knowledge of the available
> extensions on the platform.  Let's imagine now that we use an
> alternative for the qspinlock implementation, it will look like this
> (I use only zabha here, that's an example):
>
> --- a/arch/riscv/include/asm/spinlock.h
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -16,8 +16,12 @@ 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);                          \
> +       asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0,                 \
> +                                     RISCV_ISA_EXT_ZABHA, 1)            \
> +                         : : : : no_zabha);                            \
> +                                                                       \
> +       return queued_spin_##op(lock);                                  \
> +no_zabha:                                                              \
>         return ticket_spin_##op(lock);                                  \
>  }
>
> apply_early_boot_alternatives() can't be used to patch the above
> alternative since it is called from setup_vm(), way before we know the
> available extensions.
> apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
> and then will be able to patch the alternatives correctly
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
>
> But then, before apply_boot_alternatives(), any use of a spinlock
> (printk does so) will use a ticket spinlock implementation, and not
> the qspinlock implementation. How do you fix that?
Why "before apply_boot_alternatives(), any use of a spinlock (printk
does so) will use a ticket spinlock implementation" ?
We treat qspinlock as the initial spinlock forever, so there is only
qspinlock -> ticket_lock direction and no reverse.

>
> >
> > >
> > > Cheers,
> > > Longman
> > >
> >
> >
> > --
> > Best Regards
> >  Guo Ren



-- 
Best Regards
 Guo Ren

Powered by blists - more mailing lists