lkml.org 
[lkml]   [2019]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v6 3/5] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
    Date
    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 | 34 ++++++++++++++++++++++---
    > kernel/locking/qspinlock.c | 7 +++++
    > 4 files changed, 43 insertions(+), 4 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..e9c76d8 100644
    > --- a/arch/x86/kernel/kvm.c
    > +++ b/arch/x86/kernel/kvm.c
    > @@ -825,18 +825,44 @@ __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))
    > + /*
    > + * PV spinlocks is disabled if no host side support, then native
    > + * qspinlock will be used. As native qspinlock is a fair lock, there is
    > + * lock holder preemption issue using it in a guest, imaging one pCPU
    > + * running 10 vCPUs of same guest contending same lock.
    > + *
    > + * virt_spin_lock() is introduced as an optimization for that scenario
    > + * which is enabled by virt_spin_lock_key key. To use that optimization,
    > + * virt_spin_lock_key isn't disabled here.
    > + */

    My take (if I properly understood what you say) would be:

    "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)) {
    > + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n");
    > + static_branch_disable(&virt_spin_lock_key);
    > + return;
    > + }
    > +
    > + if (num_possible_cpus() == 1) {
    > + pr_info("PV spinlocks disabled, single CPU.\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;

    You could've replaced this 'static_branch_disable(); return;' pattern
    with a goto to the end of the function to save a few lines but this
    looks good anyways.

    Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

    > + }
    > +
    > + 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

    --
    Vitaly

    \
     
     \ /
      Last update: 2019-10-21 13:14    [W:2.858 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site