Re: [PATCH v3 4/5] KVM: LAPIC: Delay trace advance expire delta
On Mon, May 20, 2019 at 02:38:44PM +0800, Wanpeng Li wrote: > On Sat, 18 May 2019 at 03:44, Sean Christopherson > wrote: > > This needs to be guarded with lapic_in_kernel(vcpu). But, since this is > > all in the same flow, a better approach would be to return the delta from > > wait_lapic_expire(). That saves 8 bytes in struct kvm_timer and avoids > > additional checks for tracing the delta. > > As you know, the function wait_lapic_expire() will be moved to vmx.c > and svm.c, so this is not suitable any more. Doh, I was too excited about my cleverness and completely forgot why you were moving the tracepoint in the first place.
Re: [PATCH v3 4/5] KVM: LAPIC: Delay trace advance expire delta
On Sat, 18 May 2019 at 03:44, Sean Christopherson wrote: > > On Thu, May 16, 2019 at 11:06:19AM +0800, Wanpeng Li wrote: > > From: Wanpeng Li > > > > wait_lapic_expire() call was moved above guest_enter_irqoff() because of > > its tracepoint, which violated the RCU extended quiescent state invoked > > by guest_enter_irqoff()[1][2]. This patch simply moves the tracepoint > > below guest_exit_irqoff() in vcpu_enter_guest(). Snapshot the delta before > > VM-Enter, but trace it after VM-Exit. This can help us to move > > wait_lapic_expire() just before vmentry in the later patch. > > > > [1] Commit 8b89fe1f6c43 ("kvm: x86: move tracepoints outside extended > > quiescent state") > > [2] https://patchwork.kernel.org/patch/782/ > > > > Cc: Paolo Bonzini > > Cc: Radim Krčmář > > Cc: Sean Christopherson > > Cc: Liran Alon > > Signed-off-by: Wanpeng Li > > --- > > arch/x86/kvm/lapic.c | 16 > > arch/x86/kvm/lapic.h | 1 + > > arch/x86/kvm/x86.c | 2 ++ > > 3 files changed, 11 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 2f364fe..af38ece 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1502,27 +1502,27 @@ static inline void __wait_lapic_expire(struct > > kvm_vcpu *vcpu, u64 guest_cycles) > > } > > > > static inline void adaptive_tune_timer_advancement(struct kvm_vcpu *vcpu, > > - u64 guest_tsc, u64 tsc_deadline) > > + s64 advance_expire_delta) > > { > > struct kvm_lapic *apic = vcpu->arch.apic; > > u32 timer_advance_ns = apic->lapic_timer.timer_advance_ns; > > u64 ns; > > > > /* too early */ > > - if (guest_tsc < tsc_deadline) { > > - ns = (tsc_deadline - guest_tsc) * 100ULL; > > + if (advance_expire_delta < 0) { > > + ns = -advance_expire_delta * 100ULL; > > do_div(ns, vcpu->arch.virtual_tsc_khz); > > timer_advance_ns -= min((u32)ns, > > timer_advance_ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP); > > } else { > > /* too late */ > > - ns = (guest_tsc - tsc_deadline) * 100ULL; > > + ns = advance_expire_delta * 100ULL; > > do_div(ns, vcpu->arch.virtual_tsc_khz); > > timer_advance_ns += min((u32)ns, > > timer_advance_ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP); > > } > > > > - if (abs(guest_tsc - tsc_deadline) < LAPIC_TIMER_ADVANCE_ADJUST_DONE) > > + if (abs(advance_expire_delta) < LAPIC_TIMER_ADVANCE_ADJUST_DONE) > > apic->lapic_timer.timer_advance_adjust_done = true; > > if (unlikely(timer_advance_ns > 5000)) { > > timer_advance_ns = 0; > > @@ -1545,13 +1545,13 @@ void wait_lapic_expire(struct kvm_vcpu *vcpu) > > tsc_deadline = apic->lapic_timer.expired_tscdeadline; > > apic->lapic_timer.expired_tscdeadline = 0; > > guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()); > > - trace_kvm_wait_lapic_expire(vcpu->vcpu_id, guest_tsc - tsc_deadline); > > + apic->lapic_timer.advance_expire_delta = guest_tsc - tsc_deadline; > > > > - if (guest_tsc < tsc_deadline) > > + if (apic->lapic_timer.advance_expire_delta < 0) > > I'd prefer to keep "guest_tsc < tsc_deadline" here, just so that it's > obvious that the call to __wait_lapic_expire() is safe. My eyes did a > few double takes reading this code :-) Ok. > > > __wait_lapic_expire(vcpu, tsc_deadline - guest_tsc); > > > > if (unlikely(!apic->lapic_timer.timer_advance_adjust_done)) > > - adaptive_tune_timer_advancement(vcpu, guest_tsc, > > tsc_deadline); > > + adaptive_tune_timer_advancement(vcpu, > > apic->lapic_timer.advance_expire_delta); > > } > > > > static void start_sw_tscdeadline(struct kvm_lapic *apic) > > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > > index d6d049b..3e72a25 100644 > > --- a/arch/x86/kvm/lapic.h > > +++ b/arch/x86/kvm/lapic.h > > @@ -32,6 +32,7 @@ struct kvm_timer { > > u64 tscdeadline; > > u64 expired_tscdeadline; > > u32 timer_advance_ns; > > + s64 advance_expire_delta; > > atomic_t pending; /* accumulated triggered > > timers */ > > bool hv_timer_in_use; > > bool timer_advance_adjust_done; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index f2e3847..4a7b00c 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -7961,6 +7961,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > ++vcpu->stat.exits; > > > > guest_exit_irqoff(); > > + trace_kvm_wait_lapic_expire(vcpu->vcpu_id, > > + vcpu->arch.apic->lapic_timer.advance_expire_delta); > > This needs to be guarded with lapic_in_kernel(vcpu). But, since this is > all in the same flow, a better approach would be to return the delta from > wait_lapic_expire(). That saves 8 byte
Re: [PATCH v3 4/5] KVM: LAPIC: Delay trace advance expire delta
On Thu, May 16, 2019 at 11:06:19AM +0800, Wanpeng Li wrote: > From: Wanpeng Li > > wait_lapic_expire() call was moved above guest_enter_irqoff() because of > its tracepoint, which violated the RCU extended quiescent state invoked > by guest_enter_irqoff()[1][2]. This patch simply moves the tracepoint > below guest_exit_irqoff() in vcpu_enter_guest(). Snapshot the delta before > VM-Enter, but trace it after VM-Exit. This can help us to move > wait_lapic_expire() just before vmentry in the later patch. > > [1] Commit 8b89fe1f6c43 ("kvm: x86: move tracepoints outside extended > quiescent state") > [2] https://patchwork.kernel.org/patch/782/ > > Cc: Paolo Bonzini > Cc: Radim Krčmář > Cc: Sean Christopherson > Cc: Liran Alon > Signed-off-by: Wanpeng Li > --- > arch/x86/kvm/lapic.c | 16 > arch/x86/kvm/lapic.h | 1 + > arch/x86/kvm/x86.c | 2 ++ > 3 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 2f364fe..af38ece 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1502,27 +1502,27 @@ static inline void __wait_lapic_expire(struct > kvm_vcpu *vcpu, u64 guest_cycles) > } > > static inline void adaptive_tune_timer_advancement(struct kvm_vcpu *vcpu, > - u64 guest_tsc, u64 tsc_deadline) > + s64 advance_expire_delta) > { > struct kvm_lapic *apic = vcpu->arch.apic; > u32 timer_advance_ns = apic->lapic_timer.timer_advance_ns; > u64 ns; > > /* too early */ > - if (guest_tsc < tsc_deadline) { > - ns = (tsc_deadline - guest_tsc) * 100ULL; > + if (advance_expire_delta < 0) { > + ns = -advance_expire_delta * 100ULL; > do_div(ns, vcpu->arch.virtual_tsc_khz); > timer_advance_ns -= min((u32)ns, > timer_advance_ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP); > } else { > /* too late */ > - ns = (guest_tsc - tsc_deadline) * 100ULL; > + ns = advance_expire_delta * 100ULL; > do_div(ns, vcpu->arch.virtual_tsc_khz); > timer_advance_ns += min((u32)ns, > timer_advance_ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP); > } > > - if (abs(guest_tsc - tsc_deadline) < LAPIC_TIMER_ADVANCE_ADJUST_DONE) > + if (abs(advance_expire_delta) < LAPIC_TIMER_ADVANCE_ADJUST_DONE) > apic->lapic_timer.timer_advance_adjust_done = true; > if (unlikely(timer_advance_ns > 5000)) { > timer_advance_ns = 0; > @@ -1545,13 +1545,13 @@ void wait_lapic_expire(struct kvm_vcpu *vcpu) > tsc_deadline = apic->lapic_timer.expired_tscdeadline; > apic->lapic_timer.expired_tscdeadline = 0; > guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()); > - trace_kvm_wait_lapic_expire(vcpu->vcpu_id, guest_tsc - tsc_deadline); > + apic->lapic_timer.advance_expire_delta = guest_tsc - tsc_deadline; > > - if (guest_tsc < tsc_deadline) > + if (apic->lapic_timer.advance_expire_delta < 0) I'd prefer to keep "guest_tsc < tsc_deadline" here, just so that it's obvious that the call to __wait_lapic_expire() is safe. My eyes did a few double takes reading this code :-) > __wait_lapic_expire(vcpu, tsc_deadline - guest_tsc); > > if (unlikely(!apic->lapic_timer.timer_advance_adjust_done)) > - adaptive_tune_timer_advancement(vcpu, guest_tsc, tsc_deadline); > + adaptive_tune_timer_advancement(vcpu, > apic->lapic_timer.advance_expire_delta); > } > > static void start_sw_tscdeadline(struct kvm_lapic *apic) > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index d6d049b..3e72a25 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -32,6 +32,7 @@ struct kvm_timer { > u64 tscdeadline; > u64 expired_tscdeadline; > u32 timer_advance_ns; > + s64 advance_expire_delta; > atomic_t pending; /* accumulated triggered timers > */ > bool hv_timer_in_use; > bool timer_advance_adjust_done; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index f2e3847..4a7b00c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7961,6 +7961,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > ++vcpu->stat.exits; > > guest_exit_irqoff(); > + trace_kvm_wait_lapic_expire(vcpu->vcpu_id, > + vcpu->arch.apic->lapic_timer.advance_expire_delta); This needs to be guarded with lapic_in_kernel(vcpu). But, since this is all in the same flow, a better approach would be to return the delta from wait_lapic_expire(). That saves 8 bytes in struct kvm_timer and avoids additional checks for tracing the delta. E.g.: s64 lapic_expire_delta; ... if (lapic_in_kernel(vcpu) && vcpu->arch.apic->lapic_timer.timer_advance_ns) lapic_expire_delta = wait_lapic