Re: [RFC PATCH V3 5/5] Utilize the vmx preemption timer for tsc deadline timer

[Paolo Bonzini <pbonzini@redhat.com>]

On 04/06/2016 02:42, Yunhong Jiang wrote:
> +void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
> +	WARN_ON(swait_active(&vcpu->wq));
> +	kvm_x86_ops->cancel_hv_timer(vcpu);
> +	apic->lapic_timer.hv_timer_in_use = 0;

Please use "false" here instead of 0.

> +	apic_timer_expired(apic);
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
> +

> 
> +		if (!kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
> +			apic->lapic_timer.hv_timer_in_use = true;
> +			hrtimer_cancel(&apic->lapic_timer.timer);
> +		}
> +
> +		/* In case the sw timer triggered in above small window */
> +		if (atomic_read(&apic->lapic_timer.pending) &&
> +				apic->lapic_timer.hv_timer_in_use)
> +			kvm_x86_ops->cancel_hv_timer(apic->vcpu);

Put this "if" after hrtimer_cancel, and you can avoid the "&&
apic->lapic_timer.hv_timer_in_use".  Also, I think it's clear to add an
extra

	apic->lapic_timer.hv_timer_in_use = false;

here, since you have it in kvm_lapic_expired_hv_timer.

> +void vmx_arm_hv_timer(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +	u64 tscl;
> +	u32 delta_tsc;
> +
> +	if (!apic->lapic_timer.hv_timer_in_use)

Can you change this to something like vcpu->arch.hv_deadline_tsc != -1
(by adjusting set_hv_timer and cancel_hv_timer)?

> +		return;
> +
> +	tscl = rdtsc();
> +	if (vcpu->arch.hv_deadline_tsc > tscl)
> +		/* sure to be 32 bit only because checked on set_hv_timer */

	/* vmx_set_hv_timer checks that the delta fits in 32 bits */

> +		delta_tsc = (u32)((vcpu->arch.hv_deadline_tsc - tscl) >>
> +			cpu_preemption_timer_multi);
> +	else
> +		delta_tsc = 0;
> +
> +	vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, delta_tsc);
> +}
> +

...

> +		u64 tscl = rdtsc();
>  		s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :
> -				rdtsc() - vcpu->arch.last_host_tsc;
> +				tscl - vcpu->arch.last_host_tsc;
>  		if (tsc_delta < 0)
>  			mark_tsc_unstable("KVM discovered backwards TSC");
> +
> +		/*
> +		 * If tsc backwrap, we need update the hv_deadline_tsc,
> +		 * otherwise, the ((hv_deadline_tsc-tsc) >>
> +		 * cpu_preemption_timer_multi) may >32bit on vcpu_run().
> +		 * This may cause deadline_tsc not so accurate, but that's
> +		 * inevitable anyway.
> +		 */
> +		if (tscl < vcpu->arch.hv_orig_tsc) {
> +			vcpu->arch.hv_orig_tsc = tscl;
> +			vcpu->arch.hv_deadline_tsc -=
> +				vcpu->arch.hv_orig_tsc - tscl;
> +		}

Can you instead call vmx_set_hv_timer again (and if it returns an error,
call kvm_lapic_switch_to_sw_timer)?  Perhaps it should even be done
unconditionally on vmx_vcpu_load.