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+CwP+o0TKzhfftiMpUoesmQeAw1N=maXmVcjpFsUwLWPqg@mail.gmail.com>
Date:   Thu, 27 Oct 2016 10:11:26 +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-26 21:32 GMT+08:00 Radim Krčmář <rkrcmar@...hat.com>:
> 2016-10-26 14:02+0800, Wanpeng Li:
>> 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.
>
> 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