diff mbox

KVM: halt-polling: poll if emulated lapic timer will fire soon

Message ID 1463649990-5889-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wanpeng Li May 19, 2016, 9:26 a.m. UTC 
From: Wanpeng Li <wanpeng.li@hotmail.com>

If an emulated lapic timer will fire soon(in the scope of 10us the 
base of dynamic halt-polling, lower-end of message passing workload 
latency TCP_RR's poll time < 10us) we can treat it as a short halt, 
and poll to wait it fire, the fire callback apic_timer_fn() will set 
KVM_REQ_PENDING_TIMER, and this flag will be check during busy poll. 
This can avoid context switch overhead and the latency which we wake 
up vCPU.

iperf TCP get ~6% bandwidth improvement.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Kr?má? <rkrcmar@redhat.com>
Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/arm/include/asm/kvm_host.h     |  1 +
 arch/arm64/include/asm/kvm_host.h   |  1 +
 arch/mips/include/asm/kvm_host.h    |  1 +
 arch/powerpc/include/asm/kvm_host.h |  1 +
 arch/s390/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/lapic.c                |  7 +++++++
 arch/x86/kvm/lapic.h                |  1 +
 arch/x86/kvm/x86.c                  |  5 +++++
 include/linux/kvm_host.h            |  1 +
 virt/kvm/kvm_main.c                 | 13 ++++++++++---
 10 files changed, 29 insertions(+), 3 deletions(-)

Comments

Christian Borntraeger May 19, 2016, 11:23 a.m. UTC | #1 
On 05/19/2016 11:26 AM, Wanpeng Li wrote:

I think in general a good idea to poll if a timer will expire soon.

Some patch comments:

Same for all non-x86 archs:
> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}

A function returning int, without a return statement? 
That gives at least a compiler warning.

> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -78,6 +78,9 @@ module_param(halt_poll_ns_grow, uint, S_IRUGO | S_IWUSR);
>  static unsigned int halt_poll_ns_shrink;
>  module_param(halt_poll_ns_shrink, uint, S_IRUGO | S_IWUSR);
> 
> +/* lower-end of message passing workload latency TCP_RR's poll time < 10us */
> +static unsigned int halt_poll_ns_base = 10000;
> +
>  /*
>   * Ordering of locks:
>   *
> @@ -1966,7 +1969,7 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
>  	grow = READ_ONCE(halt_poll_ns_grow);
>  	/* 10us base */
>  	if (val == 0 && grow)
> -		val = 10000;
> +		val = halt_poll_ns_base;
>  	else
>  		val *= grow;
> 
> @@ -2015,11 +2018,15 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  	DECLARE_SWAITQUEUE(wait);
>  	bool waited = false;
>  	u64 block_ns;
> +	unsigned int delta, remaining;
> 
> +	remaining = kvm_arch_timer_remaining(vcpu);

and now it causes undefined behaviour, no?


