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
