| Message ID | 1442572493-51400-1-git-send-email-dahi@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. */ > -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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 --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. */
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(-)