Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1544405 > unrolled thread

[RFC PATCH v3 0/6] KVM: x86: avoid redundant REQ_EVENT

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v3 0/6] KVM: x86: avoid redundant REQ_EVENT Paolo Bonzini <pbonzini@redhat.com> - 2016-12-19 10:50 +0100
    [PATCH 4/6] KVM: lapic: avoid unnecessary KVM_REQ_EVENT on IRR scan Paolo Bonzini <pbonzini@redhat.com> - 2016-12-19 10:50 +0100
    [PATCH 1/6] KVM: x86: add VCPU stat for KVM_REQ_EVENT processing Paolo Bonzini <pbonzini@redhat.com> - 2016-12-19 11:00 +0100
      Re: [PATCH 1/6] KVM: x86: add VCPU stat for KVM_REQ_EVENT processing Paolo Bonzini <pbonzini@redhat.com> - 2016-12-20 10:40 +0100
    [PATCH 2/6] KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on EOI Paolo Bonzini <pbonzini@redhat.com> - 2016-12-19 11:00 +0100
      Re: [PATCH 2/6] KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on  EOI Paolo Bonzini <pbonzini@redhat.com> - 2016-12-23 13:10 +0100
    Re: [RFC PATCH v3 0/6] KVM: x86: avoid redundant REQ_EVENT Paolo Bonzini <pbonzini@redhat.com> - 2016-12-19 16:10 +0100

#1544405 — [RFC PATCH v3 0/6] KVM: x86: avoid redundant REQ_EVENT

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

[toc] | [next] | [standalone]

#1544406 — [PATCH 4/6] KVM: lapic: avoid unnecessary KVM_REQ_EVENT on IRR scan

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 <pbonzini@redhat.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1544413 — [PATCH 1/6] KVM: x86: add VCPU stat for KVM_REQ_EVENT processing

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1545004 — Re: [PATCH 1/6] KVM: x86: add VCPU stat for KVM_REQ_EVENT processing

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 <pbonzini@redhat.com>
>> ---
>>  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

[toc] | [prev] | [next] | [standalone]

#1544414 — [PATCH 2/6] KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on EOI

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 <pbonzini@redhat.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1546844 — Re: [PATCH 2/6] KVM: lapic: do not set KVM_REQ_EVENT unnecessarily on EOI

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 <pbonzini@redhat.com>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1544604

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web