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. > > > > > > > > > Cheers, > > > Longman > > > > > > > > > -- > > Best Regards > > Guo Ren -- Best Regards Guo Ren
