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

[Wanpeng Li <kernellwp@gmail.com>]

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