This is the result of cleaning up the places that set REQ_EVENT unnecessarily.
The savings on self-IPI kvm-unit-tests are:
self_ipi_sti_nop ~300 clock cycles
self_ipi_sti_hlt ~300 clock cycles
self_ipi_tpr ~400 clock cycles
self_ipi_tpr_sti_nop ~700 clock cycles
self_ipi_tpr_sti_hlt ~700 clock cycles
which is in the 5-10% range. Please help me comparing this series with
"[PATCH v2] KVM: x86: avoid redundant REQ_EVENT" on the real workload!
Thanks,
Paolo
Paolo Bonzini (6):
KVM: x86: add VCPU stat for KVM_REQ_EVENT processing
KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on EOI
KVM: vmx: speed up TPR below threshold vmexits
KVM: lapic: remove unnecessary KVM_REQ_EVENT on PPR update
KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on PPR update
KVM: lapic: do not scan IRR when delivering an interrupt
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/lapic.c | 58 +++++++++++++++++++++++++----------------
arch/x86/kvm/lapic.h | 1 +
arch/x86/kvm/vmx.c | 2 +-
arch/x86/kvm/x86.c | 2 ++
5 files changed, 41 insertions(+), 23 deletions(-)
--
1.8.3.1
Since we're already in VCPU context, all we have to do here is recompute
the PPR value. That will in turn generate a KVM_REQ_EVENT if necessary.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/lapic.c | 6 ++++++
arch/x86/kvm/lapic.h | 1 +
arch/x86/kvm/vmx.c | 2 +-
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 70c7428b7a57..ae59e655cd6d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -595,6 +595,12 @@ static void apic_update_ppr(struct kvm_lapic *apic)
}
}
+void kvm_apic_update_ppr(struct kvm_vcpu *vcpu)
+{
+ apic_update_ppr(vcpu->arch.apic);
+}
+EXPORT_SYMBOL_GPL(kvm_apic_update_ppr);
+
static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
{
kvm_lapic_set_reg(apic, APIC_TASKPRI, tpr);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index cb16e6fd2330..5b5b1ba644cb 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -73,6 +73,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
void __kvm_apic_update_irr(u32 *pir, void *regs);
void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
+void kvm_apic_update_ppr(struct kvm_vcpu *vcpu);
int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
struct dest_map *dest_map);
int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 24db5fb6f575..5fb54e3de061 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6158,7 +6158,7 @@ static int handle_wrmsr(struct kvm_vcpu *vcpu)
static int handle_tpr_below_threshold(struct kvm_vcpu *vcpu)
{
- kvm_make_request(KVM_REQ_EVENT, vcpu);
+ kvm_apic_update_ppr(vcpu);
return 1;
}
--
1.8.3.1
On interrupt delivery the PPR can only grow, so it is impossible
that interrupt delivery results in KVM_REQ_EVENT. Make this
clear by using __apic_update_ppr, and by not using apic_*_isr
for Hyper-V auto-EOI interrupts.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/lapic.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index dc4ea8bdea9c..4dc02482faf7 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2110,6 +2110,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
{
int vector = kvm_apic_has_interrupt(vcpu);
struct kvm_lapic *apic = vcpu->arch.apic;
+ u32 ppr;
if (vector == -1)
return -1;
@@ -2121,15 +2122,11 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
* because the process would deliver it through the IDT.
*/
- apic_set_isr(vector, apic);
- apic_update_ppr(apic);
- apic_clear_irr(vector, apic);
-
- if (test_bit(vector, vcpu_to_synic(vcpu)->auto_eoi_bitmap)) {
- apic_clear_isr(vector, apic);
- apic_update_ppr(apic);
- }
+ if (!test_bit(vector, vcpu_to_synic(vcpu)->auto_eoi_bitmap))
+ apic_set_isr(vector, apic);
+ apic_clear_irr(vector, apic);
+ __apic_update_ppr(apic, &ppr);
return vector;
}
--
1.8.3.1
On PPR update, we set KVM_REQ_EVENT unconditionally anytime PPR is lowered.
But we can take into account IRR here already.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/lapic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f936644d8011..dc4ea8bdea9c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -607,7 +607,8 @@ static void apic_update_ppr(struct kvm_lapic *apic)
{
u32 ppr;
- if (__apic_update_ppr(apic, &ppr))
+ if (__apic_update_ppr(apic, &ppr) &&
+ apic_has_interrupt_for_ppr(apic, ppr) != -1)
kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
}
--
1.8.3.1
PPR needs to be updated whenever on every IRR read because we may have
missed TPR writes that _increased_ PPR. However, these writes need not
generate KVM_REQ_EVENT, because KVM_REQ_EVENT has been set already in
__apic_accept_irq, by apic_update_ppr when TPR was lowered, or again by
apic_update_ppr when EOI cleared a bit in ISR.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/lapic.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ae59e655cd6d..f936644d8011 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -570,7 +570,15 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
}
-static void apic_update_ppr(struct kvm_lapic *apic)
+static int apic_has_interrupt_for_ppr(struct kvm_lapic *apic, u32 ppr)
+{
+ int highest_irr = apic_find_highest_irr(apic);
+ if (highest_irr == -1 || (highest_irr & 0xF0) <= ppr)
+ return -1;
+ return highest_irr;
+}
+
+static bool __apic_update_ppr(struct kvm_lapic *apic, u32 *new_ppr)
{
u32 tpr, isrv, ppr, old_ppr;
int isr;
@@ -588,11 +596,19 @@ static void apic_update_ppr(struct kvm_lapic *apic)
apic_debug("vlapic %p, ppr 0x%x, isr 0x%x, isrv 0x%x",
apic, ppr, isr, isrv);
- if (old_ppr != ppr) {
+ *new_ppr = ppr;
+ if (old_ppr != ppr)
kvm_lapic_set_reg(apic, APIC_PROCPRI, ppr);
- if (ppr < old_ppr)
- kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
- }
+
+ return ppr < old_ppr;
+}
+
+static void apic_update_ppr(struct kvm_lapic *apic)
+{
+ u32 ppr;
+
+ if (__apic_update_ppr(apic, &ppr))
+ kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
}
void kvm_apic_update_ppr(struct kvm_vcpu *vcpu)
@@ -2051,17 +2067,13 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
{
struct kvm_lapic *apic = vcpu->arch.apic;
- int highest_irr;
+ u32 ppr;
if (!apic_enabled(apic))
return -1;
- apic_update_ppr(apic);
- highest_irr = apic_find_highest_irr(apic);
- if ((highest_irr == -1) ||
- ((highest_irr & 0xF0) <= kvm_lapic_get_reg(apic, APIC_PROCPRI)))
- return -1;
- return highest_irr;
+ __apic_update_ppr(apic, &ppr);
+ return apic_has_interrupt_for_ppr(apic, ppr);
}
int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
--
1.8.3.1
On EOI, there is no need to set KVM_REQ_EVENT unconditionally. The PPR
update is already setting it if resetting the ISR bit causes PPR to
decrease. Even a level-triggered IOAPIC interrupt will set KVM_REQ_EVENT
on a reinjection (ioapic_service -> kvm_irq_delivery_to_apic and from
there to __apic_accept_irq).
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/lapic.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e35cbd44c505..70c7428b7a57 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1066,7 +1066,6 @@ static int apic_set_eoi(struct kvm_lapic *apic)
kvm_hv_synic_send_eoi(apic->vcpu, vector);
kvm_ioapic_send_eoi(apic, vector);
- kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
return vector;
}
@@ -1081,7 +1080,6 @@ void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
trace_kvm_eoi(apic, vector);
kvm_ioapic_send_eoi(apic, vector);
- kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
}
EXPORT_SYMBOL_GPL(kvm_apic_set_eoi_accelerated);
--
1.8.3.1
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/x86.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 994d8ed9fc6c..08cfd45a9452 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -850,6 +850,7 @@ struct kvm_vcpu_stat {
u64 hypercalls;
u64 irq_injections;
u64 nmi_injections;
+ u64 req_event;
};
struct x86_instruction_info;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e95d94edbdc3..e9b512090865 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -180,6 +180,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
{ "insn_emulation_fail", VCPU_STAT(insn_emulation_fail) },
{ "irq_injections", VCPU_STAT(irq_injections) },
{ "nmi_injections", VCPU_STAT(nmi_injections) },
+ { "req_event", VCPU_STAT(req_event) },
{ "mmu_shadow_zapped", VM_STAT(mmu_shadow_zapped) },
{ "mmu_pte_write", VM_STAT(mmu_pte_write) },
{ "mmu_pte_updated", VM_STAT(mmu_pte_updated) },
@@ -6691,6 +6692,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
}
if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
+ ++vcpu->stat.req_event;
kvm_apic_accept_events(vcpu);
if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
r = 1;
--
1.8.3.1
On 19.12.2016 12:47, Paolo Bonzini wrote:
> This is the result of cleaning up the places that set REQ_EVENT unnecessarily.
> The savings on self-IPI kvm-unit-tests are:
>
> self_ipi_sti_nop ~300 clock cycles
> self_ipi_sti_hlt ~300 clock cycles
> self_ipi_tpr ~400 clock cycles
> self_ipi_tpr_sti_nop ~700 clock cycles
> self_ipi_tpr_sti_hlt ~700 clock cycles
>
> which is in the 5-10% range. Please help me comparing this series with
> "[PATCH v2] KVM: x86: avoid redundant REQ_EVENT" on the real workload!
>
> Thanks,
>
> Paolo
This new patch-set does avoid unnecessary interrupt re-injections - checked.
The test (MS Windows, sending messages between multiple windows using
windows-specific interface),
which showed performance growth with "[PATCH v2] KVM: x86: avoid
redundant REQ_EVENT" showed pretty much the same performance level with
this new patch-set.
The test score difference (+2.4% to [PATCH v2]) is within the tolerance
range(5%).
The test score on mainstream v4.9 kernel CPU Intel i5-2400, guest
Windows Server 2012 2VCPU, 2GB:
without patch: 31510 (+/- 4%)
with patch: 36270 (+/- 5%)
difference = (36270-31510)/31510 * 100 = +15% -- looks good!
Thanks!
>
> Paolo Bonzini (6):
> KVM: x86: add VCPU stat for KVM_REQ_EVENT processing
> KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on EOI
> KVM: vmx: speed up TPR below threshold vmexits
> KVM: lapic: remove unnecessary KVM_REQ_EVENT on PPR update
> KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on PPR update
> KVM: lapic: do not scan IRR when delivering an interrupt
>
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/lapic.c | 58 +++++++++++++++++++++++++----------------
> arch/x86/kvm/lapic.h | 1 +
> arch/x86/kvm/vmx.c | 2 +-
> arch/x86/kvm/x86.c | 2 ++
> 5 files changed, 41 insertions(+), 23 deletions(-)
>
--
Best,
Denis
On 19/12/2016 14:05, Denis Plotnikov wrote:
>>
> This new patch-set does avoid unnecessary interrupt re-injections -
> checked.
>
> The test (MS Windows, sending messages between multiple windows using
> windows-specific interface),
> which showed performance growth with "[PATCH v2] KVM: x86: avoid
> redundant REQ_EVENT" showed pretty much the same performance level with
> this new patch-set.
> The test score difference (+2.4% to [PATCH v2]) is within the tolerance
> range(5%).
>
> The test score on mainstream v4.9 kernel CPU Intel i5-2400, guest
> Windows Server 2012 2VCPU, 2GB:
>
> without patch: 31510 (+/- 4%)
> with patch: 36270 (+/- 5%)
>
> difference = (36270-31510)/31510 * 100 = +15% -- looks good!
Awesome! I hope it also qualifies as less ugly. :)
Paolo
On 20/12/2016 10:21, Roman Kagan wrote:
> On Mon, Dec 19, 2016 at 10:47:13AM +0100, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 +
>> arch/x86/kvm/x86.c | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 994d8ed9fc6c..08cfd45a9452 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -850,6 +850,7 @@ struct kvm_vcpu_stat {
>> u64 hypercalls;
>> u64 irq_injections;
>> u64 nmi_injections;
>> + u64 req_event;
>> };
>>
>> struct x86_instruction_info;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index e95d94edbdc3..e9b512090865 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -180,6 +180,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>> { "insn_emulation_fail", VCPU_STAT(insn_emulation_fail) },
>> { "irq_injections", VCPU_STAT(irq_injections) },
>> { "nmi_injections", VCPU_STAT(nmi_injections) },
>> + { "req_event", VCPU_STAT(req_event) },
>> { "mmu_shadow_zapped", VM_STAT(mmu_shadow_zapped) },
>> { "mmu_pte_write", VM_STAT(mmu_pte_write) },
>> { "mmu_pte_updated", VM_STAT(mmu_pte_updated) },
>> @@ -6691,6 +6692,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> }
>>
>> if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
>> + ++vcpu->stat.req_event;
>> kvm_apic_accept_events(vcpu);
>> if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> r = 1;
>
> Just curious what kind of information you expect to be able to extract
> from this counter? I mean, I'm not opposed to introducing it, I just
> want to figure out how to use it properly. Compare against
> irq_injections?
Yes, exactly. Since vmexit cycle counts are always a bit noisy, I could
compare irq_injections against req_event and get an idea of the expected
improvement.
Also for example the stat shows that the sti_hlt case has a higher # of
req_event than sti_nop. I have a patch to fix that, but it's left for
later because I don't know of a workload that triggers it.
I don't really need this patch of course.
> Also I guess it may be interesting to count branching to
> cancel_injection label in vcpu_enter_guest.
Interesting one too.
Paolo
On Mon, Dec 19, 2016 at 03:30:17PM +0100, Paolo Bonzini wrote:
>
>
> On 19/12/2016 14:05, Denis Plotnikov wrote:
> >>
> > This new patch-set does avoid unnecessary interrupt re-injections -
> > checked.
> >
> > The test (MS Windows, sending messages between multiple windows using
> > windows-specific interface),
> > which showed performance growth with "[PATCH v2] KVM: x86: avoid
> > redundant REQ_EVENT" showed pretty much the same performance level with
> > this new patch-set.
> > The test score difference (+2.4% to [PATCH v2]) is within the tolerance
> > range(5%).
> >
> > The test score on mainstream v4.9 kernel CPU Intel i5-2400, guest
> > Windows Server 2012 2VCPU, 2GB:
> >
> > without patch: 31510 (+/- 4%)
> > with patch: 36270 (+/- 5%)
> >
> > difference = (36270-31510)/31510 * 100 = +15% -- looks good!
>
> Awesome! I hope it also qualifies as less ugly. :)
Absolutely. FWIW
Reviewed-by: Roman Kagan <[email protected]>
Roman.
On 19/12/2016 10:47, Paolo Bonzini wrote:
> + if (!test_bit(vector, vcpu_to_synic(vcpu)->auto_eoi_bitmap))
> + apic_set_isr(vector, apic);
>
> + apic_clear_irr(vector, apic);
> + __apic_update_ppr(apic, &ppr);
Hmm, EOI does apic_update_ppr, so for auto-EOI interrupts I think it's
safer to do apic_update_ppr instead. You could have to interrupts
injected at the same time, and the lower-priority interrupt would be
lost if the higher-priority interrupt does automatic EOI.
Paolo
On 19/12/2016 10:47, Paolo Bonzini wrote:
> On EOI, there is no need to set KVM_REQ_EVENT unconditionally. The PPR
> update is already setting it if resetting the ISR bit causes PPR to
> decrease. Even a level-triggered IOAPIC interrupt will set KVM_REQ_EVENT
> on a reinjection (ioapic_service -> kvm_irq_delivery_to_apic and from
> there to __apic_accept_irq).
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/lapic.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index e35cbd44c505..70c7428b7a57 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1066,7 +1066,6 @@ static int apic_set_eoi(struct kvm_lapic *apic)
> kvm_hv_synic_send_eoi(apic->vcpu, vector);
>
> kvm_ioapic_send_eoi(apic, vector);
> - kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> return vector;
> }
>
> @@ -1081,7 +1080,6 @@ void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
> trace_kvm_eoi(apic, vector);
>
> kvm_ioapic_send_eoi(apic, vector);
> - kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> }
> EXPORT_SYMBOL_GPL(kvm_apic_set_eoi_accelerated);
>
>
This patch breaks Windows XP and 2003 (reverting it while keeping the
others results in 3 successful installs, compared to a failure rate of
12/13 with the patch included).
Paolo