diff mbox

[6/5] KVM: x86: fix periodic lapic timer with hrtimers

Message ID 20161025114334.GD3197@potion (mailing list archive)
State New, archived
Headers show

Commit Message

Radim Krčmář Oct. 25, 2016, 11:43 a.m. UTC
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.


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.
--
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

Comments

Paolo Bonzini Oct. 25, 2016, 11:55 a.m. UTC | #1
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
Wanpeng Li Oct. 26, 2016, 6:02 a.m. UTC | #2
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
Wanpeng Li Oct. 26, 2016, 6:08 a.m. UTC | #3
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
Wanpeng Li Oct. 26, 2016, 10:23 a.m. UTC | #4
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
Paolo Bonzini Oct. 26, 2016, 11:15 a.m. UTC | #5
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
Wanpeng Li Oct. 26, 2016, 11:26 a.m. UTC | #6
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
Radim Krčmář Oct. 26, 2016, 1:32 p.m. UTC | #7
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
Radim Krčmář Oct. 26, 2016, 2:01 p.m. UTC | #8
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
Wanpeng Li Oct. 27, 2016, 2:11 a.m. UTC | #9
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
Wanpeng Li Oct. 27, 2016, 2:33 a.m. UTC | #10
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 mbox

Patch

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;
 	}