[PATCH] KVM: disable halt_poll_ns as default for s390x

[PATCH] KVM: disable halt_poll_ns as default for s390x

[David Hildenbrand]

We observed some performance degradation on s390x with dynamic
halt polling. Until we can provide a proper fix, let's enable
halt_poll_ns as default only for supported architectures.

Architectures are now free to set their own halt_poll_ns
default value.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.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/include/asm/kvm_host.h     | 1 +
 virt/kvm/kvm_main.c                 | 4 ++--
 7 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index dcba0fa..780499f 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -39,6 +39,7 @@
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 #define KVM_HAVE_ONE_REG
+#define KVM_HALT_POLL_NS_DEFAULT 500000
 
 #define KVM_VCPU_MAX_FEATURES 2
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 415938d..bac73c8 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -39,6 +39,7 @@
 #define KVM_USER_MEM_SLOTS 32
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
+#define KVM_HALT_POLL_NS_DEFAULT 500000
 
 #include <kvm/arm_vgic.h>
 #include <kvm/arm_arch_timer.h>
diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h
index e8c8d9d..3b8aca4 100644
--- a/arch/mips/include/asm/kvm_host.h
+++ b/arch/mips/include/asm/kvm_host.h
@@ -61,6 +61,7 @@
 #define KVM_PRIVATE_MEM_SLOTS 	0
 
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
+#define KVM_HALT_POLL_NS_DEFAULT 500000
 
 
 
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 98eebbf..4e51268 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -44,6 +44,7 @@
 #ifdef CONFIG_KVM_MMIO
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 #endif
+#define KVM_HALT_POLL_NS_DEFAULT 500000
 
 /* These values are internal and can be increased later */
 #define KVM_NR_IRQCHIPS          1
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 3d012e0..0cb5aad 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -35,6 +35,7 @@
  */
 #define KVM_NR_IRQCHIPS 1
 #define KVM_IRQCHIP_NUM_PINS 4096
+#define KVM_HALT_POLL_NS_DEFAULT 0
 
 #define SIGP_CTRL_C		0x80
 #define SIGP_CTRL_SCN_MASK	0x3f
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c12e845..a66a98c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -40,6 +40,7 @@
 
 #define KVM_PIO_PAGE_OFFSET 1
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 2
+#define KVM_HALT_POLL_NS_DEFAULT 500000
 
 #define KVM_IRQCHIP_NUM_PINS  KVM_IOAPIC_NUM_PINS
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index eb4c9d2..a447c8c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -66,8 +66,8 @@
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
-/* halt polling only reduces halt latency by 5-7 us, 500us is enough */
-static unsigned int halt_poll_ns = 500000;
+/* Architectures should define their poll value according to the halt latency */
+static unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
 module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
 
 /* Default doubles per-vcpu halt_poll_ns. */
-- 
2.3.8

Re: [PATCH] KVM: disable halt_poll_ns as default for s390x

[Christian Borntraeger]

Am 18.09.2015 um 12:34 schrieb David Hildenbrand:
> We observed some performance degradation on s390x with dynamic
> halt polling. Until we can provide a proper fix, let's enable
> halt_poll_ns as default only for supported architectures.
> 
> Architectures are now free to set their own halt_poll_ns
> default value.
> 
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
[...]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index eb4c9d2..a447c8c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -66,8 +66,8 @@
>  MODULE_AUTHOR("Qumranet");
>  MODULE_LICENSE("GPL");
> 
> -/* halt polling only reduces halt latency by 5-7 us, 500us is enough */
> -static unsigned int halt_poll_ns = 500000;
> +/* Architectures should define their poll value according to the halt latency */
> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;

Yes, I prefer this over disabling it via Kconfig. There are benchmarks which
benefit from polling on s390. Furthermore it seems that the latency
strongly depends on timing of the architecture so making it per arch is
probably the right thing to do.


Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>





