lkml.org 
[lkml]   [2020]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2] KVM: X86: Ultra fast single target IPI fastpath
    On Fri, Apr 10, 2020 at 09:03:27AM +0800, Wanpeng Li wrote:
    > From: Wanpeng Li <wanpengli@tencent.com>
    >
    > IPI and Timer cause the main MSRs write vmexits in cloud environment
    > observation, let's optimize virtual IPI latency more aggressively to
    > inject target IPI as soon as possible.
    >
    > Running kvm-unit-tests/vmexit.flat IPI testing on SKX server, disable
    > adaptive advance lapic timer and adaptive halt-polling to avoid the
    > interference, this patch can give another 7% improvement.
    >
    > w/o fastpath -> fastpath 4238 -> 3543 16.4%
    > fastpath -> ultra fastpath 3543 -> 3293 7%
    > w/o fastpath -> ultra fastpath 4238 -> 3293 22.3%
    >
    > This also revises the performance data in commit 1e9e2622a1 (KVM: VMX:
    > FIXED+PHYSICAL mode single target IPI fastpath), that testing adds
    > --overcommit cpu-pm=on to kvm-unit-tests guest which is unnecessary.
    >
    > Tested-by: Haiwei Li <lihaiwei@tencent.com>
    > Cc: Haiwei Li <lihaiwei@tencent.com>
    > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
    > ---
    > v1 -> v2:
    > * rebase on latest kvm/queue
    > * update patch description
    >
    > arch/x86/include/asm/kvm_host.h | 6 +++---
    > arch/x86/kvm/svm/svm.c | 21 ++++++++++++++-------
    > arch/x86/kvm/vmx/vmx.c | 19 +++++++++++++------
    > arch/x86/kvm/x86.c | 4 ++--
    > 4 files changed, 32 insertions(+), 18 deletions(-)
    >
    > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
    > index c7da23a..e667cf3 100644
    > --- a/arch/x86/include/asm/kvm_host.h
    > +++ b/arch/x86/include/asm/kvm_host.h
    > @@ -1124,7 +1124,8 @@ struct kvm_x86_ops {
    > */
    > void (*tlb_flush_guest)(struct kvm_vcpu *vcpu);
    >
    > - void (*run)(struct kvm_vcpu *vcpu);
    > + void (*run)(struct kvm_vcpu *vcpu,
    > + enum exit_fastpath_completion *exit_fastpath);
    > int (*handle_exit)(struct kvm_vcpu *vcpu,
    > enum exit_fastpath_completion exit_fastpath);
    > int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
    > @@ -1174,8 +1175,7 @@ struct kvm_x86_ops {
    > struct x86_instruction_info *info,
    > enum x86_intercept_stage stage,
    > struct x86_exception *exception);
    > - void (*handle_exit_irqoff)(struct kvm_vcpu *vcpu,
    > - enum exit_fastpath_completion *exit_fastpath);
    > + void (*handle_exit_irqoff)(struct kvm_vcpu *vcpu);
    >
    > int (*check_nested_events)(struct kvm_vcpu *vcpu);
    > void (*request_immediate_exit)(struct kvm_vcpu *vcpu);
    > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
    > index 27f4684..c019332 100644
    > --- a/arch/x86/kvm/svm/svm.c
    > +++ b/arch/x86/kvm/svm/svm.c
    > @@ -3283,9 +3283,20 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
    > svm_complete_interrupts(svm);
    > }
    >
    > +static enum exit_fastpath_completion svm_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
    > +{
    > + if (!is_guest_mode(vcpu) &&
    > + to_svm(vcpu)->vmcb->control.exit_code == SVM_EXIT_MSR &&
    > + to_svm(vcpu)->vmcb->control.exit_info_1)
    > + return handle_fastpath_set_msr_irqoff(vcpu);
    > +
    > + return EXIT_FASTPATH_NONE;
    > +}
    > +
    > bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
    >
    > -static void svm_vcpu_run(struct kvm_vcpu *vcpu)
    > +static void svm_vcpu_run(struct kvm_vcpu *vcpu,
    > + enum exit_fastpath_completion *exit_fastpath)
    > {
    > struct vcpu_svm *svm = to_svm(vcpu);
    >
    > @@ -3388,6 +3399,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
    > kvm_load_host_xsave_state(vcpu);
    > stgi();
    >
    > + *exit_fastpath = svm_exit_handlers_fastpath(vcpu);
    > /* Any pending NMI will happen here */
    >
    > if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_NMI))
    > @@ -3719,13 +3731,8 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
    > return ret;
    > }
    >
    > -static void svm_handle_exit_irqoff(struct kvm_vcpu *vcpu,
    > - enum exit_fastpath_completion *exit_fastpath)
    > +static void svm_handle_exit_irqoff(struct kvm_vcpu *vcpu)
    > {
    > - if (!is_guest_mode(vcpu) &&
    > - to_svm(vcpu)->vmcb->control.exit_code == SVM_EXIT_MSR &&
    > - to_svm(vcpu)->vmcb->control.exit_info_1)
    > - *exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
    > }
    >
    > static void svm_sched_in(struct kvm_vcpu *vcpu, int cpu)
    > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
    > index 1d2bb57..61a1725 100644
    > --- a/arch/x86/kvm/vmx/vmx.c
    > +++ b/arch/x86/kvm/vmx/vmx.c
    > @@ -6354,8 +6354,7 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
    > }
    > STACK_FRAME_NON_STANDARD(handle_external_interrupt_irqoff);
    >
    > -static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
    > - enum exit_fastpath_completion *exit_fastpath)
    > +static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu)
    > {
    > struct vcpu_vmx *vmx = to_vmx(vcpu);
    >
    > @@ -6363,9 +6362,6 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
    > handle_external_interrupt_irqoff(vcpu);
    > else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
    > handle_exception_nmi_irqoff(vmx);
    > - else if (!is_guest_mode(vcpu) &&
    > - vmx->exit_reason == EXIT_REASON_MSR_WRITE)
    > - *exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
    > }
    >
    > static bool vmx_has_emulated_msr(int index)
    > @@ -6570,9 +6566,19 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
    > }
    > }
    >
    > +static enum exit_fastpath_completion vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
    > +{
    > + if (!is_guest_mode(vcpu) &&
    > + to_vmx(vcpu)->exit_reason == EXIT_REASON_MSR_WRITE)

    Bad indentation.

    > + return handle_fastpath_set_msr_irqoff(vcpu);
    > +
    > + return EXIT_FASTPATH_NONE;
    > +}
    > +
    > bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
    >
    > -static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
    > +static void vmx_vcpu_run(struct kvm_vcpu *vcpu,
    > + enum exit_fastpath_completion *exit_fastpath)

    Why pass a pointer instead of returning the enum?

    > {
    > struct vcpu_vmx *vmx = to_vmx(vcpu);
    > unsigned long cr3, cr4;
    > @@ -6737,6 +6743,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
    > vmx->idt_vectoring_info = 0;
    >
    > vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
    > + *exit_fastpath = vmx_exit_handlers_fastpath(vcpu);

    IMO, this should come at the very end of vmx_vcpu_run(). At a minimum, it
    needs to be moved below the #MC handling and below

    if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
    return;

    KVM more or less assumes vmx->idt_vectoring_info is always valid, and it's
    not obvious that a generic fastpath call can safely run before
    vmx_complete_interrupts(), e.g. the kvm_clear_interrupt_queue() call.

    In a normal scenario, the added latency is <50 cycles. ~30 for the VMREAD
    of IDT_VECTORING_INFO_FIELD, a handful of zeroing instructions, and a few
    CMP+Jcc style uops to skip NMI blocking and interrupt completion.

    And if the result is returned, it means VMX won't need a local variable, e.g.:

    vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
    if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
    kvm_machine_check();

    if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
    return EXIT_FASTPATH_NONE;

    vmx->loaded_vmcs->launched = 1;
    vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);

    vmx_recover_nmi_blocking(vmx);
    vmx_complete_interrupts(vmx);

    return vmx_exit_handlers_fastpath(vcpu);
    }

    > if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
    > kvm_machine_check();
    >
    > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
    > index 3089aa4..eed31e2 100644
    > --- a/arch/x86/kvm/x86.c
    > +++ b/arch/x86/kvm/x86.c
    > @@ -8409,7 +8409,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
    > vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
    > }
    >
    > - kvm_x86_ops.run(vcpu);
    > + kvm_x86_ops.run(vcpu, &exit_fastpath);

    Pretty sre
    >
    > /*
    > * Do this here before restoring debug registers on the host. And
    > @@ -8441,7 +8441,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
    > vcpu->mode = OUTSIDE_GUEST_MODE;
    > smp_wmb();
    >
    > - kvm_x86_ops.handle_exit_irqoff(vcpu, &exit_fastpath);
    > + kvm_x86_ops.handle_exit_irqoff(vcpu);
    >
    > /*
    > * Consume any pending interrupts, including the possible source of
    > --
    > 2.7.4
    >

    \
     
     \ /
      Last update: 2020-04-10 17:36    [W:2.587 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site