| Message ID | 1569847479-13201-2-git-send-email-zhenzhong.duan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a unified parameter "nopvspin" | expand |
On Mon, Sep 30, 2019 at 08:44:36PM +0800, Zhenzhong Duan wrote: > There are cases where a guest tries to switch spinlocks to bare metal > behavior (e.g. by setting "xen_nopvspin" on XEN platform and > "hv_nopvspin" on HYPER_V). > > That feature is missed on KVM, add a new parameter "nopvspin" to disable > PV spinlocks for KVM guest. > > This new parameter is also used to replace "xen_nopvspin" and > "hv_nopvspin". This is confusing as there are no Xen or Hyper-V changes in this patch. Please make it clear that you're talking about future patches, e.g.: The new 'nopvspin' parameter will also replace Xen and Hyper-V specific parameters in future patches. > > The global variable pvspin isn't defined as __initdata as it's used at > runtime by XEN guest. Same comment as above regarding what this patch is doing versus what will be done in the future. Arguably you should even mark it __initdata in this patch and deal with conflict in the Xen patch, e.g. use it only to set the existing xen_pvspin variable. > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krcmar <rkrcmar@redhat.com> > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com> > Cc: Wanpeng Li <wanpengli@tencent.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: Joerg Roedel <joro@8bytes.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Will Deacon <will@kernel.org> > --- > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > arch/x86/include/asm/qspinlock.h | 1 + > arch/x86/kernel/kvm.c | 7 +++++++ > kernel/locking/qspinlock.c | 7 +++++++ > 4 files changed, 19 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index c7ac2f3..4b956d8 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -5330,6 +5330,10 @@ > as generic guest with no PV drivers. Currently support > XEN HVM, KVM, HYPER_V and VMWARE guest. > > + nopvspin [X86,KVM] Disables the qspinlock slow path > + using PV optimizations which allow the hypervisor to > + 'idle' the guest on lock contention. > + > xirc2ps_cs= [NET,PCMCIA] > Format: > <irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]] > diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h > index 444d6fd..34a4484 100644 > --- a/arch/x86/include/asm/qspinlock.h > +++ b/arch/x86/include/asm/qspinlock.h > @@ -32,6 +32,7 @@ static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lo > extern void __pv_init_lock_hash(void); > extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); > extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock); > +extern bool pvspin; > > #define queued_spin_unlock queued_spin_unlock > /** > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index e820568..a4f108d 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void) > if (num_possible_cpus() == 1) > return; > > + if (!pvspin) { > + pr_info("PV spinlocks disabled\n"); > + static_branch_disable(&virt_spin_lock_key); > + return; > + } > + pr_info("PV spinlocks enabled\n"); These prints could be confusing as KVM also disables PV spinlocks when it sees KVM_HINTS_REALTIME. > + > __pv_init_lock_hash(); > pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; > pv_ops.lock.queued_spin_unlock = > diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c > index 2473f10..945b510 100644 > --- a/kernel/locking/qspinlock.c > +++ b/kernel/locking/qspinlock.c > @@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val) > #include "qspinlock_paravirt.h" > #include "qspinlock.c" > > +bool pvspin = true; This can be __ro_after_init, or probably better __initdata and have Xen snapshot the value for its use case. Personal preference: I'd invert the bool and name it nopvspin to make it easier to connect the variable to the kernel param. > +static __init int parse_nopvspin(char *arg) > +{ > + pvspin = false; > + return 0; > +} > +early_param("nopvspin", parse_nopvspin); > #endif > -- > 1.8.3.1 >
On 2019/10/3 1:10, Sean Christopherson wrote: > On Mon, Sep 30, 2019 at 08:44:36PM +0800, Zhenzhong Duan wrote: >> There are cases where a guest tries to switch spinlocks to bare metal >> behavior (e.g. by setting "xen_nopvspin" on XEN platform and >> "hv_nopvspin" on HYPER_V). >> >> That feature is missed on KVM, add a new parameter "nopvspin" to disable >> PV spinlocks for KVM guest. >> >> This new parameter is also used to replace "xen_nopvspin" and >> "hv_nopvspin". > This is confusing as there are no Xen or Hyper-V changes in this patch. > Please make it clear that you're talking about future patches, e.g.: > > The new 'nopvspin' parameter will also replace Xen and Hyper-V specific > parameters in future patches. Will fix > >> The global variable pvspin isn't defined as __initdata as it's used at >> runtime by XEN guest. > Same comment as above regarding what this patch is doing versus what will > be done in the future. Arguably you should even mark it __initdata in > this patch and deal with conflict in the Xen patch, e.g. use it only to > set the existing xen_pvspin variable. Will fix > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com> ......snip >> /** >> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c >> index e820568..a4f108d 100644 >> --- a/arch/x86/kernel/kvm.c >> +++ b/arch/x86/kernel/kvm.c >> @@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void) >> if (num_possible_cpus() == 1) >> return; >> >> + if (!pvspin) { >> + pr_info("PV spinlocks disabled\n"); >> + static_branch_disable(&virt_spin_lock_key); >> + return; >> + } >> + pr_info("PV spinlocks enabled\n"); > These prints could be confusing as KVM also disables PV spinlocks when it > sees KVM_HINTS_REALTIME. What about below: pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n"); Or you prefer separate print for each disabling like below? /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { pr_info("PV spinlocks disabled, KVM_FEATURE_PV_UNHALT feature needed.\n"); return; } if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { pr_info("PV spinlocks disabled, having non-preemption hints.\n"); return; } /* Don't use the pvqspinlock code if there is only 1 vCPU. */ if (num_possible_cpus() == 1) { pr_info("PV spinlocks disabled on UP.\n"); return; } if (!pvspin) { pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n"); static_branch_disable(&virt_spin_lock_key); return; } pr_info("PV spinlocks enabled\n"); > >> + >> __pv_init_lock_hash(); >> pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; >> pv_ops.lock.queued_spin_unlock = >> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c >> index 2473f10..945b510 100644 >> --- a/kernel/locking/qspinlock.c >> +++ b/kernel/locking/qspinlock.c >> @@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val) >> #include "qspinlock_paravirt.h" >> #include "qspinlock.c" >> >> +bool pvspin = true; > This can be __ro_after_init, or probably better __initdata and have Xen > snapshot the value for its use case. I will use __initdata > > Personal preference: I'd invert the bool and name it nopvspin to make it > easier to connect the variable to the kernel param. OK, will do that. Thanks for review for all the patches. Zhenzhong
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index c7ac2f3..4b956d8 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -5330,6 +5330,10 @@ as generic guest with no PV drivers. Currently support XEN HVM, KVM, HYPER_V and VMWARE guest. + nopvspin [X86,KVM] Disables the qspinlock slow path + using PV optimizations which allow the hypervisor to + 'idle' the guest on lock contention. + xirc2ps_cs= [NET,PCMCIA] Format: <irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]] diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h index 444d6fd..34a4484 100644 --- a/arch/x86/include/asm/qspinlock.h +++ b/arch/x86/include/asm/qspinlock.h @@ -32,6 +32,7 @@ static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lo extern void __pv_init_lock_hash(void); extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock); +extern bool pvspin; #define queued_spin_unlock queued_spin_unlock /** diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index e820568..a4f108d 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void) if (num_possible_cpus() == 1) return; + if (!pvspin) { + pr_info("PV spinlocks disabled\n"); + static_branch_disable(&virt_spin_lock_key); + return; + } + pr_info("PV spinlocks enabled\n"); + __pv_init_lock_hash(); pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; pv_ops.lock.queued_spin_unlock = diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c index 2473f10..945b510 100644 --- a/kernel/locking/qspinlock.c +++ b/kernel/locking/qspinlock.c @@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val) #include "qspinlock_paravirt.h" #include "qspinlock.c" +bool pvspin = true; +static __init int parse_nopvspin(char *arg) +{ + pvspin = false; + return 0; +} +early_param("nopvspin", parse_nopvspin); #endif
There are cases where a guest tries to switch spinlocks to bare metal behavior (e.g. by setting "xen_nopvspin" on XEN platform and "hv_nopvspin" on HYPER_V). That feature is missed on KVM, add a new parameter "nopvspin" to disable PV spinlocks for KVM guest. This new parameter is also used to replace "xen_nopvspin" and "hv_nopvspin". The global variable pvspin isn't defined as __initdata as it's used at runtime by XEN guest. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Radim Krcmar <rkrcmar@redhat.com> Cc: Sean Christopherson <sean.j.christopherson@intel.com> Cc: Vitaly Kuznetsov <vkuznets@redhat.com> Cc: Wanpeng Li <wanpengli@tencent.com> Cc: Jim Mattson <jmattson@google.com> Cc: Joerg Roedel <joro@8bytes.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Will Deacon <will@kernel.org> --- Documentation/admin-guide/kernel-parameters.txt | 4 ++++ arch/x86/include/asm/qspinlock.h | 1 + arch/x86/kernel/kvm.c | 7 +++++++ kernel/locking/qspinlock.c | 7 +++++++ 4 files changed, 19 insertions(+)