Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension

[Alexandre Ghiti]

On Tue, Jul 16, 2024 at 10:31 AM Guo Ren <guoren@xxxxxxxxxx> wrote:
>
> On Tue, Jul 16, 2024 at 2:43 PM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
> >
> > Hi Guo, Waiman,
> >
> > On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@xxxxxxxxxx> wrote:
> > > >
> > > > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > > > Hi Guo,
> > > > >
> > > > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@xxxxxxxxxx> wrote:
> > > > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> 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.

Can you please provide an implementation of what you suggest using the
current alternatives tools? I'm about to send the v3 of this series,
you can use this as a base.

Thanks,

Alex

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