diff mbox series

[v2,2/3] KVM: LAPIC: lapic timer interrupt is injected by posted interrupt

Message ID 1559799086-13912-3-git-send-email-wanpengli@tencent.com (mailing list archive)
State New, archived
Headers show
Series KVM: LAPIC: Implement Exitless Timer | expand

Commit Message

Wanpeng Li June 6, 2019, 5:31 a.m. UTC 
From: Wanpeng Li <wanpengli@tencent.com>

Dedicated instances are currently disturbed by unnecessary jitter due 
to the emulated lapic timers fire on the same pCPUs which vCPUs resident.
There is no hardware virtual timer on Intel for guest like ARM. Both 
programming timer in guest and the emulated timer fires incur vmexits.
This patch tries to avoid vmexit which is incurred by the emulated 
timer fires in dedicated instance scenario. 

When nohz_full is enabled in dedicated instances scenario, the emulated 
timers can be offload to the nearest busy housekeeping cpus since APICv 
is really common in recent years. The guest timer interrupt is injected 
by posted-interrupt which is delivered by housekeeping cpu once the emulated 
timer fires. 

3%~5% redis performance benefit can be observed on Skylake server.

Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/lapic.c | 32 +++++++++++++++++++++++++-------
 arch/x86/kvm/x86.h   |  5 +++++
 2 files changed, 30 insertions(+), 7 deletions(-)

Comments

Radim Krčmář June 10, 2019, 4:51 p.m. UTC | #1 
2019-06-06 13:31+0800, Wanpeng Li:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Dedicated instances are currently disturbed by unnecessary jitter due 
> to the emulated lapic timers fire on the same pCPUs which vCPUs resident.
> There is no hardware virtual timer on Intel for guest like ARM. Both 
> programming timer in guest and the emulated timer fires incur vmexits.
> This patch tries to avoid vmexit which is incurred by the emulated 
> timer fires in dedicated instance scenario. 
> 
> When nohz_full is enabled in dedicated instances scenario, the emulated 
> timers can be offload to the nearest busy housekeeping cpus since APICv 
> is really common in recent years. The guest timer interrupt is injected 
> by posted-interrupt which is delivered by housekeeping cpu once the emulated 
> timer fires. 
> 
> 3%~5% redis performance benefit can be observed on Skylake server.
> 
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/lapic.c | 32 +++++++++++++++++++++++++-------
>  arch/x86/kvm/x86.h   |  5 +++++
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 09b7387..c08e5a8 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -133,6 +133,12 @@ static inline bool posted_interrupt_inject_timer_enabled(struct kvm_vcpu *vcpu)
>  		kvm_mwait_in_guest(vcpu->kvm);
>  }
>  
> +static inline bool can_posted_interrupt_inject_timer(struct kvm_vcpu *vcpu)
> +{
> +	return posted_interrupt_inject_timer_enabled(vcpu) &&
> +		!vcpu_halt_in_guest(vcpu);

It would make more sense to have a condition for general blocking in
KVM, but keep in mind that we're not running on the same cpu anymore, so
any code like that has to be properly protected against VM entries under
our hands.  (The VCPU could appear halted here, but before we get make
the timer pending, the VCPU would enter and potentially never check the
interrupt.)

I think we should be able to simply do

  if (posted_interrupt_inject_timer_enabled(vcpu))
  	kvm_inject_apic_timer_irqs();

directly in the apic_timer_expired() as the injection will wake up the
target if necessary.  It's going to be a bit slow for timer callback in
those (too slow to warrant special handling?), but there hopefully
aren't any context restrictions in place.


Thanks.
Wanpeng Li June 11, 2019, 8:18 a.m. UTC | #2 
On Tue, 11 Jun 2019 at 00:51, Radim Krčmář <rkrcmar@redhat.com> wrote:
>
> 2019-06-06 13:31+0800, Wanpeng Li:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Dedicated instances are currently disturbed by unnecessary jitter due
> > to the emulated lapic timers fire on the same pCPUs which vCPUs resident.
> > There is no hardware virtual timer on Intel for guest like ARM. Both
> > programming timer in guest and the emulated timer fires incur vmexits.
> > This patch tries to avoid vmexit which is incurred by the emulated
> > timer fires in dedicated instance scenario.
> >
> > When nohz_full is enabled in dedicated instances scenario, the emulated
> > timers can be offload to the nearest busy housekeeping cpus since APICv
> > is really common in recent years. The guest timer interrupt is injected
> > by posted-interrupt which is delivered by housekeeping cpu once the emulated
> > timer fires.
> >
> > 3%~5% redis performance benefit can be observed on Skylake server.
> >
> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> > ---
> >  arch/x86/kvm/lapic.c | 32 +++++++++++++++++++++++++-------
> >  arch/x86/kvm/x86.h   |  5 +++++
> >  2 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 09b7387..c08e5a8 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -133,6 +133,12 @@ static inline bool posted_interrupt_inject_timer_enabled(struct kvm_vcpu *vcpu)
> >               kvm_mwait_in_guest(vcpu->kvm);
> >  }
> >
> > +static inline bool can_posted_interrupt_inject_timer(struct kvm_vcpu *vcpu)
> > +{
> > +     return posted_interrupt_inject_timer_enabled(vcpu) &&
> > +             !vcpu_halt_in_guest(vcpu);
>
> It would make more sense to have a condition for general blocking in
> KVM, but keep in mind that we're not running on the same cpu anymore, so
> any code like that has to be properly protected against VM entries under
> our hands.  (The VCPU could appear halted here, but before we get make
> the timer pending, the VCPU would enter and potentially never check the
> interrupt.)
>
> I think we should be able to simply do
>
>   if (posted_interrupt_inject_timer_enabled(vcpu))
>         kvm_inject_apic_timer_irqs();
>
> directly in the apic_timer_expired() as the injection will wake up the
> target if necessary.  It's going to be a bit slow for timer callback in
> those (too slow to warrant special handling?), but there hopefully
> aren't any context restrictions in place.

The vCPU halt status is used to handle non-PV apf notification aware
guest which is pointed out by Paolo.
https://lkml.org/lkml/2019/6/5/436 The vCPU will not re-vmentry w/
timer interrupt even if there is a kick since vcpu->arch.apf.halted is
true and it can't escape from kvm_vcpu_check_block().

Regards,
Wanpeng Li
Paolo Bonzini June 11, 2019, 11:40 a.m. UTC | #3 
On 06/06/19 07:31, Wanpeng Li wrote:
> +static inline bool can_posted_interrupt_inject_timer(struct kvm_vcpu *vcpu)
> +{
> +	return posted_interrupt_inject_timer_enabled(vcpu) &&
> +		!vcpu_halt_in_guest(vcpu);
> +}
> +

I agree with Radim, what you want here is just use kvm_hlt_in_guest.

I'll post shortly a prerequisite patch to block APF artificial halt when
kvm_hlt_in_guest is true.

Paolo
Wanpeng Li June 11, 2019, 12:18 p.m. UTC | #4 
On Tue, 11 Jun 2019 at 19:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 06/06/19 07:31, Wanpeng Li wrote:
> > +static inline bool can_posted_interrupt_inject_timer(struct kvm_vcpu *vcpu)
> > +{
> > +     return posted_interrupt_inject_timer_enabled(vcpu) &&
> > +             !vcpu_halt_in_guest(vcpu);
> > +}
> > +
>
> I agree with Radim, what you want here is just use kvm_hlt_in_guest.

Do it in v3.

>
> I'll post shortly a prerequisite patch to block APF artificial halt when
> kvm_hlt_in_guest is true.

Thanks Paolo!

Regards,
Wanpeng Li
diff mbox series

Patch

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 09b7387..c08e5a8 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -133,6 +133,12 @@  static inline bool posted_interrupt_inject_timer_enabled(struct kvm_vcpu *vcpu)
 		kvm_mwait_in_guest(vcpu->kvm);
 }
 
