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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANRm+Cw7UnrE_250uwRJpci+W_+z+u2o-tpYozD02SaK=HN5ng@mail.gmail.com>
Date:   Wed, 26 Oct 2016 14:02:36 +0800
From:   Wanpeng Li <kernellwp@...il.com>
To:     Radim Krčmář <rkrcmar@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kvm <kvm@...r.kernel.org>,
        Yunhong Jiang <yunhong.jiang@...el.com>,
        Wanpeng Li <wanpeng.li@...mail.com>
Subject: Re: [PATCH 6/5] KVM: x86: fix periodic lapic timer with hrtimers

2016-10-25 19:43 GMT+08:00 Radim Krčmář <rkrcmar@...hat.com>:
> 2016-10-25 07:39+0800, Wanpeng Li:
>> 2016-10-24 23:27 GMT+08:00 Radim Krčmář <rkrcmar@...hat.com>:
>>> 2016-10-24 17:09+0200, Paolo Bonzini:
>>>> On 24/10/2016 17:03, Radim Krčmář wrote:
>>> [...]
>>>>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@...hat.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