| Message ID | 1570439071-9814-4-git-send-email-zhenzhong.duan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a unified parameter "nopvspin" | expand |
Zhenzhong Duan <zhenzhong.duan@oracle.com> writes: > 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. > > The new 'nopvspin' parameter will also replace Xen and Hyper-V specific > parameters in future patches. > > Define variable nopvsin as global because it will be used in future > patches as above. > > 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 | 5 +++++ > arch/x86/include/asm/qspinlock.h | 1 + > arch/x86/kernel/kvm.c | 21 +++++++++++++++++---- > kernel/locking/qspinlock.c | 7 +++++++ > 4 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index c7ac2f3..89d77ea 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -5330,6 +5330,11 @@ > 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..d86ab94 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 nopvspin; > > #define queued_spin_unlock queued_spin_unlock > /** > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index ef836d6..6e14bd4 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -825,18 +825,31 @@ __visible bool __kvm_vcpu_is_preempted(long cpu) > */ > void __init kvm_spinlock_init(void) > { > - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ > - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) > + /* > + * Disable PV qspinlocks if host kernel doesn't support > + * KVM_FEATURE_PV_UNHALT feature or there is only 1 vCPU. > + * virt_spin_lock_key is enabled to avoid lock holder > + * preemption issue. > + */ > + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || > + num_possible_cpus() == 1) { > + pr_info("PV spinlocks disabled\n"); Why don't we need static_branch_disable(&virt_spin_lock_key) here? Also, as you're printing the exact reason for PV spinlocks disablement in other cases, I'd suggest separating "no host support" and "single CPU" cases. > return; > + } > > if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { > + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); > static_branch_disable(&virt_spin_lock_key); > return; > } > > - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ > - if (num_possible_cpus() == 1) > + if (nopvspin) { > + pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n"); Nit: to make it sound better a comma is missing between 'disabled' and 'forced', or "PV spinlocks forcefully disabled by ..." if you prefer. > + 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; > diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c > index 2473f10..75193d6 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 nopvspin __initdata; > +static __init int parse_nopvspin(char *arg) > +{ > + nopvspin = true; > + return 0; > +} > +early_param("nopvspin", parse_nopvspin); > #endif
On 2019/10/13 17:02, Vitaly Kuznetsov wrote: > Zhenzhong Duan <zhenzhong.duan@oracle.com> writes: ...snip > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index ef836d6..6e14bd4 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -825,18 +825,31 @@ __visible bool __kvm_vcpu_is_preempted(long cpu) > */ > void __init kvm_spinlock_init(void) > { > - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ > - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) > + /* > + * Disable PV qspinlocks if host kernel doesn't support > + * KVM_FEATURE_PV_UNHALT feature or there is only 1 vCPU. > + * virt_spin_lock_key is enabled to avoid lock holder > + * preemption issue. > + */ > + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || > + num_possible_cpus() == 1) { > + pr_info("PV spinlocks disabled\n"); > Why don't we need static_branch_disable(&virt_spin_lock_key) here? Thanks for review. I have a brief explanation in above comment area. Boris also raised the same question in v4 and see my detailed explanation in https://lkml.org/lkml/2019/10/6/39 > > Also, as you're printing the exact reason for PV spinlocks disablement > in other cases, I'd suggest separating "no host support" and "single > CPU" cases. Will do after reaching a consensus on your first question. > >> return; >> + } >> >> if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { >> + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); >> static_branch_disable(&virt_spin_lock_key); >> return; >> } >> >> - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ >> - if (num_possible_cpus() == 1) >> + if (nopvspin) { >> + pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n"); > Nit: to make it sound better a comma is missing between 'disabled' and > 'forced', or > > "PV spinlocks forcefully disabled by ..." if you prefer. Will do. Zhenzhong
Zhenzhong Duan <zhenzhong.duan@oracle.com> writes: > On 2019/10/13 17:02, Vitaly Kuznetsov wrote: >> Zhenzhong Duan <zhenzhong.duan@oracle.com> writes: > ...snip >> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c >> index ef836d6..6e14bd4 100644 >> --- a/arch/x86/kernel/kvm.c >> +++ b/arch/x86/kernel/kvm.c >> @@ -825,18 +825,31 @@ __visible bool __kvm_vcpu_is_preempted(long cpu) >> */ >> void __init kvm_spinlock_init(void) >> { >> - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ >> - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) >> + /* >> + * Disable PV qspinlocks if host kernel doesn't support >> + * KVM_FEATURE_PV_UNHALT feature or there is only 1 vCPU. >> + * virt_spin_lock_key is enabled to avoid lock holder >> + * preemption issue. >> + */ >> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || >> + num_possible_cpus() == 1) { >> + pr_info("PV spinlocks disabled\n"); >> Why don't we need static_branch_disable(&virt_spin_lock_key) here? > > Thanks for review. > > I have a brief explanation in above comment area. > > Boris also raised the same question in v4 and see my detailed explanation > > in https://lkml.org/lkml/2019/10/6/39 > >> >> Also, as you're printing the exact reason for PV spinlocks disablement >> in other cases, I'd suggest separating "no host support" and "single >> CPU" cases. > > Will do after reaching a consensus on your first question. Oh, sorry I missed v4 discussion. As I'm not the first to ask why we don't do static_branch_disable(&virt_spin_lock_key) here I suggest we do the followin: - Split !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) and num_possible_cpus() == 1 cases - Do static_branch_disable(&virt_spin_lock_key) for UP case (just for consistency). - Add a comment why we don't do that for !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case (basically, what you replied to Boris) This will also allow us to print the exact reason. > >> >>> return; >>> + } >>> >>> if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { >>> + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); >>> static_branch_disable(&virt_spin_lock_key); >>> return; >>> } >>> >>> - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ >>> - if (num_possible_cpus() == 1) >>> + if (nopvspin) { >>> + pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n"); >> Nit: to make it sound better a comma is missing between 'disabled' and >> 'forced', or >> >> "PV spinlocks forcefully disabled by ..." if you prefer. > > Will do. > > Zhenzhong > >
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index c7ac2f3..89d77ea 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -5330,6 +5330,11 @@ 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..d86ab94 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 nopvspin; #define queued_spin_unlock queued_spin_unlock /** diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index ef836d6..6e14bd4 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -825,18 +825,31 @@ __visible bool __kvm_vcpu_is_preempted(long cpu) */ void __init kvm_spinlock_init(void) { - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) + /* + * Disable PV qspinlocks if host kernel doesn't support + * KVM_FEATURE_PV_UNHALT feature or there is only 1 vCPU. + * virt_spin_lock_key is enabled to avoid lock holder + * preemption issue. + */ + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || + num_possible_cpus() == 1) { + pr_info("PV spinlocks disabled\n"); return; + } if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); static_branch_disable(&virt_spin_lock_key); return; } - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ - if (num_possible_cpus() == 1) + if (nopvspin) { + 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; diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c index 2473f10..75193d6 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 nopvspin __initdata; +static __init int parse_nopvspin(char *arg) +{ + nopvspin = true; + 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. The new 'nopvspin' parameter will also replace Xen and Hyper-V specific parameters in future patches. Define variable nopvsin as global because it will be used in future patches as above. 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 | 5 +++++ arch/x86/include/asm/qspinlock.h | 1 + arch/x86/kernel/kvm.c | 21 +++++++++++++++++---- kernel/locking/qspinlock.c | 7 +++++++ 4 files changed, 30 insertions(+), 4 deletions(-)