>  module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
> 
>  /* Default doubles per-vcpu halt_poll_ns. */
> 

Re: [PATCH] KVM: disable halt_poll_ns as default for s390x

[Paolo Bonzini]

On 18/09/2015 12:54, Christian Borntraeger wrote:
> > -/* halt polling only reduces halt latency by 5-7 us, 500us is enough */
> > -static unsigned int halt_poll_ns = 500000;
> > +/* Architectures should define their poll value according to the halt latency */
> > +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
> 
> Yes, I prefer this over disabling it via Kconfig. There are benchmarks which
> benefit from polling on s390. Furthermore it seems that the latency
> strongly depends on timing of the architecture so making it per arch is
> probably the right thing to do.

Perhaps a #ifndef is better than replicating the 500us default in all
architectures?  Or should the default be 0?

Paolo

Re: [PATCH] KVM: disable halt_poll_ns as default for s390x

[Christian Borntraeger]

Am 18.09.2015 um 13:29 schrieb Paolo Bonzini:
> 
> 
> On 18/09/2015 12:54, Christian Borntraeger wrote:
>>> -/* halt polling only reduces halt latency by 5-7 us, 500us is enough */
>>> -static unsigned int halt_poll_ns = 500000;
>>> +/* Architectures should define their poll value according to the halt latency */
>>> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
>>
>> Yes, I prefer this over disabling it via Kconfig. There are benchmarks which
>> benefit from polling on s390. Furthermore it seems that the latency
>> strongly depends on timing of the architecture so making it per arch is
>> probably the right thing to do.
> 
> Perhaps a #ifndef is better than replicating the 500us default in all
> architectures?  Or should the default be 0?

I prefer to not have an ifdef in .c files. I would assume that over time
architectures will provide their own number, e.g. for my uperf cases that
got better, 50000 was enough. 500000 just increased cpu overhead.

Christian

Re: [PATCH] KVM: disable halt_poll_ns as default for s390x

[Christian Borntraeger]

Am 18.09.2015 um 13:29 schrieb Paolo Bonzini:
> 
> 
> On 18/09/2015 12:54, Christian Borntraeger wrote:
>>> -/* halt polling only reduces halt latency by 5-7 us, 500us is enough */
>>> -static unsigned int halt_poll_ns = 500000;
>>> +/* Architectures should define their poll value according to the halt latency */
>>> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
>>
>> Yes, I prefer this over disabling it via Kconfig. There are benchmarks which
>> benefit from polling on s390. Furthermore it seems that the latency
>> strongly depends on timing of the architecture so making it per arch is
>> probably the right thing to do.
> 
> Perhaps a #ifndef is better than replicating the 500us default in all
> architectures?  Or should the default be 0?

Any guidance from your side? All different proposals are certainly ok.
Are you going to take Davids patch or shall he respin?

Christian

Re: [PATCH] KVM: disable halt_poll_ns as default for s390x

[Paolo Bonzini]

On 24/09/2015 13:06, Christian Borntraeger wrote:
> Am 18.09.2015 um 13:29 schrieb Paolo Bonzini:
>>
>>
>> On 18/09/2015 12:54, Christian Borntraeger wrote:
>>>> -/* halt polling only reduces halt latency by 5-7 us, 500us is enough */
>>>> -static unsigned int halt_poll_ns = 500000;
>>>> +/* Architectures should define their poll value according to the halt latency */
>>>> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
>>>
>>> Yes, I prefer this over disabling it via Kconfig. There are benchmarks which
>>> benefit from polling on s390. Furthermore it seems that the latency
>>> strongly depends on timing of the architecture so making it per arch is
>>> probably the right thing to do.
>>
>> Perhaps a #ifndef is better than replicating the 500us default in all
>> architectures?  Or should the default be 0?
> 
> Any guidance from your side? All different proposals are certainly ok.
> Are you going to take Davids patch or shall he respin?

I've committed the patch as is, and I'm preparing a pull request.

Paolo