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

[Waiman Long]

On 6/29/22 03:08, Arnd Bergmann wrote:
> On Wed, Jun 29, 2022 at 3:34 AM Waiman Long <longman@xxxxxxxxxx> wrote:
> > 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:
> > > >
> > > > 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.
> I think the usecase on risc-v is this: there are cases where the qspinlock
> is preferred for performance reasons, but there are also CPU cores on
> which it is not safe to use. risc-v like most modern architectures has a
> strict rule about being able to build kernels that work on all machines,
> so without something like this, it would not be able to use qspinlock at all.

My objection for the current patch is really on the fact that everything 
is determined at compiled time. So there is no point to use static key 
if it cannot be changed at the boot time. Adding a boot time switch do 
make the use of static key more reasonable.


> On the other hand, I don't really like the idea of putting the static-key
> wrapper into the asm-generic header. Especially the ticket spinlock
> implementation should be simple and not depend on jump labels.
>
>  From looking at the header file dependencies on arm64, I know that
> putting jump labels into core infrastructure like the arch_spin_lock()
> makes a big mess of indirect includes and measurably slows down
> the kernel build.
>
> I think this can still be done in the riscv asm/spinlock.h header with
> minimal impact on the asm-generic file if the riscv maintainers see
> a significant enough advantage, but I don't want it in the common code.

I have a similar feeling. In addition, I don't like the idea of adding a 
static key to qspinlock.c that have nothing to do with the qspinlock 
logic. I would like to see it put elsewhere.

Cheers,
Longman