Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370895 > unrolled thread

[PATCH] kvm: x86: make lapic hrtimer pinned

Back to article view | Back to linux.kernel

Contents

  [PATCH] kvm: x86: make lapic hrtimer pinned Luiz Capitulino <lcapitulino@redhat.com> - 2016-04-04 22:50 +0200
    Re: [PATCH] kvm: x86: make lapic hrtimer pinned Rik van Riel <riel@redhat.com> - 2016-04-04 23:10 +0200
      Re: [PATCH] kvm: x86: make lapic hrtimer pinned Yang Zhang <yang.zhang.wz@gmail.com> - 2016-04-05 08:20 +0200
        Re: [PATCH] kvm: x86: make lapic hrtimer pinned Luiz Capitulino <lcapitulino@redhat.com> - 2016-04-05 14:50 +0200
        Re: [PATCH] kvm: x86: make lapic hrtimer pinned Radim Krčmář <rkrcmar@redhat.com> - 2016-04-05 18:00 +0200
          Re: [PATCH] kvm: x86: make lapic hrtimer pinned Yang Zhang <yang.zhang.wz@gmail.com> - 2016-04-07 04:10 +0200
    Re: [PATCH] kvm: x86: make lapic hrtimer pinned Paolo Bonzini <pbonzini@redhat.com> - 2016-04-05 12:10 +0200

#1370895 — [PATCH] kvm: x86: make lapic hrtimer pinned

When a vCPU runs on a nohz_full core, the hrtimer used by
the lapic emulation code can be migrated to another core.
When this happens, it's possible to observe milisecond
latency when delivering timer IRQs to KVM guests.

The huge latency is mainly due to the fact that
apic_timer_fn() expects to run during a kvm exit. It
sets KVM_REQ_PENDING_TIMER and let it be handled on kvm
entry. However, if the timer fires on a different core,
we have to wait until the next kvm exit for the guest
to see KVM_REQ_PENDING_TIMER set.

This problem became visible after commit 9642d18ee. This
commit changed the timer migration code to always attempt
to migrate timers away from nohz_full cores. While it's
discussable if this is correct/desirable (I don't think
it is), it's clear that the lapic emulation code has
a requirement on firing the hrtimer in the same core
where it was started. This is achieved by making the
hrtimer pinned.

Lastly, note that KVM has code to migrate timers when a
vCPU is scheduled to run in different core. However, this
forced migration may fail. When this happens, we can have
the same problem. If we want 100% correctness, we'll have
to modify apic_timer_fn() to cause a kvm exit when it runs
on a different core than the vCPU. Not sure if this is
possible.

Here's a reproducer for the issue being fixed:

 1. Set all cores but core0 to be nohz_full cores
 2. Start a guest with a single vCPU
 3. Trace apic_timer_fn() and kvm_inject_apic_timer_irqs()

You'll see that apic_timer_fn() will run in core0 while
kvm_inject_apic_timer_irqs() runs in a different core. If
you get both on core0, try running a program that takes 100%
of the CPU and pin it to core0 to force the vCPU out.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 arch/x86/kvm/lapic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 443d2a5..1a2da0e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1369,7 +1369,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 
 		hrtimer_start(&apic->lapic_timer.timer,
 			      ktime_add_ns(now, apic->lapic_timer.period),
-			      HRTIMER_MODE_ABS);
+			      HRTIMER_MODE_ABS_PINNED);
 
 		apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
 			   PRIx64 ", "
@@ -1402,7 +1402,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 			expire = ktime_add_ns(now, ns);
 			expire = ktime_sub_ns(expire, lapic_timer_advance_ns);
 			hrtimer_start(&apic->lapic_timer.timer,
-				      expire, HRTIMER_MODE_ABS);
+				      expire, HRTIMER_MODE_ABS_PINNED);
 		} else
 			apic_timer_expired(apic);
 
