| Message ID | 20161024150323.GB2247@potion (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/10/2016 17:03, Radim Krčmář wrote: > I have only compile-tested it, but it should optimize the switch and > also fix two bugs. The first one is major. > This needs that the deadline clearing in [5/5] is fixed. > ---8<--- > We must start the hrimer even if the expiration is already in the past, > otherwise the periodic timer would not rearm the hrtimer. > > And computing next expiration of a period timer does not require current > time. The period should be constant, so it is more precise to add the > period to the last expiration time. This fixes a time difference > between hrtimer expiration and apic->lapic_timer.target_expiration. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > arch/x86/kvm/lapic.c | 38 +++++++++++++------------------------- > 1 file changed, 13 insertions(+), 25 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index f4734fe12803..6244988418be 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1350,19 +1350,19 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic) > > static void start_sw_period(struct kvm_lapic *apic) > { > - ktime_t now; > - > - now = apic->lapic_timer.timer.base->get_time(); > - > if (!apic->lapic_timer.period) > return; > > - if (likely(ktime_after(apic->lapic_timer.target_expiration, now))) > - hrtimer_start(&apic->lapic_timer.timer, > - apic->lapic_timer.target_expiration, > - HRTIMER_MODE_ABS_PINNED); > - else > + if (apic_lvtt_oneshot(apic) && > + ktime_after(apic->lapic_timer.target_expiration, > + apic->lapic_timer.timer.base->get_time())) { > apic_timer_expired(apic); > + return; > + } > + > + hrtimer_start(&apic->lapic_timer.timer, > + apic->lapic_timer.target_expiration, > + HRTIMER_MODE_ABS_PINNED); > } > > static bool set_target_expiration(struct kvm_lapic *apic) > @@ -1414,10 +1414,11 @@ static bool set_target_expiration(struct kvm_lapic *apic) > > static void advance_periodic_target_expiration(struct kvm_lapic *apic) > { > - apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, rdtsc()) + > + apic->lapic_timer.tscdeadline += > nsec_to_cycles(apic->vcpu, apic->lapic_timer.period); > - apic->lapic_timer.target_expiration = ktime_add_ns(apic->lapic_timer.timer.base->get_time(), > - apic->lapic_timer.period); > + apic->lapic_timer.target_expiration = > + ktime_add_ns(apic->lapic_timer.target_expiration, > + apic->lapic_timer.period); > } > > bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu) > @@ -1481,19 +1482,6 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu) > > WARN_ON(apic->lapic_timer.hv_timer_in_use); > > - if (apic_lvtt_period(apic)) { > - ktime_t remaining, now; > - u64 tscl = rdtsc(); > - > - now = apic->lapic_timer.timer.base->get_time(); > - remaining = hrtimer_get_remaining(&apic->lapic_timer.timer); > - if (ktime_to_ns(remaining) < 0) > - remaining = ktime_set(0, 0); > - > - apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) + > - nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining)); > - apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining)); > - } > start_hv_timer(apic); > } > EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer); > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Go ahead, squash it into 5/5 and commit to kvm/queue. :) Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-10-24 17:09+0200, Paolo Bonzini: > On 24/10/2016 17:03, Radim Krčmář wrote: [...] > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Go ahead, squash it into 5/5 and commit to kvm/queue. :) Did that, thanks. Wanpeng, the code is now under your name so please check it and/or complain. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-10-24 23:27 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2016-10-24 17:09+0200, Paolo Bonzini: >> On 24/10/2016 17:03, Radim Krčmář wrote: > [...] >> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >> >> Go ahead, squash it into 5/5 and commit to kvm/queue. :) > > Did that, thanks. > > Wanpeng, the code is now under your name so please check it and/or > complain. This patch 6/5 incurred regressions. - The latency of the periodic mode which is emulated by VMX preemption is almost the same as periodic mode which is emulated by hrtimer. - The oneshot mode test of kvm-unit-tests/apic_timer_latency.flat almost fail. Btw, hope you can also apply the testcase for kvm-unit-tests. :) Regards, Wanpeng Li -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-10-25 12:48+0000, Wanpeng Li: >> 在 2016年10月24日，下午11:03，Radim Krčmář <rkrcmar@redhat.com> 写道： >> >> I have only compile-tested it, but it should optimize the switch and >> also fix two bugs. The first one is major. >> This needs that the deadline clearing in [5/5] is fixed. >> ---8<--- >> We must start the hrimer even if the expiration is already in the past, >> otherwise the periodic timer would not rearm the hrtimer. >> >> And computing next expiration of a period timer does not require current >> time. The period should be constant, so it is more precise to add the >> period to the last expiration time. This fixes a time difference >> between hrtimer expiration and apic->lapic_timer.target_expiration. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> @@ -1481,19 +1482,6 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu) >> >> WARN_ON(apic->lapic_timer.hv_timer_in_use); >> >> - if (apic_lvtt_period(apic)) { >> - ktime_t remaining, now; >> - u64 tscl = rdtsc(); >> - >> - now = apic->lapic_timer.timer.base->get_time(); >> - remaining = hrtimer_get_remaining(&apic->lapic_timer.timer); >> - if (ktime_to_ns(remaining) < 0) >> - remaining = ktime_set(0, 0); >> - >> - apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) + >> - nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining)); >> - apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining)); >> - } > > Why you remove this? We computed those values in advance_periodic_target_expiration() and can re-use them here. hrtimer expiration == apic->lapic_timer.target_expiration. (Otherwise we're doing something wrong.) If that holds then the code does target_expiration = now + (target_expiration - now) Which can be optimized to target_expiration = target_expiration and to nothing. The same principle holds for apic->lapic_timer.tscdeadline as well. In other words: It doesn't matter if the timer switches between hrtimer and VMX deadline -- the target expiration is still the same. This hunk only added imprecision, because kvm_read_l1_tsc() and ktime_to_ns() were not using the same time for computation. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index f4734fe12803..6244988418be 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1350,19 +1350,19 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic) static void start_sw_period(struct kvm_lapic *apic) { - ktime_t now; - - now = apic->lapic_timer.timer.base->get_time(); - if (!apic->lapic_timer.period) return; - if (likely(ktime_after(apic->lapic_timer.target_expiration, now))) - hrtimer_start(&apic->lapic_timer.timer, - apic->lapic_timer.target_expiration, - HRTIMER_MODE_ABS_PINNED); - else + if (apic_lvtt_oneshot(apic) && + ktime_after(apic->lapic_timer.target_expiration, + apic->lapic_timer.timer.base->get_time())) { apic_timer_expired(apic); + return; + } + + hrtimer_start(&apic->lapic_timer.timer, + apic->lapic_timer.target_expiration, + HRTIMER_MODE_ABS_PINNED); } static bool set_target_expiration(struct kvm_lapic *apic) @@ -1414,10 +1414,11 @@ static bool set_target_expiration(struct kvm_lapic *apic) static void advance_periodic_target_expiration(struct kvm_lapic *apic) { - apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, rdtsc()) + + apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, apic->lapic_timer.period); - apic->lapic_timer.target_expiration = ktime_add_ns(apic->lapic_timer.timer.base->get_time(), - apic->lapic_timer.period); + apic->lapic_timer.target_expiration = + ktime_add_ns(apic->lapic_timer.target_expiration, + apic->lapic_timer.period); } bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu) @@ -1481,19 +1482,6 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu) WARN_ON(apic->lapic_timer.hv_timer_in_use); - if (apic_lvtt_period(apic)) { - ktime_t remaining, now; - u64 tscl = rdtsc(); - - now = apic->lapic_timer.timer.base->get_time(); - remaining = hrtimer_get_remaining(&apic->lapic_timer.timer); - if (ktime_to_ns(remaining) < 0) - remaining = ktime_set(0, 0); - - apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) + - nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining)); - apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining)); - } start_hv_timer(apic); } EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
I have only compile-tested it, but it should optimize the switch and also fix two bugs. The first one is major. This needs that the deadline clearing in [5/5] is fixed. ---8<--- We must start the hrimer even if the expiration is already in the past, otherwise the periodic timer would not rearm the hrtimer. And computing next expiration of a period timer does not require current time. The period should be constant, so it is more precise to add the period to the last expiration time. This fixes a time difference between hrtimer expiration and apic->lapic_timer.target_expiration. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- arch/x86/kvm/lapic.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-)