linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
@ 2017-06-28  1:29 Wanpeng Li
  2017-06-28 12:10 ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-28  1:29 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: Paolo Bonzini, Radim Krčmář, Wanpeng Li

From: Wanpeng Li <wanpeng.li@hotmail.com>

If the TSC deadline timer is programmed really close to the deadline or 
even in the past, the computation in vmx_set_hv_timer will program the 
absolute target tsc value to vmcs preemption timer field w/ delta == 0, 
then plays a vmentry and an upcoming vmx preemption timer fire vmexit 
dance, the lapic timer injection is delayed due to this duration. Actually 
the lapic timer which is emulated by hrtimer can handle this correctly.

This patch fixes it by firing the lapic timer and injecting a timer interrupt 
immediately during the next vmentry if the TSC deadline timer is programmed
really close to the deadline or even in the past. This saves ~300 cycles on 
the tsc_deadline_timer test of apic.flat.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3:
 * update the patch description

 arch/x86/kvm/lapic.c | 7 ++++++-
 arch/x86/kvm/vmx.c   | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d24c874..08cf73d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1504,12 +1504,17 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
 static bool start_hv_timer(struct kvm_lapic *apic)
 {
 	u64 tscdeadline = apic->lapic_timer.tscdeadline;
+	int ret = 0;
 
 	if ((atomic_read(&apic->lapic_timer.pending) &&
 		!apic_lvtt_period(apic)) ||
-		kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
+		(ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
 		if (apic->lapic_timer.hv_timer_in_use)
 			cancel_hv_timer(apic);
+		if (ret == 1) {
+			apic_timer_expired(apic);
+			return true;
+		}
 	} else {
 		apic->lapic_timer.hv_timer_in_use = true;
 		hrtimer_cancel(&apic->lapic_timer.timer);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2e906cf..2008e9b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11149,6 +11149,9 @@ static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc)
 	u64 guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
 	u64 delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
 
+	if (delta_tsc == 0)
+		return 1;
+
 	/* Convert to host delta tsc if tsc scaling is enabled */
 	if (vcpu->arch.tsc_scaling_ratio != kvm_default_tsc_scaling_ratio &&
 			u64_shl_div_u64(delta_tsc,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28  1:29 [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay Wanpeng Li
@ 2017-06-28 12:10 ` Paolo Bonzini
  2017-06-28 13:55   ` Wanpeng Li
  2017-06-28 14:27   ` Wanpeng Li
  0 siblings, 2 replies; 15+ messages in thread
From: Paolo Bonzini @ 2017-06-28 12:10 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm; +Cc: Radim Krčmář, Wanpeng Li



On 28/06/2017 03:29, Wanpeng Li wrote:
>  	u64 tscdeadline = apic->lapic_timer.tscdeadline;
> +	int ret = 0;
>  
>  	if ((atomic_read(&apic->lapic_timer.pending) &&
>  		!apic_lvtt_period(apic)) ||
> -		kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
> +		(ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>  		if (apic->lapic_timer.hv_timer_in_use)
>  			cancel_hv_timer(apic);
> +		if (ret == 1) {
> +			apic_timer_expired(apic);
> +			return true;
> +		}

The preemption timer can also be used for modes other than TSC deadline.

In periodic mode, your patch would miss a call to
advance_periodic_target_expiration, which is only called by
kvm_lapic_expired_hv_timer.

You could use something like this:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d24c8742d9b0..15b751aa7625 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
 static bool start_hv_timer(struct kvm_lapic *apic)
 {
 	u64 tscdeadline = apic->lapic_timer.tscdeadline;
+	bool need_cancel = apic->lapic_timer.hv_timer_in_use;
+	if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
+		int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
+		if (r >= 0) {
+			need_cancel = false;
+			apic->lapic_timer.hv_timer_in_use = true;
+			hrtimer_cancel(&apic->lapic_timer.timer);
 
-	if ((atomic_read(&apic->lapic_timer.pending) &&
-		!apic_lvtt_period(apic)) ||
-		kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
-		if (apic->lapic_timer.hv_timer_in_use)
-			cancel_hv_timer(apic);
-	} else {
-		apic->lapic_timer.hv_timer_in_use = true;
-		hrtimer_cancel(&apic->lapic_timer.timer);
-
-		/* In case the sw timer triggered in the window */
-		if (atomic_read(&apic->lapic_timer.pending) &&
-			!apic_lvtt_period(apic))
-			cancel_hv_timer(apic);
+			/* In case the sw timer triggered in the window */
+			if (atomic_read(&apic->lapic_timer.pending) &&
+			    !apic_lvtt_period(apic))
+				need_cancel = true;
+			else if (r)
+				kvm_lapic_expired_hv_timer(vcpu);
+		}
 	}
+
+	if (need_cancel)
+		cancel_hv_timer(apic);
+
 	trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
 			apic->lapic_timer.hv_timer_in_use);
 	return apic->lapic_timer.hv_timer_in_use;

but I'm afraid of introducing a mutual recursion between
start_hv_timer and kvm_lapic_expired_hv_timer.

Paolo

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28 12:10 ` Paolo Bonzini
@ 2017-06-28 13:55   ` Wanpeng Li
  2017-06-28 14:00     ` Paolo Bonzini
  2017-06-28 14:27   ` Wanpeng Li
  1 sibling, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-28 13:55 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-28 20:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 28/06/2017 03:29, Wanpeng Li wrote:
>>       u64 tscdeadline = apic->lapic_timer.tscdeadline;
>> +     int ret = 0;
>>
>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>               !apic_lvtt_period(apic)) ||
>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>               if (apic->lapic_timer.hv_timer_in_use)
>>                       cancel_hv_timer(apic);
>> +             if (ret == 1) {
>> +                     apic_timer_expired(apic);
>> +                     return true;
>> +             }
>
> The preemption timer can also be used for modes other than TSC deadline.
>
> In periodic mode, your patch would miss a call to
> advance_periodic_target_expiration, which is only called by
> kvm_lapic_expired_hv_timer.

Actually I considered this before, however, I referred to apic timer
periodic mode which is emulated by hrtimer, there is no hrtimer start
for the next period in start_sw_period(). If it is also buggy?

Regards,
Wanpeng Li

>
> You could use something like this:
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index d24c8742d9b0..15b751aa7625 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>  static bool start_hv_timer(struct kvm_lapic *apic)
>  {
>         u64 tscdeadline = apic->lapic_timer.tscdeadline;
> +       bool need_cancel = apic->lapic_timer.hv_timer_in_use;
> +       if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
> +               int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
> +               if (r >= 0) {
> +                       need_cancel = false;
> +                       apic->lapic_timer.hv_timer_in_use = true;
> +                       hrtimer_cancel(&apic->lapic_timer.timer);
>
> -       if ((atomic_read(&apic->lapic_timer.pending) &&
> -               !apic_lvtt_period(apic)) ||
> -               kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
> -               if (apic->lapic_timer.hv_timer_in_use)
> -                       cancel_hv_timer(apic);
> -       } else {
> -               apic->lapic_timer.hv_timer_in_use = true;
> -               hrtimer_cancel(&apic->lapic_timer.timer);
> -
> -               /* In case the sw timer triggered in the window */
> -               if (atomic_read(&apic->lapic_timer.pending) &&
> -                       !apic_lvtt_period(apic))
> -                       cancel_hv_timer(apic);
> +                       /* In case the sw timer triggered in the window */
> +                       if (atomic_read(&apic->lapic_timer.pending) &&
> +                           !apic_lvtt_period(apic))
> +                               need_cancel = true;
> +                       else if (r)
> +                               kvm_lapic_expired_hv_timer(vcpu);
> +               }
>         }
> +
> +       if (need_cancel)
> +               cancel_hv_timer(apic);
> +
>         trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>                         apic->lapic_timer.hv_timer_in_use);
>         return apic->lapic_timer.hv_timer_in_use;
>
> but I'm afraid of introducing a mutual recursion between
> start_hv_timer and kvm_lapic_expired_hv_timer.
>
> Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28 13:55   ` Wanpeng Li
@ 2017-06-28 14:00     ` Paolo Bonzini
  2017-06-28 14:05       ` Wanpeng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2017-06-28 14:00 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li



On 28/06/2017 15:55, Wanpeng Li wrote:
>>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>>               !apic_lvtt_period(apic)) ||
>>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>>               if (apic->lapic_timer.hv_timer_in_use)
>>>                       cancel_hv_timer(apic);
>>> +             if (ret == 1) {
>>> +                     apic_timer_expired(apic);
>>> +                     return true;
>>> +             }
>> The preemption timer can also be used for modes other than TSC deadline.
>>
>> In periodic mode, your patch would miss a call to
>> advance_periodic_target_expiration, which is only called by
>> kvm_lapic_expired_hv_timer.
> Actually I considered this before, however, I referred to apic timer
> periodic mode which is emulated by hrtimer

Periodic mode can also be emulated by preemption timer... it was added
by some Wanpeng Li in commit 8003c9ae204e ("KVM: LAPIC: add APIC Timer
periodic/oneshot mode VMX preemption timer support", 2016-11-02), do you
know him? ;)

> , there is no hrtimer start
> for the next period in start_sw_period(). If it is also buggy?

start_sw_period always goes through the hrtimer for periodic timer:

        if (apic_lvtt_oneshot(apic) &&
            ktime_after(ktime_get(),
                        apic->lapic_timer.target_expiration)) {
                apic_timer_expired(apic);
                return;
        }

        hrtimer_start(&apic->lapic_timer.timer,
                apic->lapic_timer.target_expiration,
                HRTIMER_MODE_ABS_PINNED);

(the direct call to apic_timer_expired is conditonal to
apic_lvtt_oneshot).  This way, apic_timer_fn takes care of advancing the
hrtimer deadline and returning HRTIMER_RESTART.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28 14:00     ` Paolo Bonzini
@ 2017-06-28 14:05       ` Wanpeng Li
  0 siblings, 0 replies; 15+ messages in thread
From: Wanpeng Li @ 2017-06-28 14:05 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-28 22:00 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 28/06/2017 15:55, Wanpeng Li wrote:
>>>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>>>               !apic_lvtt_period(apic)) ||
>>>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>>>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>>>               if (apic->lapic_timer.hv_timer_in_use)
>>>>                       cancel_hv_timer(apic);
>>>> +             if (ret == 1) {
>>>> +                     apic_timer_expired(apic);
>>>> +                     return true;
>>>> +             }
>>> The preemption timer can also be used for modes other than TSC deadline.
>>>
>>> In periodic mode, your patch would miss a call to
>>> advance_periodic_target_expiration, which is only called by
>>> kvm_lapic_expired_hv_timer.
>> Actually I considered this before, however, I referred to apic timer
>> periodic mode which is emulated by hrtimer
>
> Periodic mode can also be emulated by preemption timer... it was added
> by some Wanpeng Li in commit 8003c9ae204e ("KVM: LAPIC: add APIC Timer
> periodic/oneshot mode VMX preemption timer support", 2016-11-02), do you
> know him? ;)

Indeed. :)

>
>> , there is no hrtimer start
>> for the next period in start_sw_period(). If it is also buggy?
>
> start_sw_period always goes through the hrtimer for periodic timer:
>
>         if (apic_lvtt_oneshot(apic) &&
>             ktime_after(ktime_get(),
>                         apic->lapic_timer.target_expiration)) {
>                 apic_timer_expired(apic);
>                 return;
>         }
>
>         hrtimer_start(&apic->lapic_timer.timer,
>                 apic->lapic_timer.target_expiration,
>                 HRTIMER_MODE_ABS_PINNED);
>
> (the direct call to apic_timer_expired is conditonal to
> apic_lvtt_oneshot).  This way, apic_timer_fn takes care of advancing the
> hrtimer deadline and returning HRTIMER_RESTART.

Ah, yes, I miss the apic_lvtt_oneshot() check here.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28 12:10 ` Paolo Bonzini
  2017-06-28 13:55   ` Wanpeng Li
@ 2017-06-28 14:27   ` Wanpeng Li
  2017-06-28 14:30     ` Paolo Bonzini
  1 sibling, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-28 14:27 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-28 20:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 28/06/2017 03:29, Wanpeng Li wrote:
>>       u64 tscdeadline = apic->lapic_timer.tscdeadline;
>> +     int ret = 0;
>>
>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>               !apic_lvtt_period(apic)) ||
>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>               if (apic->lapic_timer.hv_timer_in_use)
>>                       cancel_hv_timer(apic);
>> +             if (ret == 1) {
>> +                     apic_timer_expired(apic);
>> +                     return true;
>> +             }
>
> The preemption timer can also be used for modes other than TSC deadline.
>
> In periodic mode, your patch would miss a call to
> advance_periodic_target_expiration, which is only called by
> kvm_lapic_expired_hv_timer.
>
> You could use something like this:
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index d24c8742d9b0..15b751aa7625 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>  static bool start_hv_timer(struct kvm_lapic *apic)
>  {
>         u64 tscdeadline = apic->lapic_timer.tscdeadline;
> +       bool need_cancel = apic->lapic_timer.hv_timer_in_use;
> +       if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
> +               int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
> +               if (r >= 0) {
> +                       need_cancel = false;
> +                       apic->lapic_timer.hv_timer_in_use = true;
> +                       hrtimer_cancel(&apic->lapic_timer.timer);
>
> -       if ((atomic_read(&apic->lapic_timer.pending) &&
> -               !apic_lvtt_period(apic)) ||
> -               kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
> -               if (apic->lapic_timer.hv_timer_in_use)
> -                       cancel_hv_timer(apic);
> -       } else {
> -               apic->lapic_timer.hv_timer_in_use = true;
> -               hrtimer_cancel(&apic->lapic_timer.timer);
> -
> -               /* In case the sw timer triggered in the window */
> -               if (atomic_read(&apic->lapic_timer.pending) &&
> -                       !apic_lvtt_period(apic))
> -                       cancel_hv_timer(apic);
> +                       /* In case the sw timer triggered in the window */
> +                       if (atomic_read(&apic->lapic_timer.pending) &&
> +                           !apic_lvtt_period(apic))
> +                               need_cancel = true;
> +                       else if (r)
> +                               kvm_lapic_expired_hv_timer(vcpu);
> +               }
>         }
> +
> +       if (need_cancel)
> +               cancel_hv_timer(apic);
> +
>         trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>                         apic->lapic_timer.hv_timer_in_use);
>         return apic->lapic_timer.hv_timer_in_use;
>
> but I'm afraid of introducing a mutual recursion between
> start_hv_timer and kvm_lapic_expired_hv_timer.

We can just handle the apic timer oneshot/tscdeadline mode instead of
periodic mode just like which is emulated by hrtimer to avoid the
mutual recusion, what do you think?

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28 14:27   ` Wanpeng Li
@ 2017-06-28 14:30     ` Paolo Bonzini
  2017-06-29  3:42       ` Wanpeng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2017-06-28 14:30 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li



On 28/06/2017 16:27, Wanpeng Li wrote:
> 2017-06-28 20:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 28/06/2017 03:29, Wanpeng Li wrote:
>>>       u64 tscdeadline = apic->lapic_timer.tscdeadline;
>>> +     int ret = 0;
>>>
>>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>>               !apic_lvtt_period(apic)) ||
>>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>>               if (apic->lapic_timer.hv_timer_in_use)
>>>                       cancel_hv_timer(apic);
>>> +             if (ret == 1) {
>>> +                     apic_timer_expired(apic);
>>> +                     return true;
>>> +             }
>>
>> The preemption timer can also be used for modes other than TSC deadline.
>>
>> In periodic mode, your patch would miss a call to
>> advance_periodic_target_expiration, which is only called by
>> kvm_lapic_expired_hv_timer.
>>
>> You could use something like this:
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d24c8742d9b0..15b751aa7625 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>>  static bool start_hv_timer(struct kvm_lapic *apic)
>>  {
>>         u64 tscdeadline = apic->lapic_timer.tscdeadline;
>> +       bool need_cancel = apic->lapic_timer.hv_timer_in_use;
>> +       if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
>> +               int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
>> +               if (r >= 0) {
>> +                       need_cancel = false;
>> +                       apic->lapic_timer.hv_timer_in_use = true;
>> +                       hrtimer_cancel(&apic->lapic_timer.timer);
>>
>> -       if ((atomic_read(&apic->lapic_timer.pending) &&
>> -               !apic_lvtt_period(apic)) ||
>> -               kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>> -               if (apic->lapic_timer.hv_timer_in_use)
>> -                       cancel_hv_timer(apic);
>> -       } else {
>> -               apic->lapic_timer.hv_timer_in_use = true;
>> -               hrtimer_cancel(&apic->lapic_timer.timer);
>> -
>> -               /* In case the sw timer triggered in the window */
>> -               if (atomic_read(&apic->lapic_timer.pending) &&
>> -                       !apic_lvtt_period(apic))
>> -                       cancel_hv_timer(apic);
>> +                       /* In case the sw timer triggered in the window */
>> +                       if (atomic_read(&apic->lapic_timer.pending) &&
>> +                           !apic_lvtt_period(apic))
>> +                               need_cancel = true;
>> +                       else if (r)
>> +                               kvm_lapic_expired_hv_timer(vcpu);
>> +               }
>>         }
>> +
>> +       if (need_cancel)
>> +               cancel_hv_timer(apic);
>> +
>>         trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>>                         apic->lapic_timer.hv_timer_in_use);
>>         return apic->lapic_timer.hv_timer_in_use;
>>
>> but I'm afraid of introducing a mutual recursion between
>> start_hv_timer and kvm_lapic_expired_hv_timer.
> 
> We can just handle the apic timer oneshot/tscdeadline mode instead of
> periodic mode just like which is emulated by hrtimer to avoid the
> mutual recusion, what do you think?

In that case, set_hv_timer should probably always enable the preemption
timer.  You can then cancel it if it returns 1 _and_ the APIC timer's
mode is oneshot/tscdeadline.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-28 14:30     ` Paolo Bonzini
@ 2017-06-29  3:42       ` Wanpeng Li
  2017-06-29  7:55         ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-29  3:42 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-28 22:30 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 28/06/2017 16:27, Wanpeng Li wrote:
>> 2017-06-28 20:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>
>>>
>>> On 28/06/2017 03:29, Wanpeng Li wrote:
>>>>       u64 tscdeadline = apic->lapic_timer.tscdeadline;
>>>> +     int ret = 0;
>>>>
>>>>       if ((atomic_read(&apic->lapic_timer.pending) &&
>>>>               !apic_lvtt_period(apic)) ||
>>>> -             kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>>>> +             (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>>>               if (apic->lapic_timer.hv_timer_in_use)
>>>>                       cancel_hv_timer(apic);
>>>> +             if (ret == 1) {
>>>> +                     apic_timer_expired(apic);
>>>> +                     return true;
>>>> +             }
>>>
>>> The preemption timer can also be used for modes other than TSC deadline.
>>>
>>> In periodic mode, your patch would miss a call to
>>> advance_periodic_target_expiration, which is only called by
>>> kvm_lapic_expired_hv_timer.
>>>
>>> You could use something like this:
>>>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index d24c8742d9b0..15b751aa7625 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>>>  static bool start_hv_timer(struct kvm_lapic *apic)
>>>  {
>>>         u64 tscdeadline = apic->lapic_timer.tscdeadline;
>>> +       bool need_cancel = apic->lapic_timer.hv_timer_in_use;
>>> +       if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
>>> +               int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
>>> +               if (r >= 0) {
>>> +                       need_cancel = false;
>>> +                       apic->lapic_timer.hv_timer_in_use = true;
>>> +                       hrtimer_cancel(&apic->lapic_timer.timer);
>>>
>>> -       if ((atomic_read(&apic->lapic_timer.pending) &&
>>> -               !apic_lvtt_period(apic)) ||
>>> -               kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>>> -               if (apic->lapic_timer.hv_timer_in_use)
>>> -                       cancel_hv_timer(apic);
>>> -       } else {
>>> -               apic->lapic_timer.hv_timer_in_use = true;
>>> -               hrtimer_cancel(&apic->lapic_timer.timer);
>>> -
>>> -               /* In case the sw timer triggered in the window */
>>> -               if (atomic_read(&apic->lapic_timer.pending) &&
>>> -                       !apic_lvtt_period(apic))
>>> -                       cancel_hv_timer(apic);
>>> +                       /* In case the sw timer triggered in the window */
>>> +                       if (atomic_read(&apic->lapic_timer.pending) &&
>>> +                           !apic_lvtt_period(apic))
>>> +                               need_cancel = true;
>>> +                       else if (r)
>>> +                               kvm_lapic_expired_hv_timer(vcpu);
>>> +               }
>>>         }
>>> +
>>> +       if (need_cancel)
>>> +               cancel_hv_timer(apic);
>>> +
>>>         trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>>>                         apic->lapic_timer.hv_timer_in_use);
>>>         return apic->lapic_timer.hv_timer_in_use;
>>>
>>> but I'm afraid of introducing a mutual recursion between
>>> start_hv_timer and kvm_lapic_expired_hv_timer.
>>
>> We can just handle the apic timer oneshot/tscdeadline mode instead of
>> periodic mode just like which is emulated by hrtimer to avoid the
>> mutual recusion, what do you think?
>
> In that case, set_hv_timer should probably always enable the preemption
> timer.  You can then cancel it if it returns 1 _and_ the APIC timer's
> mode is oneshot/tscdeadline.
>
> Paolo

So how about something like this?

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d24c874..900ce86 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
 static bool start_hv_timer(struct kvm_lapic *apic)
 {
     u64 tscdeadline = apic->lapic_timer.tscdeadline;
+    bool need_cancel = apic->lapic_timer.hv_timer_in_use;
+    if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
+        int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
+        if (r >= 0) {
+            need_cancel = false;
+            apic->lapic_timer.hv_timer_in_use = true;
+            hrtimer_cancel(&apic->lapic_timer.timer);
+
+            /* In case the sw timer triggered in the window */
+            if (atomic_read(&apic->lapic_timer.pending) &&
+                !apic_lvtt_period(apic))
+                need_cancel = true;
+            else if (r && (apic_lvtt_oneshot(apic) ||
apic_lvtt_tscdeadline(apic)))
+                apic_timer_expired(apic);
+        }
+    }

-    if ((atomic_read(&apic->lapic_timer.pending) &&
-        !apic_lvtt_period(apic)) ||
-        kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
-        if (apic->lapic_timer.hv_timer_in_use)
-            cancel_hv_timer(apic);
-    } else {
-        apic->lapic_timer.hv_timer_in_use = true;
-        hrtimer_cancel(&apic->lapic_timer.timer);
+    if (need_cancel)
+        cancel_hv_timer(apic);

-        /* In case the sw timer triggered in the window */
-        if (atomic_read(&apic->lapic_timer.pending) &&
-            !apic_lvtt_period(apic))
-            cancel_hv_timer(apic);
-    }
     trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
             apic->lapic_timer.hv_timer_in_use);
     return apic->lapic_timer.hv_timer_in_use;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4f616db..0a0e696 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11125,6 +11125,9 @@ static int vmx_set_hv_timer(struct kvm_vcpu
*vcpu, u64 guest_deadline_tsc)
     u64 guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
     u64 delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;

+    if (delta_tsc == 0)
+        return 1;
+
     /* Convert to host delta tsc if tsc scaling is enabled */
     if (vcpu->arch.tsc_scaling_ratio != kvm_default_tsc_scaling_ratio &&
             u64_shl_div_u64(delta_tsc,

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29  3:42       ` Wanpeng Li
@ 2017-06-29  7:55         ` Paolo Bonzini
  2017-06-29  8:17           ` Wanpeng Li
  2017-06-29 12:05           ` Wanpeng Li
  0 siblings, 2 replies; 15+ messages in thread
From: Paolo Bonzini @ 2017-06-29  7:55 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li



On 29/06/2017 05:42, Wanpeng Li wrote:
> So how about something like this?
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index d24c874..900ce86 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>  static bool start_hv_timer(struct kvm_lapic *apic)
>  {
>      u64 tscdeadline = apic->lapic_timer.tscdeadline;
> +    bool need_cancel = apic->lapic_timer.hv_timer_in_use;
> +    if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
> +        int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
> +        if (r >= 0) {
> +            need_cancel = false;
> +            apic->lapic_timer.hv_timer_in_use = true;
> +            hrtimer_cancel(&apic->lapic_timer.timer);
> +
> +            /* In case the sw timer triggered in the window */
> +            if (atomic_read(&apic->lapic_timer.pending) &&
> +                !apic_lvtt_period(apic))
> +                need_cancel = true;
> +            else if (r && (apic_lvtt_oneshot(apic) ||
> apic_lvtt_tscdeadline(apic)))
> +                apic_timer_expired(apic);
> +        }
> +    }
> 
> -    if ((atomic_read(&apic->lapic_timer.pending) &&
> -        !apic_lvtt_period(apic)) ||
> -        kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
> -        if (apic->lapic_timer.hv_timer_in_use)
> -            cancel_hv_timer(apic);
> -    } else {
> -        apic->lapic_timer.hv_timer_in_use = true;
> -        hrtimer_cancel(&apic->lapic_timer.timer);
> +    if (need_cancel)
> +        cancel_hv_timer(apic);
> 
> -        /* In case the sw timer triggered in the window */
> -        if (atomic_read(&apic->lapic_timer.pending) &&
> -            !apic_lvtt_period(apic))
> -            cancel_hv_timer(apic);
> -    }
>      trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>              apic->lapic_timer.hv_timer_in_use);
>      return apic->lapic_timer.hv_timer_in_use;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4f616db..0a0e696 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11125,6 +11125,9 @@ static int vmx_set_hv_timer(struct kvm_vcpu
> *vcpu, u64 guest_deadline_tsc)
>      u64 guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
>      u64 delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
> 
> +    if (delta_tsc == 0)
> +        return 1;

You still need to enable the preemption timer even if you return 1, so
in lapic.c it becomes

	if (!apic_lvtt_period(apic)) {
		if (r)
			apic_timer_expired(apic);
		if (atomic_read(&apic->lapic_timer.pending))
			need_cancel = true;
	}


Otherwise it looks good.  Thanks for your persistence. :)

Paolo

>      /* Convert to host delta tsc if tsc scaling is enabled */
>      if (vcpu->arch.tsc_scaling_ratio != kvm_default_tsc_scaling_ratio &&
>              u64_shl_div_u64(delta_tsc,
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29  7:55         ` Paolo Bonzini
@ 2017-06-29  8:17           ` Wanpeng Li
  2017-06-29  8:44             ` Wanpeng Li
  2017-06-29 12:05           ` Wanpeng Li
  1 sibling, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-29  8:17 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-29 15:55 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> +
>> +            /* In case the sw timer triggered in the window */
>> +            if (atomic_read(&apic->lapic_timer.pending) &&
>> +                !apic_lvtt_period(apic))
>> +                need_cancel = true;
>> +            else if (r && (apic_lvtt_oneshot(apic) ||
>> apic_lvtt_tscdeadline(apic)))
>> +                apic_timer_expired(apic);
>> +        }
>> +    }

[...]

>
> You still need to enable the preemption timer even if you return 1, so
> in lapic.c it becomes
>
>         if (!apic_lvtt_period(apic)) {
>                 if (r)
>                         apic_timer_expired(apic);
>                 if (atomic_read(&apic->lapic_timer.pending))
>                         need_cancel = true;
>         }

I think the codes are more clear but the same as above. We didn't
program preemption timer vmcs field if delta == 0, so how to
understand "need to enable the preemption timer even if return 1"?

>
>
> Otherwise it looks good.  Thanks for your persistence. :)

Thanks for your help and patient. :)

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29  8:17           ` Wanpeng Li
@ 2017-06-29  8:44             ` Wanpeng Li
  2017-06-29 11:43               ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-29  8:44 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-29 16:17 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2017-06-29 15:55 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>> +
>>> +            /* In case the sw timer triggered in the window */
>>> +            if (atomic_read(&apic->lapic_timer.pending) &&
>>> +                !apic_lvtt_period(apic))
>>> +                need_cancel = true;
>>> +            else if (r && (apic_lvtt_oneshot(apic) ||
>>> apic_lvtt_tscdeadline(apic)))
>>> +                apic_timer_expired(apic);
>>> +        }
>>> +    }
>
> [...]
>
>>
>> You still need to enable the preemption timer even if you return 1, so
>> in lapic.c it becomes
>>
>>         if (!apic_lvtt_period(apic)) {
>>                 if (r)
>>                         apic_timer_expired(apic);
>>                 if (atomic_read(&apic->lapic_timer.pending))
>>                         need_cancel = true;
>>         }
>
> I think the codes are more clear but the same as above. We didn't
> program preemption timer vmcs field if delta == 0, so how to
> understand "need to enable the preemption timer even if return 1"?

I guess you mean start_hv_timer() should return true, right?

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29  8:44             ` Wanpeng Li
@ 2017-06-29 11:43               ` Paolo Bonzini
  2017-06-29 11:54                 ` Wanpeng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2017-06-29 11:43 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li



On 29/06/2017 10:44, Wanpeng Li wrote:
>>> You still need to enable the preemption timer even if you return 1, so
>>> in lapic.c it becomes
>>>
>>>         if (!apic_lvtt_period(apic)) {
>>>                 if (r)
>>>                         apic_timer_expired(apic);
>>>                 if (atomic_read(&apic->lapic_timer.pending))
>>>                         need_cancel = true;
>>>         }
>> I think the codes are more clear but the same as above. We didn't
>> program preemption timer vmcs field if delta == 0, so how to
>> understand "need to enable the preemption timer even if return 1"?
> I guess you mean start_hv_timer() should return true, right?

vmx.c's set_hv_timer callback should set the preemption timer execution
control.  Otherwise, kvm_lapic_hv_timer_expired is again not called.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29 11:43               ` Paolo Bonzini
@ 2017-06-29 11:54                 ` Wanpeng Li
  0 siblings, 0 replies; 15+ messages in thread
From: Wanpeng Li @ 2017-06-29 11:54 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-29 19:43 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 29/06/2017 10:44, Wanpeng Li wrote:
>>>> You still need to enable the preemption timer even if you return 1, so
>>>> in lapic.c it becomes
>>>>
>>>>         if (!apic_lvtt_period(apic)) {
>>>>                 if (r)
>>>>                         apic_timer_expired(apic);
>>>>                 if (atomic_read(&apic->lapic_timer.pending))
>>>>                         need_cancel = true;
>>>>         }
>>> I think the codes are more clear but the same as above. We didn't
>>> program preemption timer vmcs field if delta == 0, so how to
>>> understand "need to enable the preemption timer even if return 1"?
>> I guess you mean start_hv_timer() should return true, right?
>
> vmx.c's set_hv_timer callback should set the preemption timer execution
> control.  Otherwise, kvm_lapic_hv_timer_expired is again not called.

I see, this should be set for apic timer periodic mode.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29  7:55         ` Paolo Bonzini
  2017-06-29  8:17           ` Wanpeng Li
@ 2017-06-29 12:05           ` Wanpeng Li
  2017-06-29 12:13             ` Paolo Bonzini
  1 sibling, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2017-06-29 12:05 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li

2017-06-29 15:55 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 29/06/2017 05:42, Wanpeng Li wrote:
>> So how about something like this?
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d24c874..900ce86 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>>  static bool start_hv_timer(struct kvm_lapic *apic)
>>  {
>>      u64 tscdeadline = apic->lapic_timer.tscdeadline;
>> +    bool need_cancel = apic->lapic_timer.hv_timer_in_use;
>> +    if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
>> +        int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
>> +        if (r >= 0) {
>> +            need_cancel = false;
>> +            apic->lapic_timer.hv_timer_in_use = true;
>> +            hrtimer_cancel(&apic->lapic_timer.timer);
>> +
>> +            /* In case the sw timer triggered in the window */
>> +            if (atomic_read(&apic->lapic_timer.pending) &&
>> +                !apic_lvtt_period(apic))
>> +                need_cancel = true;
>> +            else if (r && (apic_lvtt_oneshot(apic) ||
>> apic_lvtt_tscdeadline(apic)))
>> +                apic_timer_expired(apic);
>> +        }
>> +    }
>>
>> -    if ((atomic_read(&apic->lapic_timer.pending) &&
>> -        !apic_lvtt_period(apic)) ||
>> -        kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>> -        if (apic->lapic_timer.hv_timer_in_use)
>> -            cancel_hv_timer(apic);
>> -    } else {
>> -        apic->lapic_timer.hv_timer_in_use = true;
>> -        hrtimer_cancel(&apic->lapic_timer.timer);
>> +    if (need_cancel)
>> +        cancel_hv_timer(apic);
>>
>> -        /* In case the sw timer triggered in the window */
>> -        if (atomic_read(&apic->lapic_timer.pending) &&
>> -            !apic_lvtt_period(apic))
>> -            cancel_hv_timer(apic);
>> -    }
>>      trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>>              apic->lapic_timer.hv_timer_in_use);
>>      return apic->lapic_timer.hv_timer_in_use;
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4f616db..0a0e696 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -11125,6 +11125,9 @@ static int vmx_set_hv_timer(struct kvm_vcpu
>> *vcpu, u64 guest_deadline_tsc)
>>      u64 guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
>>      u64 delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
>>
>> +    if (delta_tsc == 0)
>> +        return 1;
>
> You still need to enable the preemption timer even if you return 1, so
> in lapic.c it becomes
>
>         if (!apic_lvtt_period(apic)) {
>                 if (r)
>                         apic_timer_expired(apic);
>                 if (atomic_read(&apic->lapic_timer.pending))
>                         need_cancel = true;
>         }