>  	start = cur = ktime_get();
> -	if (vcpu->halt_poll_ns) {
> -		ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
> +	if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
> +		ktime_t stop;
> 
> +		delta = vcpu->halt_poll_ns ? vcpu->halt_poll_ns : remaining;
> +		stop = ktime_add_ns(ktime_get(), delta);
>  		++vcpu->stat.halt_attempted_poll;
>  		do {
>  			/*
> 

So you avoid to shrink/grow for these cases? Probably makes sense


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wanpeng Li May 19, 2016, 11:35 a.m. UTC | #2 
2016-05-19 19:23 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
> On 05/19/2016 11:26 AM, Wanpeng Li wrote:
>
> I think in general a good idea to poll if a timer will expire soon.
>
> Some patch comments:
>
> Same for all non-x86 archs:
>> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
>
> A function returning int, without a return statement?
> That gives at least a compiler warning.

How about return 0 for all non-x86 archs?

>
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -78,6 +78,9 @@ module_param(halt_poll_ns_grow, uint, S_IRUGO | S_IWUSR);
>>  static unsigned int halt_poll_ns_shrink;
>>  module_param(halt_poll_ns_shrink, uint, S_IRUGO | S_IWUSR);
>>
>> +/* lower-end of message passing workload latency TCP_RR's poll time < 10us */
>> +static unsigned int halt_poll_ns_base = 10000;
>> +
>>  /*
>>   * Ordering of locks:
>>   *
>> @@ -1966,7 +1969,7 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
>>       grow = READ_ONCE(halt_poll_ns_grow);
>>       /* 10us base */
>>       if (val == 0 && grow)
>> -             val = 10000;
>> +             val = halt_poll_ns_base;
>>       else
>>               val *= grow;
>>
>> @@ -2015,11 +2018,15 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>>       DECLARE_SWAITQUEUE(wait);
>>       bool waited = false;
>>       u64 block_ns;
>> +     unsigned int delta, remaining;
>>
>> +     remaining = kvm_arch_timer_remaining(vcpu);
>
> and now it causes undefined behaviour, no?

Ditto.

>
>
>>       start = cur = ktime_get();
>> -     if (vcpu->halt_poll_ns) {
>> -             ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
>> +     if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
>> +             ktime_t stop;
>>
>> +             delta = vcpu->halt_poll_ns ? vcpu->halt_poll_ns : remaining;
>> +             stop = ktime_add_ns(ktime_get(), delta);
>>               ++vcpu->stat.halt_attempted_poll;
>>               do {
>>                       /*
>>
>
> So you avoid to shrink/grow for these cases? Probably makes sense

I think my patch also shrink/grow for these cases.

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Borntraeger May 19, 2016, 11:42 a.m. UTC | #3 
On 05/19/2016 01:35 PM, Wanpeng Li wrote:
> 2016-05-19 19:23 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
>> On 05/19/2016 11:26 AM, Wanpeng Li wrote:
>>
>> I think in general a good idea to poll if a timer will expire soon.
>>
>> Some patch comments:
>>
>> Same for all non-x86 archs:
>>> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
>>
>> A function returning int, without a return statement?
>> That gives at least a compiler warning.
> 
> How about return 0 for all non-x86 archs?

We will provide an s390 implementation soon, but until then a proper
default would be good.

[....]
>>> +     if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {

but then remaining is 0 and the 2nd condition will always be true, no?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wanpeng Li May 19, 2016, 11:48 a.m. UTC | #4 
2016-05-19 19:42 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
> On 05/19/2016 01:35 PM, Wanpeng Li wrote:
>> 2016-05-19 19:23 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
>>> On 05/19/2016 11:26 AM, Wanpeng Li wrote:
>>>
>>> I think in general a good idea to poll if a timer will expire soon.
>>>
>>> Some patch comments:
>>>
>>> Same for all non-x86 archs:
>>>> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
>>>
>>> A function returning int, without a return statement?
>>> That gives at least a compiler warning.
>>
>> How about return 0 for all non-x86 archs?
>
> We will provide an s390 implementation soon, but until then a proper
> default would be good.
>
> [....]
>>>> +     if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
>
> but then remaining is 0 and the 2nd condition will always be true, no?

Nice catch!

How about something like below:

+       if (vcpu->halt_poll_ns ||
+               (remaining != 0 && remaining < halt_poll_ns_base)) {

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Borntraeger May 19, 2016, 11:56 a.m. UTC | #5 
On 05/19/2016 01:48 PM, Wanpeng Li wrote:
> 2016-05-19 19:42 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
>> On 05/19/2016 01:35 PM, Wanpeng Li wrote:
>>> 2016-05-19 19:23 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
>>>> On 05/19/2016 11:26 AM, Wanpeng Li wrote:
>>>>
>>>> I think in general a good idea to poll if a timer will expire soon.
>>>>
>>>> Some patch comments:
>>>>
>>>> Same for all non-x86 archs:
>>>>> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
>>>>
>>>> A function returning int, without a return statement?
>>>> That gives at least a compiler warning.
>>>
>>> How about return 0 for all non-x86 archs?
>>
>> We will provide an s390 implementation soon, but until then a proper
>> default would be good.
>>
>> [....]
>>>>> +     if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
>>
>> but then remaining is 0 and the 2nd condition will always be true, no?
> 
> Nice catch!
> 
> How about something like below:
> 
> +       if (vcpu->halt_poll_ns ||
> +               (remaining != 0 && remaining < halt_poll_ns_base)) {

Maybe just use -1UL to have a "will never expire" and change the return value into u64
while changing that.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wanpeng Li May 19, 2016, noon UTC | #6 
2016-05-19 19:56 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
> On 05/19/2016 01:48 PM, Wanpeng Li wrote:
>> 2016-05-19 19:42 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
>>> On 05/19/2016 01:35 PM, Wanpeng Li wrote:
>>>> 2016-05-19 19:23 GMT+08:00 Christian Borntraeger <borntraeger@de.ibm.com>:
>>>>> On 05/19/2016 11:26 AM, Wanpeng Li wrote:
>>>>>
>>>>> I think in general a good idea to poll if a timer will expire soon.
>>>>>
>>>>> Some patch comments:
>>>>>
>>>>> Same for all non-x86 archs:
>>>>>> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
>>>>>
>>>>> A function returning int, without a return statement?
>>>>> That gives at least a compiler warning.
>>>>
>>>> How about return 0 for all non-x86 archs?
>>>
>>> We will provide an s390 implementation soon, but until then a proper
>>> default would be good.
>>>
>>> [....]
>>>>>> +     if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
>>>
>>> but then remaining is 0 and the 2nd condition will always be true, no?
>>
>> Nice catch!
>>
>> How about something like below:
>>
>> +       if (vcpu->halt_poll_ns ||
>> +               (remaining != 0 && remaining < halt_poll_ns_base)) {
>
> Maybe just use -1UL to have a "will never expire" and change the return value into u64
> while changing that.

Good idea, I will do it in next version.

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 4cd8732..473d908 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -284,6 +284,7 @@  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
 static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
+static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
 
 static inline void kvm_arm_init_debug(void) {}
 static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d49399d..18a7e66 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -359,6 +359,7 @@  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
 static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
+static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
 
 void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h
index 9a37a10..6c6d941 100644
--- a/arch/mips/include/asm/kvm_host.h
+++ b/arch/mips/include/asm/kvm_host.h
@@ -813,6 +813,7 @@  static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
 static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
+static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
 
 #endif /* __MIPS_KVM_HOST_H__ */
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index ec35af3..3a6134f 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -729,5 +729,6 @@  static inline void kvm_arch_exit(void) {}
 static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
+static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
 
 #endif /* __POWERPC_KVM_HOST_H__ */
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 37b9017..f8eb0f5 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -696,6 +696,7 @@  static inline void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 		struct kvm_memory_slot *slot) {}
 static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
+static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) {}
 
 void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu);
 
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index bbb5b28..f623df9 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -256,6 +256,13 @@  static inline int apic_lvtt_tscdeadline(struct kvm_lapic *apic)
 	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_TSCDEADLINE;
 }
 
