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

From: Guo Ren <guoren@kernel.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Waiman Long <longman@redhat.com>,
	Palmer Dabbelt <palmer@rivosinc.com>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Guo Ren <guoren@linux.alibaba.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH V7 4/5] asm-generic: spinlock: Add combo spinlock (ticket & queued)
Date: Wed, 29 Jun 2022 16:24:03 +0800	[thread overview]
Message-ID: <CAJF2gTTtKiMZJLWBEcRz-4CmNojH0cOgpUYGupLyCXXFjQD_FQ@mail.gmail.com> (raw)
In-Reply-To: <CAK8P3a2jQ+UQ54=QcpyVt91vXRyZrvUtOygFYOHWTBzse3q3rg@mail.gmail.com>

On Wed, Jun 29, 2022 at 3:09 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Jun 29, 2022 at 3:34 AM Waiman Long <longman@redhat.com> wrote:
> > On 6/28/22 21:17, Guo Ren wrote:
> > > On Wed, Jun 29, 2022 at 2:13 AM Waiman Long <longman@redhat.com> wrote:
> > >> On 6/28/22 04:17, guoren@kernel.org 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.
>
> 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.
If CONFIG_ARCH_USE_QUEUED_SPINLOCKS is not enabled, the patch still
will keep the ticket-lock simple without 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.
arm64 needn't combo spinlock, it could use pure qspinlock with keeping
current header files included.

>
> 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.
Yes, it could. I agree with using combo spinlock only with riscv.

>
>         Arnd


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/