Message ID | 1571649076-2421-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> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.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 | 34 ++++++++++++++++++++----- > kernel/locking/qspinlock.c | 7 +++++ > 4 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index a84a83f..bd49ed2 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -5334,6 +5334,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 249f14a..3945aa5 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -825,18 +825,36 @@ __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)) > + /* > + * In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an > + * advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is > + * preferred over native qspinlock when vCPU is preempted. > + */ > + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { > + pr_info("PV spinlocks disabled, no host support.\n"); > return; > + } > > + /* > + * Disable PV qspinlock and use native qspinlock when dedicated pCPUs > + * are available. > + */ > if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { > - static_branch_disable(&virt_spin_lock_key); > - return; > + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); > + goto out; > } > > - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ > - if (num_possible_cpus() == 1) > - return; > + if (num_possible_cpus() == 1) { > + pr_info("PV spinlocks disabled, single CPU.\n"); > + goto out; > + } > + > + if (nopvspin) { > + pr_info("PV spinlocks disabled, forced by \"nopvspin\" parameter.\n"); > + goto out; > + } > + > + pr_info("PV spinlocks enabled\n"); > > __pv_init_lock_hash(); > pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; > @@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void) > pv_ops.lock.vcpu_is_preempted = > PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); > } > +out: > + static_branch_disable(&virt_spin_lock_key); You probably need to add 'return' before 'out:' as it seems you're disabling virt_spin_lock_key in all cases now). > } > > #endif /* CONFIG_PARAVIRT_SPINLOCKS */ > 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
Hi Vitaly, On 2019/10/22 19:36, 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 249f14a..3945aa5 100644 >> --- a/arch/x86/kernel/kvm.c >> +++ b/arch/x86/kernel/kvm.c >> @@ -825,18 +825,36 @@ __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)) >> + /* >> + * In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an >> + * advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is >> + * preferred over native qspinlock when vCPU is preempted. >> + */ >> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { >> + pr_info("PV spinlocks disabled, no host support.\n"); >> return; >> + } >> >> + /* >> + * Disable PV qspinlock and use native qspinlock when dedicated pCPUs >> + * are available. >> + */ >> if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { >> - static_branch_disable(&virt_spin_lock_key); >> - return; >> + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); >> + goto out; >> } >> >> - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ >> - if (num_possible_cpus() == 1) >> - return; >> + if (num_possible_cpus() == 1) { >> + pr_info("PV spinlocks disabled, single CPU.\n"); >> + goto out; >> + } >> + >> + if (nopvspin) { >> + pr_info("PV spinlocks disabled, forced by \"nopvspin\" parameter.\n"); >> + goto out; >> + } >> + >> + pr_info("PV spinlocks enabled\n"); >> >> __pv_init_lock_hash(); >> pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; >> @@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void) >> pv_ops.lock.vcpu_is_preempted = >> PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); >> } >> +out: >> + static_branch_disable(&virt_spin_lock_key); > You probably need to add 'return' before 'out:' as it seems you're > disabling virt_spin_lock_key in all cases now). virt_spin_lock_key is kept enabled in !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case which is the only case virt_spin_lock() optimization is used. When PV qspinlock is enabled, virt_spin_lock() isn't called in __pv_queued_spin_lock_slowpath() in which case we don't care virt_spin_lock_key's value. So adding 'return' or not are both ok, I chosed to save a line, let me know if you prefer to add a 'return' and I'll change it. btw: __pv_queued_spin_lock_slowpath() is alias of queued_spin_lock_slowpath() Thanks Zhenzhong
Zhenzhong Duan <zhenzhong.duan@oracle.com> writes: > Hi Vitaly, > > On 2019/10/22 19:36, 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 249f14a..3945aa5 100644 >>> --- a/arch/x86/kernel/kvm.c >>> +++ b/arch/x86/kernel/kvm.c >>> @@ -825,18 +825,36 @@ __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)) >>> + /* >>> + * In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an >>> + * advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is >>> + * preferred over native qspinlock when vCPU is preempted. >>> + */ >>> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { >>> + pr_info("PV spinlocks disabled, no host support.\n"); >>> return; >>> + } >>> >>> + /* >>> + * Disable PV qspinlock and use native qspinlock when dedicated pCPUs >>> + * are available. >>> + */ >>> if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { >>> - static_branch_disable(&virt_spin_lock_key); >>> - return; >>> + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); >>> + goto out; >>> } >>> >>> - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ >>> - if (num_possible_cpus() == 1) >>> - return; >>> + if (num_possible_cpus() == 1) { >>> + pr_info("PV spinlocks disabled, single CPU.\n"); >>> + goto out; >>> + } >>> + >>> + if (nopvspin) { >>> + pr_info("PV spinlocks disabled, forced by \"nopvspin\" parameter.\n"); >>> + goto out; >>> + } >>> + >>> + pr_info("PV spinlocks enabled\n"); >>> >>> __pv_init_lock_hash(); >>> pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; >>> @@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void) >>> pv_ops.lock.vcpu_is_preempted = >>> PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); >>> } >>> +out: >>> + static_branch_disable(&virt_spin_lock_key); >> You probably need to add 'return' before 'out:' as it seems you're >> disabling virt_spin_lock_key in all cases now). > > virt_spin_lock_key is kept enabled in !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) > case which is the only case virt_spin_lock() optimization is used. > > When PV qspinlock is enabled, virt_spin_lock() isn't called in > __pv_queued_spin_lock_slowpath() in which case we don't care > virt_spin_lock_key's value. > True, my bad: I though we still need it enabled for something. > So adding 'return' or not are both ok, I chosed to save a line, > let me know if you prefer to add a 'return' and I'll change it. No, please ignore. > > btw: __pv_queued_spin_lock_slowpath() is alias of queued_spin_lock_slowpath() > > Thanks > Zhenzhong >
On Tue, Oct 22, 2019 at 08:46:46PM +0800, Zhenzhong Duan wrote: > Hi Vitaly, > > On 2019/10/22 19:36, 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 249f14a..3945aa5 100644 > >>--- a/arch/x86/kernel/kvm.c > >>+++ b/arch/x86/kernel/kvm.c > >>@@ -825,18 +825,36 @@ __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)) > >>+ /* > >>+ * In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an > >>+ * advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is > >>+ * preferred over native qspinlock when vCPU is preempted. > >>+ */ > >>+ if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { > >>+ pr_info("PV spinlocks disabled, no host support.\n"); > >> return; > >>+ } > >>+ /* > >>+ * Disable PV qspinlock and use native qspinlock when dedicated pCPUs > >>+ * are available. > >>+ */ > >> if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { > >>- static_branch_disable(&virt_spin_lock_key); > >>- return; > >>+ pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); > >>+ goto out; > >> } > >>- /* Don't use the pvqspinlock code if there is only 1 vCPU. */ > >>- if (num_possible_cpus() == 1) > >>- return; > >>+ if (num_possible_cpus() == 1) { > >>+ pr_info("PV spinlocks disabled, single CPU.\n"); > >>+ goto out; > >>+ } > >>+ > >>+ if (nopvspin) { > >>+ pr_info("PV spinlocks disabled, forced by \"nopvspin\" parameter.\n"); > >>+ goto out; > >>+ } > >>+ > >>+ pr_info("PV spinlocks enabled\n"); > >> __pv_init_lock_hash(); > >> pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; > >>@@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void) > >> pv_ops.lock.vcpu_is_preempted = > >> PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); > >> } > >>+out: > >>+ static_branch_disable(&virt_spin_lock_key); > >You probably need to add 'return' before 'out:' as it seems you're > >disabling virt_spin_lock_key in all cases now). > > virt_spin_lock_key is kept enabled in !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) > case which is the only case virt_spin_lock() optimization is used. > > When PV qspinlock is enabled, virt_spin_lock() isn't called in > __pv_queued_spin_lock_slowpath() in which case we don't care > virt_spin_lock_key's value. > > So adding 'return' or not are both ok, I chosed to save a line, > let me know if you prefer to add a 'return' and I'll change it. It'd be worth adding a comment here if you end up spinning another version to change the logging prefix. The logic is sound and I like the end result, but I had the same knee jerk "this can't be right!?!?" reaction as Vitaly.
On 2019/10/23 5:03, Sean Christopherson wrote: > On Tue, Oct 22, 2019 at 08:46:46PM +0800, Zhenzhong Duan wrote: >> Hi Vitaly, >> >> On 2019/10/22 19:36, 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 249f14a..3945aa5 100644 >>>> --- a/arch/x86/kernel/kvm.c >>>> +++ b/arch/x86/kernel/kvm.c >>>> @@ -825,18 +825,36 @@ __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)) >>>> + /* >>>> + * In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an >>>> + * advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is >>>> + * preferred over native qspinlock when vCPU is preempted. >>>> + */ >>>> + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { >>>> + pr_info("PV spinlocks disabled, no host support.\n"); >>>> return; >>>> + } >>>> + /* >>>> + * Disable PV qspinlock and use native qspinlock when dedicated pCPUs >>>> + * are available. >>>> + */ >>>> if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { >>>> - static_branch_disable(&virt_spin_lock_key); >>>> - return; >>>> + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); >>>> + goto out; >>>> } >>>> - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ >>>> - if (num_possible_cpus() == 1) >>>> - return; >>>> + if (num_possible_cpus() == 1) { >>>> + pr_info("PV spinlocks disabled, single CPU.\n"); >>>> + goto out; >>>> + } >>>> + >>>> + if (nopvspin) { >>>> + pr_info("PV spinlocks disabled, forced by \"nopvspin\" parameter.\n"); >>>> + goto out; >>>> + } >>>> + >>>> + pr_info("PV spinlocks enabled\n"); >>>> __pv_init_lock_hash(); >>>> pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; >>>> @@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void) >>>> pv_ops.lock.vcpu_is_preempted = >>>> PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); >>>> } >>>> +out: >>>> + static_branch_disable(&virt_spin_lock_key); >>> You probably need to add 'return' before 'out:' as it seems you're >>> disabling virt_spin_lock_key in all cases now). >> virt_spin_lock_key is kept enabled in !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) >> case which is the only case virt_spin_lock() optimization is used. >> >> When PV qspinlock is enabled, virt_spin_lock() isn't called in >> __pv_queued_spin_lock_slowpath() in which case we don't care >> virt_spin_lock_key's value. >> >> So adding 'return' or not are both ok, I chosed to save a line, >> let me know if you prefer to add a 'return' and I'll change it. > It'd be worth adding a comment here if you end up spinning another version > to change the logging prefix. The logic is sound and I like the end > result, but I had the same knee jerk "this can't be right!?!?" reaction as > Vitaly. Sure, will do in next version. Thanks Zhenzhong
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a84a83f..bd49ed2 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -5334,6 +5334,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 249f14a..3945aa5 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -825,18 +825,36 @@ __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)) + /* + * In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an + * advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is + * preferred over native qspinlock when vCPU is preempted. + */ + if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { + pr_info("PV spinlocks disabled, no host support.\n"); return; + } + /* + * Disable PV qspinlock and use native qspinlock when dedicated pCPUs + * are available. + */ if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { - static_branch_disable(&virt_spin_lock_key); - return; + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); + goto out; } - /* Don't use the pvqspinlock code if there is only 1 vCPU. */ - if (num_possible_cpus() == 1) - return; + if (num_possible_cpus() == 1) { + pr_info("PV spinlocks disabled, single CPU.\n"); + goto out; + } + + if (nopvspin) { + pr_info("PV spinlocks disabled, forced by \"nopvspin\" parameter.\n"); + goto out; + } + + pr_info("PV spinlocks enabled\n"); __pv_init_lock_hash(); pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath; @@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void) pv_ops.lock.vcpu_is_preempted = PV_CALLEE_SAVE(__kvm_vcpu_is_preempted); } +out: + static_branch_disable(&virt_spin_lock_key); } #endif /* CONFIG_PARAVIRT_SPINLOCKS */ 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