@@ -1868,7 +1868,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 	apic->vcpu = vcpu;
 
 	hrtimer_init(&apic->lapic_timer.timer, CLOCK_MONOTONIC,
-		     HRTIMER_MODE_ABS);
+		     HRTIMER_MODE_ABS_PINNED);
 	apic->lapic_timer.timer.function = apic_timer_fn;
 
 	/*
@@ -2003,7 +2003,7 @@ void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
 
 	timer = &vcpu->arch.apic->lapic_timer.timer;
 	if (hrtimer_cancel(timer))
-		hrtimer_start_expires(timer, HRTIMER_MODE_ABS);
+		hrtimer_start_expires(timer, HRTIMER_MODE_ABS_PINNED);
 }
 
 /*
-- 
2.1.0

[toc] | [next] | [standalone]

#1370898

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2016-04-04 at 16:46 -0400, Luiz Capitulino wrote:
> When a vCPU runs on a nohz_full core, the hrtimer used by
> the lapic emulation code can be migrated to another core.
> When this happens, it's possible to observe milisecond
> latency when delivering timer IRQs to KVM guests.
> 
> The huge latency is mainly due to the fact that
> apic_timer_fn() expects to run during a kvm exit. It
> sets KVM_REQ_PENDING_TIMER and let it be handled on kvm
> entry. However, if the timer fires on a different core,
> we have to wait until the next kvm exit for the guest
> to see KVM_REQ_PENDING_TIMER set.
> 
> This problem became visible after commit 9642d18ee. This
> commit changed the timer migration code to always attempt
> to migrate timers away from nohz_full cores. While it's
> discussable if this is correct/desirable (I don't think
> it is), it's clear that the lapic emulation code has
> a requirement on firing the hrtimer in the same core
> where it was started. This is achieved by making the
> hrtimer pinned.

Given that delivering a timer to a guest seems to
involve trapping from the guest to the host, anyway,
I don't see a downside to your patch.

If that is ever changed (eg. allowing delivery of
a timer interrupt to a VCPU without trapping to the
host), we may want to revisit this.

Until then...

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All Rights Reversed.

[toc] | [prev] | [next] | [standalone]

#1371257

On 2016/4/5 5:00, Rik van Riel wrote:
> On Mon, 2016-04-04 at 16:46 -0400, Luiz Capitulino wrote:
>> When a vCPU runs on a nohz_full core, the hrtimer used by
>> the lapic emulation code can be migrated to another core.
>> When this happens, it's possible to observe milisecond
>> latency when delivering timer IRQs to KVM guests.
>>
>> The huge latency is mainly due to the fact that
>> apic_timer_fn() expects to run during a kvm exit. It
>> sets KVM_REQ_PENDING_TIMER and let it be handled on kvm
>> entry. However, if the timer fires on a different core,
>> we have to wait until the next kvm exit for the guest
>> to see KVM_REQ_PENDING_TIMER set.
>>
>> This problem became visible after commit 9642d18ee. This
>> commit changed the timer migration code to always attempt
>> to migrate timers away from nohz_full cores. While it's
>> discussable if this is correct/desirable (I don't think
>> it is), it's clear that the lapic emulation code has
>> a requirement on firing the hrtimer in the same core
>> where it was started. This is achieved by making the
>> hrtimer pinned.
>
> Given that delivering a timer to a guest seems to
> involve trapping from the guest to the host, anyway,
> I don't see a downside to your patch.
>
> If that is ever changed (eg. allowing delivery of
> a timer interrupt to a VCPU without trapping to the
> host), we may want to revisit this.


Posted interrupt helps in this case. Currently, KVM doesn't use PI for 
lapic timer is due to same affinity for lapic timer and VCPU. Now, we 
can change to use PI for lapic timer. The only concern is what's 
frequency of timer migration in upstream Linux? If it is frequently, 
will it bring additional cost?

BTW, in what case the migration of timers during VCPU scheduling will fail?

-- 
best regards
yang

[toc] | [prev] | [next] | [standalone]

#1371520

On Tue, 5 Apr 2016 14:18:01 +0800
Yang Zhang <yang.zhang.wz@gmail.com> wrote:

> On 2016/4/5 5:00, Rik van Riel wrote:
> > On Mon, 2016-04-04 at 16:46 -0400, Luiz Capitulino wrote:
> >> When a vCPU runs on a nohz_full core, the hrtimer used by
> >> the lapic emulation code can be migrated to another core.
> >> When this happens, it's possible to observe milisecond
> >> latency when delivering timer IRQs to KVM guests.
> >>
> >> The huge latency is mainly due to the fact that
> >> apic_timer_fn() expects to run during a kvm exit. It
> >> sets KVM_REQ_PENDING_TIMER and let it be handled on kvm
> >> entry. However, if the timer fires on a different core,
> >> we have to wait until the next kvm exit for the guest
> >> to see KVM_REQ_PENDING_TIMER set.
> >>
> >> This problem became visible after commit 9642d18ee. This
> >> commit changed the timer migration code to always attempt
> >> to migrate timers away from nohz_full cores. While it's
> >> discussable if this is correct/desirable (I don't think
> >> it is), it's clear that the lapic emulation code has
> >> a requirement on firing the hrtimer in the same core
> >> where it was started. This is achieved by making the
> >> hrtimer pinned.
> >
> > Given that delivering a timer to a guest seems to
> > involve trapping from the guest to the host, anyway,
> > I don't see a downside to your patch.
> >
> > If that is ever changed (eg. allowing delivery of
> > a timer interrupt to a VCPU without trapping to the
> > host), we may want to revisit this.
> 
> 
> Posted interrupt helps in this case. Currently, KVM doesn't use PI for 
> lapic timer is due to same affinity for lapic timer and VCPU. Now, we 
> can change to use PI for lapic timer. The only concern is what's 
> frequency of timer migration in upstream Linux? If it is frequently, 
> will it bring additional cost?

I can't answer this questions.

> BTW, in what case the migration of timers during VCPU scheduling will fail?

For hrtimers (which is the lapic emulation case), it only succeeds if
the destination core has a hrtimer expiring before the hrtimer being
migrated.

Also, if the hrtimer callback function is already running (that is,
the timer fired already) it's not migrated either. But I _guess_ this
case doesn't affect KVM (and there's no much do about it anyways).

[toc] | [prev] | [next] | [standalone]

#1371751

2016-04-05 14:18+0800, Yang Zhang:
> On 2016/4/5 5:00, Rik van Riel wrote:
>>Given that delivering a timer to a guest seems to
>>involve trapping from the guest to the host, anyway,
>>I don't see a downside to your patch.
>>
>>If that is ever changed (eg. allowing delivery of
>>a timer interrupt to a VCPU without trapping to the
>>host), we may want to revisit this.
> 
> Posted interrupt helps in this case. Currently, KVM doesn't use PI for lapic
> timer is due to same affinity for lapic timer and VCPU. Now, we can change
> to use PI for lapic timer. The only concern is what's frequency of timer
> migration in upstream Linux? If it is frequently, will it bring additional
> cost?

It's a scheduler bug if the timer migration frequency would matter. :)
Additional costs arise when the timer and VCPU are on two different
CPUs.  (e.g. if both CPUs are in deep C-state, we wasted one wakeup;
the timer would sometimes needs to send an interrupt.)

Fine tuned KVM could benefit from having the lapic timer backend on a
different physical core, but the general case would need some experience
to decide.

I think that we'd still want to have timer interrupts on the same
physical core if the host didn't have PI, and the fraction of timers
that can be injected without a guest entry is important to decide
whether PI can make the effort worthwhile.

The biggest benefit might come from handling multiple lapic timers in
one host interrupt.

[toc] | [prev] | [next] | [standalone]

#1373023

On 2016/4/5 23:54, Radim Krčmář wrote:
> 2016-04-05 14:18+0800, Yang Zhang:
>> On 2016/4/5 5:00, Rik van Riel wrote:
>>> Given that delivering a timer to a guest seems to
>>> involve trapping from the guest to the host, anyway,
>>> I don't see a downside to your patch.
>>>
>>> If that is ever changed (eg. allowing delivery of
>>> a timer interrupt to a VCPU without trapping to the
>>> host), we may want to revisit this.
>>
>> Posted interrupt helps in this case. Currently, KVM doesn't use PI for lapic
>> timer is due to same affinity for lapic timer and VCPU. Now, we can change
>> to use PI for lapic timer. The only concern is what's frequency of timer
>> migration in upstream Linux? If it is frequently, will it bring additional
>> cost?
>
> It's a scheduler bug if the timer migration frequency would matter. :)
> Additional costs arise when the timer and VCPU are on two different
> CPUs.  (e.g. if both CPUs are in deep C-state, we wasted one wakeup;
> the timer would sometimes needs to send an interrupt.)

Yes, it's possible. But the premise is VCPU is pinned to other CPU. 
Normally, the VCPU will wake up on the same CPU where timer interrupt is 
stay if CPU is idle.

>
> Fine tuned KVM could benefit from having the lapic timer backend on a
> different physical core, but the general case would need some experience
> to decide.
>
> I think that we'd still want to have timer interrupts on the same
> physical core if the host didn't have PI, and the fraction of timers
> that can be injected without a guest entry is important to decide
> whether PI can make the effort worthwhile.

Agree. I can do some experiences to see how much improvement we can get.

>
> The biggest benefit might come from handling multiple lapic timers in
> one host interrupt.

This is should be another story.We need to align multiple lapic timers 
into one timer firstly.:)

-- 
best regards
yang

[toc] | [prev] | [next] | [standalone]

#1371376

On 04/04/2016 22:46, Luiz Capitulino wrote:
> When a vCPU runs on a nohz_full core, the hrtimer used by
> the lapic emulation code can be migrated to another core.
> When this happens, it's possible to observe milisecond
> latency when delivering timer IRQs to KVM guests.
> 
> The huge latency is mainly due to the fact that
> apic_timer_fn() expects to run during a kvm exit. It
> sets KVM_REQ_PENDING_TIMER and let it be handled on kvm
> entry. However, if the timer fires on a different core,
> we have to wait until the next kvm exit for the guest
> to see KVM_REQ_PENDING_TIMER set.
> 
> This problem became visible after commit 9642d18ee. This
> commit changed the timer migration code to always attempt
> to migrate timers away from nohz_full cores. While it's
> discussable if this is correct/desirable (I don't think
> it is), it's clear that the lapic emulation code has
> a requirement on firing the hrtimer in the same core
> where it was started. This is achieved by making the
> hrtimer pinned.
> 
> Lastly, note that KVM has code to migrate timers when a
> vCPU is scheduled to run in different core. However, this
> forced migration may fail. When this happens, we can have
> the same problem. If we want 100% correctness, we'll have
> to modify apic_timer_fn() to cause a kvm exit when it runs
> on a different core than the vCPU. Not sure if this is
> possible.
> 
> Here's a reproducer for the issue being fixed:
> 
>  1. Set all cores but core0 to be nohz_full cores
>  2. Start a guest with a single vCPU
>  3. Trace apic_timer_fn() and kvm_inject_apic_timer_irqs()
> 
> You'll see that apic_timer_fn() will run in core0 while
> kvm_inject_apic_timer_irqs() runs in a different core. If
> you get both on core0, try running a program that takes 100%
> of the CPU and pin it to core0 to force the vCPU out.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 443d2a5..1a2da0e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1369,7 +1369,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
>  
>  		hrtimer_start(&apic->lapic_timer.timer,
>  			      ktime_add_ns(now, apic->lapic_timer.period),
> -			      HRTIMER_MODE_ABS);
> +			      HRTIMER_MODE_ABS_PINNED);
>  
>  		apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
>  			   PRIx64 ", "
> @@ -1402,7 +1402,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
>  			expire = ktime_add_ns(now, ns);
>  			expire = ktime_sub_ns(expire, lapic_timer_advance_ns);
>  			hrtimer_start(&apic->lapic_timer.timer,
> -				      expire, HRTIMER_MODE_ABS);
> +				      expire, HRTIMER_MODE_ABS_PINNED);
>  		} else
>  			apic_timer_expired(apic);
>  
> @@ -1868,7 +1868,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>  	apic->vcpu = vcpu;
>  
>  	hrtimer_init(&apic->lapic_timer.timer, CLOCK_MONOTONIC,
> -		     HRTIMER_MODE_ABS);
> +		     HRTIMER_MODE_ABS_PINNED);
>  	apic->lapic_timer.timer.function = apic_timer_fn;
>  
>  	/*
> @@ -2003,7 +2003,7 @@ void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>  
>  	timer = &vcpu->arch.apic->lapic_timer.timer;
>  	if (hrtimer_cancel(timer))
> -		hrtimer_start_expires(timer, HRTIMER_MODE_ABS);
> +		hrtimer_start_expires(timer, HRTIMER_MODE_ABS_PINNED);
>  }
>  
>  /*
> 

Queued for 4.6.0-rc3, thanks.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web