Re: [PATCH V7 4/5] asm-generic: spinlock: Add combo spinlock (ticket & queued)

[Waiman Long]

On 6/28/22 21:17, Guo Ren wrote:
> On Wed, Jun 29, 2022 at 2:13 AM Waiman Long <longman@xxxxxxxxxx> wrote:
> > On 6/28/22 04:17, guoren@xxxxxxxxxx wrote:
> > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > >
> > > Some architecture has a flexible requirement on the type of spinlock.
> > > Some LL/SC architectures of ISA don't force micro-arch to give a strong
> > > forward guarantee. Thus different kinds of memory model micro-arch would
> > > come out in one ISA. The ticket lock is suitable for exclusive monitor
> > > designed LL/SC micro-arch with limited cores and "!NUMA". The
> > > queue-spinlock could deal with NUMA/large-scale scenarios with a strong
> > > forward guarantee designed LL/SC micro-arch.
> > >
> > > So, make the spinlock a combo with feature.
> > >
> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> > > Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > > Cc: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> > > ---
> > >    include/asm-generic/spinlock.h | 43 ++++++++++++++++++++++++++++++++--
> > >    kernel/locking/qspinlock.c     |  2 ++
> > >    2 files changed, 43 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/asm-generic/spinlock.h b/include/asm-generic/spinlock.h
> > > index f41dc7c2b900..a9b43089bf99 100644
> > > --- a/include/asm-generic/spinlock.h
> > > +++ b/include/asm-generic/spinlock.h
> > > @@ -28,34 +28,73 @@
> > >    #define __ASM_GENERIC_SPINLOCK_H
> > >
> > >    #include <asm-generic/ticket_spinlock.h>
> > > +#ifdef CONFIG_ARCH_USE_QUEUED_SPINLOCKS
> > > +#include <linux/jump_label.h>
> > > +#include <asm-generic/qspinlock.h>
> > > +
> > > +DECLARE_STATIC_KEY_TRUE(use_qspinlock_key);
> > > +#endif
> > > +
> > > +#undef arch_spin_is_locked
> > > +#undef arch_spin_is_contended
> > > +#undef arch_spin_value_unlocked
> > > +#undef arch_spin_lock
> > > +#undef arch_spin_trylock
> > > +#undef arch_spin_unlock
> > >
> > >    static __always_inline void arch_spin_lock(arch_spinlock_t *lock)
> > >    {
> > > -     ticket_spin_lock(lock);
> > > +#ifdef CONFIG_ARCH_USE_QUEUED_SPINLOCKS
> > > +     if (static_branch_likely(&use_qspinlock_key))
> > > +             queued_spin_lock(lock);
> > > +     else
> > > +#endif
> > > +             ticket_spin_lock(lock);
> > >    }
> > Why do you use a static key to control whether to use qspinlock or
> > ticket lock? In the next patch, you have
> >
> > +#if !defined(CONFIG_NUMA) && defined(CONFIG_QUEUED_SPINLOCKS)
> > +       static_branch_disable(&use_qspinlock_key);
> > +#endif
> >
> > So the current config setting determines if qspinlock will be used, not
> > some boot time parameter that user needs to specify. This patch will
> > just add useless code to lock/unlock sites. I don't see any benefit of
> > doing that.
> This is a startup patch for riscv. next, we could let vendors make choices.
> I'm not sure they like cmdline or vendor-specific errata style.
>
> Eventually, we would let one riscv Image support all machines, some
> use ticket-lock, and some use qspinlock.

OK. Maybe you can postpone this combo spinlock until there is a good use 
case for it. Upstream usually don't accept patches that have no good use 
case yet.

Cheers,
Longman