On 9/26/2023 9:15 PM, Mingwei Zhang wrote:
> ah, typo in the subject: The 2nd KVM_REQ_NMI should be KVM_REQ_PMI.
> Sorry about that.
>
> On Tue, Sep 26, 2023 at 9:09 PM Mingwei Zhang <[email protected]> wrote:
>>
>> Move kvm_check_request(KVM_REQ_NMI) after kvm_check_request(KVM_REQ_NMI).
Please remove it, no need to repeat the subject.
>> When vPMU is active use, processing each KVM_REQ_PMI will generate a
>> KVM_REQ_NMI. Existing control flow after KVM_REQ_PMI finished will fail the
>> guest enter, jump to kvm_x86_cancel_injection(), and re-enter
>> vcpu_enter_guest(), this wasted lot of cycles and increase the overhead for
>> vPMU as well as the virtualization.
Optimization is after correctness, so please explain if this is correct
first!
>>
>> So move the code snippet of kvm_check_request(KVM_REQ_NMI) to make KVM
>> runloop more efficient with vPMU.
>>
>> To evaluate the effectiveness of this change, we launch a 8-vcpu QEMU VM on
>> an Intel SPR CPU. In the VM, we run perf with all 48 events Intel vtune
>> uses. In addition, we use SPEC2017 benchmark programs as the workload with
>> the setup of using single core, single thread.
>>
>> At the host level, we probe the invocations to vmx_cancel_injection() with
>> the following command:
>>
>> $ perf probe -a vmx_cancel_injection
>> $ perf stat -a -e probe:vmx_cancel_injection -I 10000 # per 10 seconds
>>
>> The following is the result that we collected at beginning of the spec2017
>> benchmark run (so mostly for 500.perlbench_r in spec2017). Kindly forgive
>> the incompleteness.
>>
>> On kernel without the change:
>> 10.010018010 14254 probe:vmx_cancel_injection
>> 20.037646388 15207 probe:vmx_cancel_injection
>> 30.078739816 15261 probe:vmx_cancel_injection
>> 40.114033258 15085 probe:vmx_cancel_injection
>> 50.149297460 15112 probe:vmx_cancel_injection
>> 60.185103088 15104 probe:vmx_cancel_injection
>>
>> On kernel with the change:
>> 10.003595390 40 probe:vmx_cancel_injection
>> 20.017855682 31 probe:vmx_cancel_injection
>> 30.028355883 34 probe:vmx_cancel_injection
>> 40.038686298 31 probe:vmx_cancel_injection
>> 50.048795162 20 probe:vmx_cancel_injection
>> 60.069057747 19 probe:vmx_cancel_injection
>>
>> From the above, it is clear that we save 1500 invocations per vcpu per
>> second to vmx_cancel_injection() for workloads like perlbench.
>>
>> Signed-off-by: Mingwei Zhang <[email protected]>
>> ---
>> arch/x86/kvm/x86.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 42a4e8f5e89a..302b6f8ddfb1 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10580,12 +10580,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> if (kvm_check_request(KVM_REQ_SMI, vcpu))
>> process_smi(vcpu);
>> #endif
>> - if (kvm_check_request(KVM_REQ_NMI, vcpu))
>> - process_nmi(vcpu);
>> if (kvm_check_request(KVM_REQ_PMU, vcpu))
>> kvm_pmu_handle_event(vcpu);
>> if (kvm_check_request(KVM_REQ_PMI, vcpu))
>> kvm_pmu_deliver_pmi(vcpu);
>> + if (kvm_check_request(KVM_REQ_NMI, vcpu))
>> + process_nmi(vcpu);
>> if (kvm_check_request(KVM_REQ_IOAPIC_EOI_EXIT, vcpu)) {
>> BUG_ON(vcpu->arch.pending_ioapic_eoi > 255);
>> if (test_bit(vcpu->arch.pending_ioapic_eoi,
>>
>> base-commit: 73554b29bd70546c1a9efc9c160641ef1b849358
>> --
>> 2.42.0.515.g380fc7ccd1-goog
>>
>
--
Thanks!
Xin
On Tue, Sep 26, 2023, Xin Li wrote:
> On 9/26/2023 9:15 PM, Mingwei Zhang wrote:
> > ah, typo in the subject: The 2nd KVM_REQ_NMI should be KVM_REQ_PMI.
> > Sorry about that.
> >
> > On Tue, Sep 26, 2023 at 9:09 PM Mingwei Zhang <[email protected]> wrote:
> > >
> > > Move kvm_check_request(KVM_REQ_NMI) after kvm_check_request(KVM_REQ_NMI).
>
> Please remove it, no need to repeat the subject.
Heh, from Documentation/process/maintainer-kvm-x86.rst:
Changelog
~~~~~~~~~
Most importantly, write changelogs using imperative mood and avoid pronouns.
See :ref:`describe_changes` for more information, with one amendment: lead with
a short blurb on the actual changes, and then follow up with the context and
background. Note! This order directly conflicts with the tip tree's preferred
approach! Please follow the tip tree's preferred style when sending patches
that primarily target arch/x86 code that is _NOT_ KVM code.
That said, I do prefer that the changelog intro isn't just a copy+paste of the
shortlog, and the shortlog and changelog should use conversational language instead
of describing the literal code movement.
> > > When vPMU is active use, processing each KVM_REQ_PMI will generate a
This is not guaranteed.
> > > KVM_REQ_NMI. Existing control flow after KVM_REQ_PMI finished will fail the
> > > guest enter, jump to kvm_x86_cancel_injection(), and re-enter
> > > vcpu_enter_guest(), this wasted lot of cycles and increase the overhead for
> > > vPMU as well as the virtualization.
As above, use conversational language, the changelog isn't meant to be a play-by-play.
E.g.
KVM: x86: Service NMI requests *after* PMI requests in VM-Enter path
Move the handling of NMI requests after PMI requests in the VM-Enter path
so that KVM doesn't need to cancel and redo VM-Enter in the likely
scenario that the vCPU has configured its LVPTC entry to generate an NMI.
Because APIC emulation "injects" NMIs via KVM_REQ_NMI, handling PMI
requests after NMI requests means KVM won't detect the pending NMI request
until the final check for outstanding requests. Detecting requests at the
final stage is costly as KVM has already loaded guest state, potentially
queued events for injection, disabled IRQs, dropped SRCU, etc., most of
which needs to be unwound.
> Optimization is after correctness, so please explain if this is correct
> first!
Not first. Leading with an in-depth description of KVM requests and NMI handling
is not going to help understand *why* this change is being first. But I do agree
that this should provide an analysis of why it's ok to swap the order, specificially
why it's architecturally ok if KVM drops an NMI due to the swapped ordering, e.g.
if the PMI is coincident with two other NMIs (or one other NMI and NMIs are blocked).
> > > So move the code snippet of kvm_check_request(KVM_REQ_NMI) to make KVM
> > > runloop more efficient with vPMU.
> > >
> > > To evaluate the effectiveness of this change, we launch a 8-vcpu QEMU VM on
Avoid pronouns. There's no need for all the "fluff", just state the setup, the
test, and the results.
Really getting into the nits, but the whole "8-vcpu QEMU VM" versus
"the setup of using single core, single thread" is confusing IMO. If there were
potential performance downsides and/or tradeoffs, then getting the gory details
might be necessary, but that's not the case here, and if it were really necessary
to drill down that deep, then I would want to better quantify the impact, e.g. in
terms latency.
E.g. on Intel SPR running SPEC2017 benchmark and Intel vtune in the guest,
handling PMI requests before NMI requests reduces the number of canceled
runs by ~1500 per second, per vCPU (counted by probing calls to
vmx_cancel_injection()).
> > > an Intel SPR CPU. In the VM, we run perf with all 48 events Intel vtune
> > > uses. In addition, we use SPEC2017 benchmark programs as the workload with
> > > the setup of using single core, single thread.
> > >
> > > At the host level, we probe the invocations to vmx_cancel_injection() with
> > > the following command:
> > >
> > > $ perf probe -a vmx_cancel_injection
> > > $ perf stat -a -e probe:vmx_cancel_injection -I 10000 # per 10 seconds
> > >
> > > The following is the result that we collected at beginning of the spec2017
> > > benchmark run (so mostly for 500.perlbench_r in spec2017). Kindly forgive
> > > the incompleteness.
> > >
> > > On kernel without the change:
> > > 10.010018010 14254 probe:vmx_cancel_injection
> > > 20.037646388 15207 probe:vmx_cancel_injection
> > > 30.078739816 15261 probe:vmx_cancel_injection
> > > 40.114033258 15085 probe:vmx_cancel_injection
> > > 50.149297460 15112 probe:vmx_cancel_injection
> > > 60.185103088 15104 probe:vmx_cancel_injection
> > >
> > > On kernel with the change:
> > > 10.003595390 40 probe:vmx_cancel_injection
> > > 20.017855682 31 probe:vmx_cancel_injection
> > > 30.028355883 34 probe:vmx_cancel_injection
> > > 40.038686298 31 probe:vmx_cancel_injection
> > > 50.048795162 20 probe:vmx_cancel_injection
> > > 60.069057747 19 probe:vmx_cancel_injection
> > >
> > > From the above, it is clear that we save 1500 invocations per vcpu per
> > > second to vmx_cancel_injection() for workloads like perlbench.
Nit, this really should have:
Suggested-by: Sean Christopherson <[email protected]>
I personally don't care about the attribution, but (a) others often do care and
(b) the added context is helpful. E.g. for bad/questionable suggestsions/ideas,
knowing that person X was also involved helps direct and/or curate questions/comments
accordingly.
On Wed, Sep 27, 2023 at 9:10 AM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Sep 26, 2023, Xin Li wrote:
> > On 9/26/2023 9:15 PM, Mingwei Zhang wrote:
> > > ah, typo in the subject: The 2nd KVM_REQ_NMI should be KVM_REQ_PMI.
> > > Sorry about that.
> > >
> > > On Tue, Sep 26, 2023 at 9:09 PM Mingwei Zhang <[email protected]> wrote:
> > > >
> > > > Move kvm_check_request(KVM_REQ_NMI) after kvm_check_request(KVM_REQ_NMI).
> >
> > Please remove it, no need to repeat the subject.
>
> Heh, from Documentation/process/maintainer-kvm-x86.rst:
>
> Changelog
> ~~~~~~~~~
> Most importantly, write changelogs using imperative mood and avoid pronouns.
>
> See :ref:`describe_changes` for more information, with one amendment: lead with
> a short blurb on the actual changes, and then follow up with the context and
> background. Note! This order directly conflicts with the tip tree's preferred
> approach! Please follow the tip tree's preferred style when sending patches
> that primarily target arch/x86 code that is _NOT_ KVM code.
>
> That said, I do prefer that the changelog intro isn't just a copy+paste of the
> shortlog, and the shortlog and changelog should use conversational language instead
> of describing the literal code movement.
>
> > > > When vPMU is active use, processing each KVM_REQ_PMI will generate a
>
> This is not guaranteed.
>
> > > > KVM_REQ_NMI. Existing control flow after KVM_REQ_PMI finished will fail the
> > > > guest enter, jump to kvm_x86_cancel_injection(), and re-enter
> > > > vcpu_enter_guest(), this wasted lot of cycles and increase the overhead for
> > > > vPMU as well as the virtualization.
>
> As above, use conversational language, the changelog isn't meant to be a play-by-play.
>
> E.g.
>
> KVM: x86: Service NMI requests *after* PMI requests in VM-Enter path
>
> Move the handling of NMI requests after PMI requests in the VM-Enter path
> so that KVM doesn't need to cancel and redo VM-Enter in the likely
> scenario that the vCPU has configured its LVPTC entry to generate an NMI.
>
> Because APIC emulation "injects" NMIs via KVM_REQ_NMI, handling PMI
> requests after NMI requests means KVM won't detect the pending NMI request
> until the final check for outstanding requests. Detecting requests at the
> final stage is costly as KVM has already loaded guest state, potentially
> queued events for injection, disabled IRQs, dropped SRCU, etc., most of
> which needs to be unwound.
>
> > Optimization is after correctness, so please explain if this is correct
> > first!
>
> Not first. Leading with an in-depth description of KVM requests and NMI handling
> is not going to help understand *why* this change is being first. But I do agree
> that this should provide an analysis of why it's ok to swap the order, specificially
> why it's architecturally ok if KVM drops an NMI due to the swapped ordering, e.g.
> if the PMI is coincident with two other NMIs (or one other NMI and NMIs are blocked).
>
> > > > So move the code snippet of kvm_check_request(KVM_REQ_NMI) to make KVM
> > > > runloop more efficient with vPMU.
> > > >
> > > > To evaluate the effectiveness of this change, we launch a 8-vcpu QEMU VM on
>
> Avoid pronouns. There's no need for all the "fluff", just state the setup, the
> test, and the results.
>
> Really getting into the nits, but the whole "8-vcpu QEMU VM" versus
> "the setup of using single core, single thread" is confusing IMO. If there were
> potential performance downsides and/or tradeoffs, then getting the gory details
> might be necessary, but that's not the case here, and if it were really necessary
> to drill down that deep, then I would want to better quantify the impact, e.g. in
> terms latency.
>
> E.g. on Intel SPR running SPEC2017 benchmark and Intel vtune in the guest,
> handling PMI requests before NMI requests reduces the number of canceled
> runs by ~1500 per second, per vCPU (counted by probing calls to
> vmx_cancel_injection()).
>
> > > > an Intel SPR CPU. In the VM, we run perf with all 48 events Intel vtune
> > > > uses. In addition, we use SPEC2017 benchmark programs as the workload with
> > > > the setup of using single core, single thread.
> > > >
> > > > At the host level, we probe the invocations to vmx_cancel_injection() with
> > > > the following command:
> > > >
> > > > $ perf probe -a vmx_cancel_injection
> > > > $ perf stat -a -e probe:vmx_cancel_injection -I 10000 # per 10 seconds
> > > >
> > > > The following is the result that we collected at beginning of the spec2017
> > > > benchmark run (so mostly for 500.perlbench_r in spec2017). Kindly forgive
> > > > the incompleteness.
> > > >
> > > > On kernel without the change:
> > > > 10.010018010 14254 probe:vmx_cancel_injection
> > > > 20.037646388 15207 probe:vmx_cancel_injection
> > > > 30.078739816 15261 probe:vmx_cancel_injection
> > > > 40.114033258 15085 probe:vmx_cancel_injection
> > > > 50.149297460 15112 probe:vmx_cancel_injection
> > > > 60.185103088 15104 probe:vmx_cancel_injection
> > > >
> > > > On kernel with the change:
> > > > 10.003595390 40 probe:vmx_cancel_injection
> > > > 20.017855682 31 probe:vmx_cancel_injection
> > > > 30.028355883 34 probe:vmx_cancel_injection
> > > > 40.038686298 31 probe:vmx_cancel_injection
> > > > 50.048795162 20 probe:vmx_cancel_injection
> > > > 60.069057747 19 probe:vmx_cancel_injection
> > > >
> > > > From the above, it is clear that we save 1500 invocations per vcpu per
> > > > second to vmx_cancel_injection() for workloads like perlbench.
>
> Nit, this really should have:
>
> Suggested-by: Sean Christopherson <[email protected]>
>
> I personally don't care about the attribution, but (a) others often do care and
> (b) the added context is helpful. E.g. for bad/questionable suggestsions/ideas,
> knowing that person X was also involved helps direct and/or curate questions/comments
> accordingly.
For sure! I will also pay more attention to that in the future.
Thanks.
-Mingwei