As you point out, we should cancel the preemption timer if it returns
1 and the APIC timer's mode is oneshot/tscdeadline. How about
something like this:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d24c874..b801385 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1504,21 +1504,28 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
 static bool start_hv_timer(struct kvm_lapic *apic)
 {
     u64 tscdeadline = apic->lapic_timer.tscdeadline;
+    bool need_cancel = apic->lapic_timer.hv_timer_in_use;
+    if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
+        int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
+        if (r >= 0) {
+            need_cancel = false;
+            apic->lapic_timer.hv_timer_in_use = true;
+            hrtimer_cancel(&apic->lapic_timer.timer);
+
+            /* In case the sw timer triggered in the window */
+            if (!apic_lvtt_period(apic)) {
+                if (r || atomic_read(&apic->lapic_timer.pending)) {
+                    need_cancel = true;
+                    if (r)
+                    apic_timer_expired(apic);
+                }
+            }
+        }
+    }

-    if ((atomic_read(&apic->lapic_timer.pending) &&
-        !apic_lvtt_period(apic)) ||
-        kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
-        if (apic->lapic_timer.hv_timer_in_use)
-            cancel_hv_timer(apic);
-    } else {
-        apic->lapic_timer.hv_timer_in_use = true;
-        hrtimer_cancel(&apic->lapic_timer.timer);
+    if (need_cancel)
+        cancel_hv_timer(apic);

-        /* In case the sw timer triggered in the window */
-        if (atomic_read(&apic->lapic_timer.pending) &&
-            !apic_lvtt_period(apic))
-            cancel_hv_timer(apic);
-    }
     trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
             apic->lapic_timer.hv_timer_in_use);
     return apic->lapic_timer.hv_timer_in_use;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ca5d2b9..b8bde13 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11134,7 +11134,11 @@ static int vmx_set_hv_timer(struct kvm_vcpu
*vcpu, u64 guest_deadline_tsc)
     vmx->hv_deadline_tsc = tscl + delta_tsc;
     vmcs_set_bits(PIN_BASED_VM_EXEC_CONTROL,
             PIN_BASED_VMX_PREEMPTION_TIMER);
