Re: [patch v2 3/3] cpuidle-haltpoll: disable host side polling when kvm virtualized

[Marcelo Tosatti <mtosatti@redhat.com>]

Hi Joao,

On Thu, Jun 06, 2019 at 07:25:28PM +0100, Joao Martins wrote:
> On 6/4/19 1:24 PM, Marcelo Tosatti wrote:
> > 
> > When performing guest side polling, it is not necessary to 
> > also perform host side polling. 
> > 
> > So disable host side polling, via the new MSR interface, 
> > when loading cpuidle-haltpoll driver.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > ---
> > 
> > v2: remove extra "}"
> > 
> >  arch/x86/Kconfig                        |    7 +++++
> >  arch/x86/include/asm/cpuidle_haltpoll.h |    8 ++++++
> >  arch/x86/kernel/kvm.c                   |   40 ++++++++++++++++++++++++++++++++
> >  drivers/cpuidle/cpuidle-haltpoll.c      |    9 ++++++-
> >  include/linux/cpuidle_haltpoll.h        |   16 ++++++++++++
> >  5 files changed, 79 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.git/arch/x86/include/asm/cpuidle_haltpoll.h
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6.git/arch/x86/include/asm/cpuidle_haltpoll.h	2019-06-03 19:38:42.328718617 -0300
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ARCH_HALTPOLL_H
> > +#define _ARCH_HALTPOLL_H
> > +
> > +void arch_haltpoll_enable(void);
> > +void arch_haltpoll_disable(void);
> > +
> > +#endif
> > Index: linux-2.6.git/drivers/cpuidle/cpuidle-haltpoll.c
> > ===================================================================
> > --- linux-2.6.git.orig/drivers/cpuidle/cpuidle-haltpoll.c	2019-06-03 19:38:12.376619124 -0300
> > +++ linux-2.6.git/drivers/cpuidle/cpuidle-haltpoll.c	2019-06-03 19:38:42.328718617 -0300
> > @@ -15,6 +15,7 @@
> >  #include <linux/module.h>
> >  #include <linux/timekeeping.h>
> >  #include <linux/sched/idle.h>
> > +#include <linux/cpuidle_haltpoll.h>
> >  #define CREATE_TRACE_POINTS
> >  #include "cpuidle-haltpoll-trace.h"
> >  
> > @@ -157,11 +158,17 @@
> >  
> >  static int __init haltpoll_init(void)
> >  {
> > -	return cpuidle_register(&haltpoll_driver, NULL);
> > +	int ret = cpuidle_register(&haltpoll_driver, NULL);
> > +
> > +	if (ret == 0)
> > +		arch_haltpoll_enable();
> > +
> > +	return ret;
> >  }
> >  
> >  static void __exit haltpoll_exit(void)
> >  {
> > +	arch_haltpoll_disable();
> >  	cpuidle_unregister(&haltpoll_driver);
> >  }
> >  
> > Index: linux-2.6.git/include/linux/cpuidle_haltpoll.h
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6.git/include/linux/cpuidle_haltpoll.h	2019-06-03 19:41:57.293366260 -0300
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _CPUIDLE_HALTPOLL_H
> > +#define _CPUIDLE_HALTPOLL_H
> > +
> > +#ifdef CONFIG_ARCH_CPUIDLE_HALTPOLL
> > +#include <asm/cpuidle_haltpoll.h>
> > +#else
> > +static inline void arch_haltpoll_enable(void)
> > +{
> > +}
> > +
> > +static inline void arch_haltpoll_disable(void)
> > +{
> > +}
> > +#endif
> > +#endif
> > Index: linux-2.6.git/arch/x86/Kconfig
> > ===================================================================
> > --- linux-2.6.git.orig/arch/x86/Kconfig	2019-06-03 19:38:12.376619124 -0300
> > +++ linux-2.6.git/arch/x86/Kconfig	2019-06-03 19:42:34.478489868 -0300
> > @@ -787,6 +787,7 @@
> >  	bool "KVM Guest support (including kvmclock)"
> >  	depends on PARAVIRT
> >  	select PARAVIRT_CLOCK
> > +	select ARCH_CPUIDLE_HALTPOLL
> >  	default y
> >  	---help---
> >  	  This option enables various optimizations for running under the KVM
> > @@ -795,6 +796,12 @@
> >  	  underlying device model, the host provides the guest with
> >  	  timing infrastructure such as time of day, and system time
> >  
> > +config ARCH_CPUIDLE_HALTPOLL
> > +        def_bool n
> > +        proUmpt "Disable host haltpoll when loading haltpoll driver"
> > +        help
> > +	  If virtualized under KVM, disable host haltpoll.
> > +
> >  config PVH
> >  	bool "Support for running PVH guests"
> >  	---help---
> > Index: linux-2.6.git/arch/x86/kernel/kvm.c
> > ===================================================================
> > --- linux-2.6.git.orig/arch/x86/kernel/kvm.c	2019-06-03 19:38:12.376619124 -0300
> > +++ linux-2.6.git/arch/x86/kernel/kvm.c	2019-06-03 19:40:14.359024312 -0300
> > @@ -853,3 +853,43 @@
> >  }
> >  
> >  #endif	/* CONFIG_PARAVIRT_SPINLOCKS */
> > +
> > +#ifdef CONFIG_ARCH_CPUIDLE_HALTPOLL
> > +
> > +void kvm_disable_host_haltpoll(void *i)
> > +{
> > +	wrmsrl(MSR_KVM_POLL_CONTROL, 0);
> > +}
> > +
> > +void kvm_enable_host_haltpoll(void *i)
> > +{
> > +	wrmsrl(MSR_KVM_POLL_CONTROL, 1);
> > +}
> > +
> > +void arch_haltpoll_enable(void)
> > +{
> > +	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL))
> > +		return;
> > +
> 
> Perhaps warn the user when failing to disable host poll e.g.:
> 
> if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
> 	pr_warn_once("haltpoll: Failed to disable host halt polling\n");
> 	return;
> }
> 
> But I wonder whether we should fail to load cpuidle-haltpoll when host halt
> polling can't be disabled[*]? That is to avoid polling in both host and guest
> and *possibly* avoid chances for performance regressions when running on older
> hypervisors?
> 
> [*] with guest still able load with lack of host polling control via modparam

I don't think so. First, the driver reduces the halt poll interval when
the system is idle, so overhead is reduced to zero in such cases.

Moreover, when both host and guest side polling are performed, 
power might be wasted, but i haven't seen a significant 
performance regression caused by it.

Thanks.

> 
> > +	preempt_disable();
> > +	/* Enabling guest halt poll disables host halt poll */
> > +	kvm_disable_host_haltpoll(NULL);
> > +	smp_call_function(kvm_disable_host_haltpoll, NULL, 1);
> > +	preempt_enable();
> > +}
> > +EXPORT_SYMBOL_GPL(arch_haltpoll_enable);
> > +
> > +void arch_haltpoll_disable(void)
> > +{
> > +	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL))
> > +		return;
> > +
> > +	preempt_disable();
> > +	/* Enabling guest halt poll disables host halt poll */
> > +	kvm_enable_host_haltpoll(NULL);
> > +	smp_call_function(kvm_enable_host_haltpoll, NULL, 1);
> > +	preempt_enable();
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(arch_haltpoll_disable);
> > +#endif
> >