| Message ID | 1570111335-12731-2-git-send-email-zhenzhong.duan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a unified parameter "nopvspin" | expand |
On 10/3/19 10:02 AM, Zhenzhong Duan wrote: > void __init kvm_spinlock_init(void) > { > - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ > - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) > - return; > - > - if (kvm_para_has_hint(KVM_HINTS_REALTIME)) > + /* > + * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT feature > + * support, or there is REALTIME hints or only 1 vCPU. > + */ > + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || > + kvm_para_has_hint(KVM_HINTS_REALTIME) || > + num_possible_cpus() == 1) { > + pr_info("PV spinlocks disabled\n"); > 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); Would it make sense to bring here the other site where the key is disabled (in kvm_smp_prepare_cpus())? (and, in fact, shouldn't all of the checks that result in early return above disable the key?) -boris > return; > + } > + pr_info("PV spinlocks enabled\n"); > > __pv_init_lock_hash(); > pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; >
On 2019/10/4 22:52, Boris Ostrovsky wrote: > On 10/3/19 10:02 AM, Zhenzhong Duan wrote: >> void __init kvm_spinlock_init(void) >> { >> - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ >> - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) >> - return; >> - >> - if (kvm_para_has_hint(KVM_HINTS_REALTIME)) >> + /* >> + * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT feature >> + * support, or there is REALTIME hints or only 1 vCPU. >> + */ >> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || >> + kvm_para_has_hint(KVM_HINTS_REALTIME) || >> + num_possible_cpus() == 1) { >> + pr_info("PV spinlocks disabled\n"); >> 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); > Would it make sense to bring here the other site where the key is > disabled (in kvm_smp_prepare_cpus())? Thanks for point out, I'll do it. Just not clear if I should do that in a separate patch, there is a history about that code: Its original place was here and then moved to kvm_smp_prepare_cpus() by below commit: 34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static key get initialized") which fixed jump_label_init() calling late issue. Then 8990cac6 ("x86/jump_label: Initialize static branching early") move jump_label_init() early, so commit 34226b6b could be reverted. > > (and, in fact, shouldn't all of the checks that result in early return > above disable the key?) I think we should enable he key for !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case, there is lock holder preemption issue as qspinlock is fair lock, virt_spin_lock() is an optimization to that, imaging one pcpu running 10 vcpus of same guest contending a same lock. For kvm_para_has_hint(KVM_HINTS_REALTIME) case, hypervisor hints there is no preemption and we should disable virt_spin_lock_key to use native qspinlock. For the UP case, we don't care virt_spin_lock_key value. For nopvspin case, we intentionally check native qspinlock code performance, compare it with PV qspinlock, etc. So virt_spin_lock() optimization should be disabled. Let me know if anything wrong with above understanding. Thanks Zhenzhong
On 10/6/19 3:49 AM, Zhenzhong Duan wrote: > On 2019/10/4 22:52, Boris Ostrovsky wrote: > >> On 10/3/19 10:02 AM, Zhenzhong Duan wrote: >>> void __init kvm_spinlock_init(void) >>> { >>> - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ >>> - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) >>> - return; >>> - >>> - if (kvm_para_has_hint(KVM_HINTS_REALTIME)) >>> + /* >>> + * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT >>> feature >>> + * support, or there is REALTIME hints or only 1 vCPU. >>> + */ >>> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || >>> + kvm_para_has_hint(KVM_HINTS_REALTIME) || >>> + num_possible_cpus() == 1) { >>> + pr_info("PV spinlocks disabled\n"); >>> 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); >> Would it make sense to bring here the other site where the key is >> disabled (in kvm_smp_prepare_cpus())? > > Thanks for point out, I'll do it. Just not clear if I should do that > in a separate patch, > there is a history about that code: > > Its original place was here and then moved to kvm_smp_prepare_cpus() > by below commit: > 34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static > key get initialized") > which fixed jump_label_init() calling late issue. > > Then 8990cac6 ("x86/jump_label: Initialize static branching early") > move jump_label_init() > early, so commit 34226b6b could be reverted. Which is similar to what you did earlier for Xen. > >> >> (and, in fact, shouldn't all of the checks that result in early return >> above disable the key?) > > I think we should enable he key for > !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case, > there is lock holder preemption issue as qspinlock is fair lock, > virt_spin_lock() > is an optimization to that, imaging one pcpu running 10 vcpus of same > guest > contending a same lock. Right. I conflated pv lock and virt_spin_lock_key, and that is wrong. -boris > > For kvm_para_has_hint(KVM_HINTS_REALTIME) case, hypervisor hints there is > no preemption and we should disable virt_spin_lock_key to use native > qspinlock. > > For the UP case, we don't care virt_spin_lock_key value. > > For nopvspin case, we intentionally check native qspinlock code > performance, > compare it with PV qspinlock, etc. So virt_spin_lock() optimization > should be disabled. > > Let me know if anything wrong with above understanding. Thanks > > Zhenzhong >
On 2019/10/7 22:46, Boris Ostrovsky wrote: > On 10/6/19 3:49 AM, Zhenzhong Duan wrote: >> On 2019/10/4 22:52, Boris Ostrovsky wrote: >> >>> On 10/3/19 10:02 AM, Zhenzhong Duan wrote: >>>> void __init kvm_spinlock_init(void) >>>> { >>>> - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ >>>> - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) >>>> - return; >>>> - >>>> - if (kvm_para_has_hint(KVM_HINTS_REALTIME)) >>>> + /* >>>> + * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT >>>> feature >>>> + * support, or there is REALTIME hints or only 1 vCPU. >>>> + */ >>>> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || >>>> + kvm_para_has_hint(KVM_HINTS_REALTIME) || >>>> + num_possible_cpus() == 1) { >>>> + pr_info("PV spinlocks disabled\n"); >>>> 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); >>> Would it make sense to bring here the other site where the key is >>> disabled (in kvm_smp_prepare_cpus())? >> Thanks for point out, I'll do it. Just not clear if I should do that >> in a separate patch, >> there is a history about that code: >> >> Its original place was here and then moved to kvm_smp_prepare_cpus() >> by below commit: >> 34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static >> key get initialized") >> which fixed jump_label_init() calling late issue. >> >> Then 8990cac6 ("x86/jump_label: Initialize static branching early") >> move jump_label_init() >> early, so commit 34226b6b could be reverted. > Which is similar to what you did earlier for Xen. You remember that, ok, I'll do the same for KVM. Thanks 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 e820568..481d879 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -831,16 +831,23 @@ __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)) - return; - - if (kvm_para_has_hint(KVM_HINTS_REALTIME)) + /* + * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT feature + * support, or there is REALTIME hints or only 1 vCPU. + */ + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) || + kvm_para_has_hint(KVM_HINTS_REALTIME) || + num_possible_cpus() == 1) { + pr_info("PV spinlocks disabled\n"); 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, 27 insertions(+), 7 deletions(-)