+static inline bool can_posted_interrupt_inject_timer(struct kvm_vcpu *vcpu)
+{
+	return posted_interrupt_inject_timer_enabled(vcpu) &&
+		!vcpu_halt_in_guest(vcpu);
+}
+
 static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
 		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
 	switch (map->mode) {
@@ -1441,6 +1447,19 @@  static void apic_update_lvtt(struct kvm_lapic *apic)
 	}
 }
 
+static void kvm_apic_inject_pending_timer_irqs(struct kvm_lapic *apic)
+{
+	struct kvm_timer *ktimer = &apic->lapic_timer;
+
+	kvm_apic_local_deliver(apic, APIC_LVTT);
+	if (apic_lvtt_tscdeadline(apic))
+		ktimer->tscdeadline = 0;
+	if (apic_lvtt_oneshot(apic)) {
+		ktimer->tscdeadline = 0;
+		ktimer->target_expiration = 0;
+	}
+}
+
 static void apic_timer_expired(struct kvm_lapic *apic)
 {
 	struct kvm_vcpu *vcpu = apic->vcpu;
@@ -1450,6 +1469,11 @@  static void apic_timer_expired(struct kvm_lapic *apic)
 	if (atomic_read(&apic->lapic_timer.pending))
 		return;
 
+	if (unlikely(can_posted_interrupt_inject_timer(apic->vcpu))) {
+		kvm_apic_inject_pending_timer_irqs(apic);
+		return;
+	}
+
 	atomic_inc(&apic->lapic_timer.pending);
 	kvm_set_pending_timer(vcpu);
 
@@ -2386,13 +2410,7 @@  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
 	if (atomic_read(&apic->lapic_timer.pending) > 0) {
-		kvm_apic_local_deliver(apic, APIC_LVTT);
-		if (apic_lvtt_tscdeadline(apic))
-			apic->lapic_timer.tscdeadline = 0;
-		if (apic_lvtt_oneshot(apic)) {
-			apic->lapic_timer.tscdeadline = 0;
-			apic->lapic_timer.target_expiration = 0;
-		}
+		kvm_apic_inject_pending_timer_irqs(apic);
 		atomic_set(&apic->lapic_timer.pending, 0);
 	}
 }
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index aa539d6..74c86cb 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -364,4 +364,9 @@  static inline bool kvm_pat_valid(u64 data)
 void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu);
 void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu);
 
+static inline bool vcpu_halt_in_guest(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.apf.halted;
+}
+
 #endif