> On 12 Apr 2019, at 23:18, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
>
> Calling apic_timer_expired() is a nop when a timer interrupt is already
> pending, i.e. there's no need to call apic_timer_expired() when there's
> a pending interrupt and the hv_timer wants to pend its own interrupt.
> Separate the two flows to make the code more readable and to avoid an
> unnecessary function call and read to ktimer->pending.
In case timer is not periodic and r==1, atomic_read(&ktimer->pending) is not executed.
>
> Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> arch/x86/kvm/lapic.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 1d649a2af04c..f0be6f148a47 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1703,9 +1703,12 @@ static bool start_hv_timer(struct kvm_lapic *apic)
> * the window. For periodic timer, leave the hv timer running for
> * simplicity, and the deadline will be recomputed on the next vmexit.
> */
> - if (!apic_lvtt_period(apic) && (r || atomic_read(&ktimer->pending))) {
> - if (r)
> - apic_timer_expired(apic);
> + if (!apic_lvtt_period(apic) && atomic_read(&ktimer->pending))
> + return false;
> +
> + /* set_hv_timer() returns '1' when the timer has already expired. */
> + if (r) {
> + apic_timer_expired(apic);
> return false;
> }
>
> --
> 2.21.0
>
First, I think you should emphasise in commit message that you have actually fixed a rare bug here.
In case timer is periodic but given ktimer->tscdeadline has already expired on host, we should call apic_timer_expired().
In addition, when start_hv_timer() returns false, restart_apic_timer() just calls start_sw_timer() which use hrtimer instead of VMX preemption timer.
Therefore, it seems a bit ineffective to me for start_hv_timer() to return false in case ktimer->pending or when ktimer->tscdeadline already expired.
Shouldn’t we return true in these cases?
-Liran