-    return 0;
+
+    if (delta_tsc == 0)
+        return 1;
+    else
+        return 0;
 }

 static void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu)

Regards,
Wanpeng Li

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay
  2017-06-29 12:05           ` Wanpeng Li
@ 2017-06-29 12:13             ` Paolo Bonzini
  0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2017-06-29 12:13 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: linux-kernel, kvm, Radim Krčmář, Wanpeng Li



On 29/06/2017 14:05, Wanpeng Li wrote:
> +            /* In case the sw timer triggered in the window */
> +            if (!apic_lvtt_period(apic)) {
> +                if (r || atomic_read(&apic->lapic_timer.pending)) {
> +                    need_cancel = true;
> +                    if (r)
> +                    apic_timer_expired(apic);
> +                }
> +            }

Yes, that's equivalent.  The compiler should thread the jumps as if it were:

	if (r) {
		apic_timer_expired(apic);
		goto cancel_timer;
	}
	if (atomic_read(&apic->lapic_timer.pending))
		goto cancel_timer;

so it produces pretty good code too.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-06-29 12:14 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-28  1:29 [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay Wanpeng Li
2017-06-28 12:10 ` Paolo Bonzini
2017-06-28 13:55   ` Wanpeng Li
2017-06-28 14:00     ` Paolo Bonzini
2017-06-28 14:05       ` Wanpeng Li
2017-06-28 14:27   ` Wanpeng Li
2017-06-28 14:30     ` Paolo Bonzini
2017-06-29  3:42       ` Wanpeng Li
2017-06-29  7:55         ` Paolo Bonzini
2017-06-29  8:17           ` Wanpeng Li
2017-06-29  8:44             ` Wanpeng Li
2017-06-29 11:43               ` Paolo Bonzini
2017-06-29 11:54                 ` Wanpeng Li
2017-06-29 12:05           ` Wanpeng Li
2017-06-29 12:13             ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).