Re: [PATCH 6/7] KVM: lapic: Clean up the code for handling of a pre-expired hv_timer

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Sun, Apr 14, 2019 at 03:15:41PM +0300, Liran Alon wrote:
> 
> 
> > On 12 Apr 2019, at 23:18, Sean Christopherson <sean.j.christopherson@intel.com> 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@tencent.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> > 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().

Heh, I actually didn't even catch that bug, I was simply cleaning up the
code because I had a hard time following the logic.

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

That also seemed weird to me.  Again, I had a hell of a time following the
intended logic and didn't want to break anything.  AFAICT, the motivation
for calling start_sw_timer() is to cancel the HV timer, and possibly to
ensure start_sw_period() is called when necessary.  But the latter will be
handled by virtue of checking "r" after apic_lvtt_period(), so this?

	if (r) {
		apic_timer_expired(apic);
		ktimer->hv_timer_in_use = false;
		return true;
	}