| Message ID | 20161025114334.GD3197@potion (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25/10/2016 13:43, Radim Krčmář wrote: > Oops, silly mistake -- apic_timer_expired() was in the 'else' branch in > [5/5] and I didn't invert the condition after moving it. > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 6244988418be..d7e74c8ec8ca 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1354,8 +1354,8 @@ static void start_sw_period(struct kvm_lapic *apic) > return; > > if (apic_lvtt_oneshot(apic) && > - ktime_after(apic->lapic_timer.target_expiration, > - apic->lapic_timer.timer.base->get_time())) { > + !ktime_after(apic->lapic_timer.target_expiration, > + apic->lapic_timer.timer.base->get_time())) { > apic_timer_expired(apic); > return; > } > > Paolo, can you squash that? Yes, will do. 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-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2016-10-25 07:39+0800, Wanpeng Li: >> 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. > > Hm, what numbers are you getting? The two fixes look good to me. However, the codes which you remove in kvm_lapic_switch_to_hv_timer() results in different numbers. w/o remove hlt average latency = 2398462 w/ remove hlt average latency = 2403845 > > When I ran the test with the original series, then it actually had worse Did you test this by running my kvm-unit-tests/apic_timer_latency.flat? > results with the VMX preemption than it did with the hrimer: > > hlt average latency = 1464151 > pause average latency = 1467605 > > htl tests the hrtimer, pause tests the VMX preemption. I just replaced > "hlt" with "pause" in the assembly loop. > > The worse result was because the VMX preemption period was computed > incorrectly -- it was being added to now(). Some time passes between > the expiration and reading of now(), so this time was extending the > period while it shouldn't have. > > If I run the test with [6/5], it gets sane numbers: > > hlt average latency = 1465107 > pause average latency = 1465093 > > The numbers are sane bacause the test is not computing latency (= how > long after the timer should have fired have we received the interrupt) > -- it is computing the duration of the period in cycles, which is much > better right now. Agreed. > >> - The oneshot mode test of kvm-unit-tests/apic_timer_latency.flat almost fail. > > Oops, silly mistake -- apic_timer_expired() was in the 'else' branch in > [5/5] and I didn't invert the condition after moving it. > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 6244988418be..d7e74c8ec8ca 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1354,8 +1354,8 @@ static void start_sw_period(struct kvm_lapic *apic) > return; > > if (apic_lvtt_oneshot(apic) && > - ktime_after(apic->lapic_timer.target_expiration, > - apic->lapic_timer.timer.base->get_time())) { > + !ktime_after(apic->lapic_timer.target_expiration, > + apic->lapic_timer.timer.base->get_time())) { > apic_timer_expired(apic); > return; > } > It works. > Paolo, can you squash that? > >> Btw, hope you can also apply the testcase for kvm-unit-tests. :) > > I will have some comments, because it would be nicer if it measured the > latency ... expected_expiration is not computed correctly. It measured the latency from guest programs the clock event device to interrupt injected to guest after timer fire. 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-26 14:02 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > 2016-10-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> 2016-10-25 07:39+0800, Wanpeng Li: >>> 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. >> >> Hm, what numbers are you getting? > > The two fixes look good to me. However, the codes which you remove in > kvm_lapic_switch_to_hv_timer() results in different numbers. > > w/o remove hlt average latency = 2398462 > w/ remove hlt average latency = 2403845 > >> >> When I ran the test with the original series, then it actually had worse > > Did you test this by running my kvm-unit-tests/apic_timer_latency.flat? > >> results with the VMX preemption than it did with the hrimer: >> >> hlt average latency = 1464151 >> pause average latency = 1467605 >> >> htl tests the hrtimer, pause tests the VMX preemption. I just replaced >> "hlt" with "pause" in the assembly loop. >> >> The worse result was because the VMX preemption period was computed >> incorrectly -- it was being added to now(). Some time passes between >> the expiration and reading of now(), so this time was extending the >> period while it shouldn't have. >> >> If I run the test with [6/5], it gets sane numbers: >> >> hlt average latency = 1465107 >> pause average latency = 1465093 >> >> The numbers are sane bacause the test is not computing latency (= how >> long after the timer should have fired have we received the interrupt) >> -- it is computing the duration of the period in cycles, which is much >> better right now. > > Agreed. > >> >>> - The oneshot mode test of kvm-unit-tests/apic_timer_latency.flat almost fail. >> >> Oops, silly mistake -- apic_timer_expired() was in the 'else' branch in >> [5/5] and I didn't invert the condition after moving it. >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 6244988418be..d7e74c8ec8ca 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -1354,8 +1354,8 @@ static void start_sw_period(struct kvm_lapic *apic) >> return; >> >> if (apic_lvtt_oneshot(apic) && >> - ktime_after(apic->lapic_timer.target_expiration, >> - apic->lapic_timer.timer.base->get_time())) { >> + !ktime_after(apic->lapic_timer.target_expiration, >> + apic->lapic_timer.timer.base->get_time())) { >> apic_timer_expired(apic); >> return; >> } >> > > It works. > >> Paolo, can you squash that? >> >>> Btw, hope you can also apply the testcase for kvm-unit-tests. :) >> >> I will have some comments, because it would be nicer if it measured the >> latency ... expected_expiration is not computed correctly. > > It measured the latency from guest programs the clock event device to > interrupt injected to guest after timer fire. When compare this with clock event device which is emulated by hrtimer, we can calculate the latency bonus from VMX preemption. 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 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2016-10-25 07:39+0800, Wanpeng Li: >> 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. > > Hm, what numbers are you getting? > > When I ran the test with the original series, then it actually had worse > results with the VMX preemption than it did with the hrimer: > > hlt average latency = 1464151 > pause average latency = 1467605 > > htl tests the hrtimer, pause tests the VMX preemption. I just replaced > "hlt" with "pause" in the assembly loop. > > The worse result was because the VMX preemption period was computed > incorrectly -- it was being added to now(). Some time passes between > the expiration and reading of now(), so this time was extending the > period while it shouldn't have. > > If I run the test with [6/5], it gets sane numbers: > > hlt average latency = 1465107 > pause average latency = 1465093 > > The numbers are sane bacause the test is not computing latency (= how > long after the timer should have fired have we received the interrupt) > -- it is computing the duration of the period in cycles, which is much > better right now. > >> - The oneshot mode test of kvm-unit-tests/apic_timer_latency.flat almost fail. > > Oops, silly mistake -- apic_timer_expired() was in the 'else' branch in > [5/5] and I didn't invert the condition after moving it. > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 6244988418be..d7e74c8ec8ca 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1354,8 +1354,8 @@ static void start_sw_period(struct kvm_lapic *apic) > return; > > if (apic_lvtt_oneshot(apic) && > - ktime_after(apic->lapic_timer.target_expiration, > - apic->lapic_timer.timer.base->get_time())) { > + !ktime_after(apic->lapic_timer.target_expiration, > + apic->lapic_timer.timer.base->get_time())) { > apic_timer_expired(apic); > return; > } > > Paolo, can you squash that? It seems that squash is impossible since the dependency of current kvm/queue(KVM: x86: use ktime_get instead of seeking the hrtimer_clock_base), I will send out a separate patch to fix this. 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
On 26/10/2016 12:23, Wanpeng Li wrote: >> > >> > if (apic_lvtt_oneshot(apic) && >> > - ktime_after(apic->lapic_timer.target_expiration, >> > - apic->lapic_timer.timer.base->get_time())) { >> > + !ktime_after(apic->lapic_timer.target_expiration, >> > + apic->lapic_timer.timer.base->get_time())) { >> > apic_timer_expired(apic); >> > return; >> > } >> > >> > Paolo, can you squash that? > It seems that squash is impossible since the dependency of current > kvm/queue(KVM: x86: use ktime_get instead of seeking the > hrtimer_clock_base), I will send out a separate patch to fix this. It is squashed in: + if (apic_lvtt_oneshot(apic) && + ktime_after(apic->lapic_timer.timer.base->get_time(), + apic->lapic_timer.target_expiration)) { + apic_timer_expired(apic); + return; + } Notice that the order of the arguments is inverted (alternatively I could have used ktime_before). 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-26 19:15 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>: > > > On 26/10/2016 12:23, Wanpeng Li wrote: >>> > >>> > if (apic_lvtt_oneshot(apic) && >>> > - ktime_after(apic->lapic_timer.target_expiration, >>> > - apic->lapic_timer.timer.base->get_time())) { >>> > + !ktime_after(apic->lapic_timer.target_expiration, >>> > + apic->lapic_timer.timer.base->get_time())) { >>> > apic_timer_expired(apic); >>> > return; >>> > } >>> > >>> > Paolo, can you squash that? >> It seems that squash is impossible since the dependency of current >> kvm/queue(KVM: x86: use ktime_get instead of seeking the >> hrtimer_clock_base), I will send out a separate patch to fix this. > > It is squashed in: > > + if (apic_lvtt_oneshot(apic) && > + ktime_after(apic->lapic_timer.timer.base->get_time(), > + apic->lapic_timer.target_expiration)) { > + apic_timer_expired(apic); > + return; > + } > > Notice that the order of the arguments is inverted (alternatively I > could have used ktime_before). Indeed. 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-26 14:02+0800, Wanpeng Li: > 2016-10-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> 2016-10-25 07:39+0800, Wanpeng Li: >>> 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. >> >> Hm, what numbers are you getting? > > The two fixes look good to me. However, the codes which you remove in > kvm_lapic_switch_to_hv_timer() results in different numbers. Which of those two results is closer to the expected duration of the period? > w/o remove hlt average latency = 2398462 > w/ remove hlt average latency = 2403845 Some increase is expected when removing the code, because kvm_lapic_switch_to_hv_timer() decreased the period by mistake: it called now = get_time() first and then did remaining = target - get_time() // = hrtimer_get_remaining() but some time has passed in between calls of get_time(), let's call the time that passed in between as "delta", so when the function later set the new target, new_target = now + remaining // = now + target - (now + delta) the new_target was "delta" earlier. 5k cycles is a huge difference, though ... You tested the original kvm_lapic_switch_to_hv_timer(), with fixed advance_periodic_target_expiration()? >> When I ran the test with the original series, then it actually had worse > > Did you test this by running my kvm-unit-tests/apic_timer_latency.flat? Yes, I used numbers from Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz, which had TSC calibrated to 2397.223 MHz, so the expected "average latency" with with the default 0x100000 ns period was 0x100000 * 2.397223 - 0x100000 = 1465094.5044479999 The expected value is pretty close to what I actually measured: >> [...] >> If I run the test with [6/5], it gets sane numbers: >> >> hlt average latency = 1465107 >> pause average latency = 1465093 -- 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-26 14:08+0800, Wanpeng Li: > 2016-10-26 14:02 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: >> 2016-10-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >>> I will have some comments, because it would be nicer if it measured the >>> latency ... expected_expiration is not computed correctly. >> >> It measured the latency from guest programs the clock event device to >> interrupt injected to guest after timer fire. No. It never computed the time when the timer fires, the test measured the duration of the period. Imagine that the dashed line below is a timeline. Pipe is idealized firing of the periodic timer and caret is the time when the guest read time in the interrupt. The number below caret is the latency. The period is 7. -------------------------------------------- | | | | | | | ^ ^ ^ ^ ^ ^ ^ 1 2 3 1 2 1 1 The test would report "latencies" as: 1 1 1 -2 1 -1 0 because it used now() + period to compute the next expected expiration Similarly in this case, -------------------------------------------- | | | | | | | ^ ^ ^ ^ ^ ^ 6 6 6 6 6 6 The latency is always 6, but the test would report 6 0 0 0 0 0 And if we improved the latency by 1, you'd only see the difference in the first number. The test measured the duration of the period. > When compare this with clock event device which is emulated by > hrtimer, we can calculate the latency bonus from VMX preemption. If we know when the timer should have fired, then we can measure the latency: latency = now() - expected_expiration The hard part is computing expected_expiration() -- it *cannot* be precise with one-shot or periodic APIC timer, because we don't send expected_expiration to KVM, but only a delta and KVM sets expected_expiration based on the delta and some random time when it gets to set the expected_expiration. The guest could do before = now() set_apic_timer(delta) after = now() to get some bounds on the expected expiration -- it would be between "before + delta" and "after + delta". -- 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-26 21:32 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2016-10-26 14:02+0800, Wanpeng Li: >> 2016-10-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >>> 2016-10-25 07:39+0800, Wanpeng Li: >>>> 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. >>> >>> Hm, what numbers are you getting? >> >> The two fixes look good to me. However, the codes which you remove in >> kvm_lapic_switch_to_hv_timer() results in different numbers. > > Which of those two results is closer to the expected duration of the > period? The result of w/ remove is more closer to the expected duration. > >> w/o remove hlt average latency = 2398462 >> w/ remove hlt average latency = 2403845 > > Some increase is expected when removing the code, because > kvm_lapic_switch_to_hv_timer() decreased the period by mistake: > it called > > now = get_time() > > first and then did > > remaining = target - get_time() // = hrtimer_get_remaining() > > but some time has passed in between calls of get_time(), let's call the > time that passed in between as "delta", so when the function later set > the new target, > > new_target = now + remaining // = now + target - (now + delta) > > the new_target was "delta" earlier. Agreed. > > 5k cycles is a huge difference, though ... Yeah, delta can't be as large as 5k cycles. > You tested the original kvm_lapic_switch_to_hv_timer(), with fixed > advance_periodic_target_expiration()? Yes. > >>> When I ran the test with the original series, then it actually had worse >> >> Did you test this by running my kvm-unit-tests/apic_timer_latency.flat? > > Yes, I used numbers from Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz, > which had TSC calibrated to 2397.223 MHz, so the expected "average > latency" with with the default 0x100000 ns period was > > 0x100000 * 2.397223 - 0x100000 = 1465094.5044479999 I agree with your remove the logic in kvm_lapic_switch_to_hv_timer() since it is more closer to the expected "average latency" now. 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-26 22:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2016-10-26 14:08+0800, Wanpeng Li: >> 2016-10-26 14:02 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: >>> 2016-10-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >>>> I will have some comments, because it would be nicer if it measured the >>>> latency ... expected_expiration is not computed correctly. >>> >>> It measured the latency from guest programs the clock event device to >>> interrupt injected to guest after timer fire. > > No. It never computed the time when the timer fires, the test measured > the duration of the period. > > Imagine that the dashed line below is a timeline. Pipe is idealized > firing of the periodic timer and caret is the time when the guest read > time in the interrupt. The number below caret is the latency. > > The period is 7. > > -------------------------------------------- > | | | | | | | > ^ ^ ^ ^ ^ ^ ^ > 1 2 3 1 2 1 1 > > The test would report "latencies" as: > > 1 1 1 -2 1 -1 0 > > because it used now() + period to compute the next expected expiration > > Similarly in this case, > -------------------------------------------- > | | | | | | | > ^ ^ ^ ^ ^ ^ > 6 6 6 6 6 6 > > The latency is always 6, but the test would report > > 6 0 0 0 0 0 > > And if we improved the latency by 1, you'd only see the difference in > the first number. The test measured the duration of the period. Agreed, thanks for the details. :) 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
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 6244988418be..d7e74c8ec8ca 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1354,8 +1354,8 @@ static void start_sw_period(struct kvm_lapic *apic) return; if (apic_lvtt_oneshot(apic) && - ktime_after(apic->lapic_timer.target_expiration, - apic->lapic_timer.timer.base->get_time())) { + !ktime_after(apic->lapic_timer.target_expiration, + apic->lapic_timer.timer.base->get_time())) { apic_timer_expired(apic); return; }