+unsigned int apic_get_timer_expire(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	return ktime_to_ns(hrtimer_get_remaining(&apic->lapic_timer.timer));
+}
+
 static inline int apic_lvt_nmi_mode(u32 lvt_val)
 {
 	return (lvt_val & (APIC_MODE_MASK | APIC_LVT_MASKED)) == APIC_DM_NMI;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 891c6da..3b1fb6e 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -212,4 +212,5 @@  bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
 			struct kvm_vcpu **dest_vcpu);
 int kvm_vector_to_index(u32 vector, u32 dest_vcpus,
 			const unsigned long *bitmap, u32 bitmap_size);
+unsigned int apic_get_timer_expire(struct kvm_vcpu *vcpu);
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a8c7ca3..cf138f6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7623,6 +7623,11 @@  bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu)
 struct static_key kvm_no_apic_vcpu __read_mostly;
 EXPORT_SYMBOL_GPL(kvm_no_apic_vcpu);
 
+unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu)
+{
+	return apic_get_timer_expire(vcpu);
+}
+
 int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	struct page *page;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b1fa8f1..f213adc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -663,6 +663,7 @@  int kvm_vcpu_yield_to(struct kvm_vcpu *target);
 void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu);
 void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
+unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu);
 
 void kvm_flush_remote_tlbs(struct kvm *kvm);
 void kvm_reload_remote_mmus(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index dd4ac9d..5ed509f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -78,6 +78,9 @@  module_param(halt_poll_ns_grow, uint, S_IRUGO | S_IWUSR);
 static unsigned int halt_poll_ns_shrink;
 module_param(halt_poll_ns_shrink, uint, S_IRUGO | S_IWUSR);
 
+/* lower-end of message passing workload latency TCP_RR's poll time < 10us */
+static unsigned int halt_poll_ns_base = 10000;
+
 /*
  * Ordering of locks:
  *
@@ -1966,7 +1969,7 @@  static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
 	grow = READ_ONCE(halt_poll_ns_grow);
 	/* 10us base */
 	if (val == 0 && grow)
-		val = 10000;
+		val = halt_poll_ns_base;
 	else
 		val *= grow;
 
@@ -2015,11 +2018,15 @@  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 	DECLARE_SWAITQUEUE(wait);
 	bool waited = false;
 	u64 block_ns;
+	unsigned int delta, remaining;
 
+	remaining = kvm_arch_timer_remaining(vcpu);
 	start = cur = ktime_get();
-	if (vcpu->halt_poll_ns) {
-		ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
+	if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
+		ktime_t stop;
 
+		delta = vcpu->halt_poll_ns ? vcpu->halt_poll_ns : remaining;
+		stop = ktime_add_ns(ktime_get(), delta);
 		++vcpu->stat.halt_attempted_poll;
 		do {
 			/*