[PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Joao Martins]

When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
past the online ones and thus fail to register the idle driver.
This is because cpuidle_add_sysfs() will return with -ENODEV as a
consequence from get_cpu_device() return no device for a non-existing
CPU.

Instead switch to cpuidle_register_driver() and manually register each
of the present cpus through cpuhp_setup_state() callback and future
ones that get onlined. This mimmics similar logic that intel_idle does.

Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
v2:
* move cpus_read_unlock() right after unregistering all cpuidle_devices;
(Marcello Tosatti)
* redundant usage of cpuidle_unregister() when only
cpuidle_unregister_driver() suffices; (Marcelo Tosatti)
* cpuhp_setup_state() returns a state (> 0) on success with CPUHP_AP_ONLINE_DYN
thus we set @ret to 0
---
 arch/x86/include/asm/cpuidle_haltpoll.h |  4 +-
 arch/x86/kernel/kvm.c                   | 18 +++----
 drivers/cpuidle/cpuidle-haltpoll.c      | 67 +++++++++++++++++++++++--
 include/linux/cpuidle_haltpoll.h        |  4 +-
 4 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/cpuidle_haltpoll.h b/arch/x86/include/asm/cpuidle_haltpoll.h
index ff8607d81526..c8b39c6716ff 100644
--- a/arch/x86/include/asm/cpuidle_haltpoll.h
+++ b/arch/x86/include/asm/cpuidle_haltpoll.h
@@ -2,7 +2,7 @@
 #ifndef _ARCH_HALTPOLL_H
 #define _ARCH_HALTPOLL_H
 
-void arch_haltpoll_enable(void);
-void arch_haltpoll_disable(void);
+void arch_haltpoll_enable(unsigned int cpu);
+void arch_haltpoll_disable(unsigned int cpu);
 
 #endif
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8d150e3732d9..a9b6c4e2446d 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -880,32 +880,26 @@ static void kvm_enable_host_haltpoll(void *i)
 	wrmsrl(MSR_KVM_POLL_CONTROL, 1);
 }
 
-void arch_haltpoll_enable(void)
+void arch_haltpoll_enable(unsigned int cpu)
 {
 	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
-		printk(KERN_ERR "kvm: host does not support poll control\n");
-		printk(KERN_ERR "kvm: host upgrade recommended\n");
+		pr_err_once("kvm: host does not support poll control\n");
+		pr_err_once("kvm: host upgrade recommended\n");
 		return;
 	}
 
-	preempt_disable();
 	/* Enable guest halt poll disables host halt poll */
-	kvm_disable_host_haltpoll(NULL);
-	smp_call_function(kvm_disable_host_haltpoll, NULL, 1);
-	preempt_enable();
+	smp_call_function_single(cpu, kvm_disable_host_haltpoll, NULL, 1);
 }
 EXPORT_SYMBOL_GPL(arch_haltpoll_enable);
 
-void arch_haltpoll_disable(void)
+void arch_haltpoll_disable(unsigned int cpu)
 {
 	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL))
 		return;
 
-	preempt_disable();
 	/* Enable guest halt poll disables host halt poll */
-	kvm_enable_host_haltpoll(NULL);
-	smp_call_function(kvm_enable_host_haltpoll, NULL, 1);
-	preempt_enable();
+	smp_call_function_single(cpu, kvm_enable_host_haltpoll, NULL, 1);
 }
 EXPORT_SYMBOL_GPL(arch_haltpoll_disable);
 #endif
diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c
index 9ac093dcbb01..8baade23f8d0 100644
--- a/drivers/cpuidle/cpuidle-haltpoll.c
+++ b/drivers/cpuidle/cpuidle-haltpoll.c
@@ -11,12 +11,15 @@
  */
 
 #include <linux/init.h>
+#include <linux/cpu.h>
 #include <linux/cpuidle.h>
 #include <linux/module.h>
 #include <linux/sched/idle.h>
 #include <linux/kvm_para.h>
 #include <linux/cpuidle_haltpoll.h>
 
+static struct cpuidle_device __percpu *haltpoll_cpuidle_devices;
+
 static int default_enter_idle(struct cpuidle_device *dev,
 			      struct cpuidle_driver *drv, int index)
 {
@@ -46,6 +49,48 @@ static struct cpuidle_driver haltpoll_driver = {
 	.state_count = 2,
 };
 
+static int haltpoll_cpu_online(unsigned int cpu)
+{
+	struct cpuidle_device *dev;
+
+	dev = per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
+	if (!dev->registered) {
+		dev->cpu = cpu;
+		if (cpuidle_register_device(dev)) {
+			pr_notice("cpuidle_register_device %d failed!\n", cpu);
+			return -EIO;
+		}
+		arch_haltpoll_enable(cpu);
+	}
+
+	return 0;
+}
+
+static void haltpoll_uninit(void)
+{
+	unsigned int cpu;
+
+	cpus_read_lock();
+
+	for_each_online_cpu(cpu) {
+		struct cpuidle_device *dev =
+			per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
+
+		if (!dev->registered)
+			continue;
+
+		arch_haltpoll_disable(cpu);
+		cpuidle_unregister_device(dev);
+	}
+
+	cpus_read_unlock();
+
+	cpuidle_unregister_driver(&haltpoll_driver);
+
+	free_percpu(haltpoll_cpuidle_devices);
+	haltpoll_cpuidle_devices = NULL;
+}
+
 static int __init haltpoll_init(void)
 {
 	int ret;
@@ -56,17 +101,29 @@ static int __init haltpoll_init(void)
 	if (!kvm_para_available())
 		return 0;
 
-	ret = cpuidle_register(&haltpoll_driver, NULL);
-	if (ret == 0)
-		arch_haltpoll_enable();
+	ret = cpuidle_register_driver(drv);
+	if (ret < 0)
+		return ret;
+
+	haltpoll_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (haltpoll_cpuidle_devices == NULL) {
+		cpuidle_unregister_driver(drv);
+		return -ENOMEM;
+	}
+
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/haltpoll:online",
+				haltpoll_cpu_online, NULL);
+	if (ret < 0)
+		haltpoll_uninit();
+	else
+		ret = 0;
 
 	return ret;
 }
 
 static void __exit haltpoll_exit(void)
 {
-	arch_haltpoll_disable();
-	cpuidle_unregister(&haltpoll_driver);
+	haltpoll_uninit();
 }
 
 module_init(haltpoll_init);
diff --git a/include/linux/cpuidle_haltpoll.h b/include/linux/cpuidle_haltpoll.h
index fe5954c2409e..d50c1e0411a2 100644
--- a/include/linux/cpuidle_haltpoll.h
+++ b/include/linux/cpuidle_haltpoll.h
@@ -5,11 +5,11 @@
 #ifdef CONFIG_ARCH_CPUIDLE_HALTPOLL
 #include <asm/cpuidle_haltpoll.h>
 #else
-static inline void arch_haltpoll_enable(void)
+static inline void arch_haltpoll_enable(unsigned int cpu)
 {
 }
 
-static inline void arch_haltpoll_disable(void)
+static inline void arch_haltpoll_disable(unsigned int cpu)
 {
 }
 #endif
-- 
2.17.1

Re: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Marcelo Tosatti]

On Thu, Aug 29, 2019 at 04:10:27PM +0100, Joao Martins wrote:
> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
> past the online ones and thus fail to register the idle driver.
> This is because cpuidle_add_sysfs() will return with -ENODEV as a
> consequence from get_cpu_device() return no device for a non-existing
> CPU.
> 
> Instead switch to cpuidle_register_driver() and manually register each
> of the present cpus through cpuhp_setup_state() callback and future
> ones that get onlined. This mimmics similar logic that intel_idle does.
> 
> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> v2:
> * move cpus_read_unlock() right after unregistering all cpuidle_devices;
> (Marcello Tosatti)
> * redundant usage of cpuidle_unregister() when only
> cpuidle_unregister_driver() suffices; (Marcelo Tosatti)
> * cpuhp_setup_state() returns a state (> 0) on success with CPUHP_AP_ONLINE_DYN
> thus we set @ret to 0
> ---
>  arch/x86/include/asm/cpuidle_haltpoll.h |  4 +-
>  arch/x86/kernel/kvm.c                   | 18 +++----
>  drivers/cpuidle/cpuidle-haltpoll.c      | 67 +++++++++++++++++++++++--
>  include/linux/cpuidle_haltpoll.h        |  4 +-
>  4 files changed, 72 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpuidle_haltpoll.h b/arch/x86/include/asm/cpuidle_haltpoll.h
> index ff8607d81526..c8b39c6716ff 100644
> --- a/arch/x86/include/asm/cpuidle_haltpoll.h
> +++ b/arch/x86/include/asm/cpuidle_haltpoll.h
> @@ -2,7 +2,7 @@
>  #ifndef _ARCH_HALTPOLL_H
>  #define _ARCH_HALTPOLL_H
>  
> -void arch_haltpoll_enable(void);
> -void arch_haltpoll_disable(void);
> +void arch_haltpoll_enable(unsigned int cpu);
> +void arch_haltpoll_disable(unsigned int cpu);
>  
>  #endif
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8d150e3732d9..a9b6c4e2446d 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -880,32 +880,26 @@ static void kvm_enable_host_haltpoll(void *i)
>  	wrmsrl(MSR_KVM_POLL_CONTROL, 1);
>  }
>  
> -void arch_haltpoll_enable(void)
> +void arch_haltpoll_enable(unsigned int cpu)
>  {
>  	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
> -		printk(KERN_ERR "kvm: host does not support poll control\n");
> -		printk(KERN_ERR "kvm: host upgrade recommended\n");
> +		pr_err_once("kvm: host does not support poll control\n");
> +		pr_err_once("kvm: host upgrade recommended\n");
>  		return;
>  	}
>  
> -	preempt_disable();
>  	/* Enable guest halt poll disables host halt poll */
> -	kvm_disable_host_haltpoll(NULL);
> -	smp_call_function(kvm_disable_host_haltpoll, NULL, 1);
> -	preempt_enable();
> +	smp_call_function_single(cpu, kvm_disable_host_haltpoll, NULL, 1);
>  }
>  EXPORT_SYMBOL_GPL(arch_haltpoll_enable);
>  
> -void arch_haltpoll_disable(void)
> +void arch_haltpoll_disable(unsigned int cpu)
>  {
>  	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL))
>  		return;
>  
> -	preempt_disable();
>  	/* Enable guest halt poll disables host halt poll */
> -	kvm_enable_host_haltpoll(NULL);
> -	smp_call_function(kvm_enable_host_haltpoll, NULL, 1);
> -	preempt_enable();
> +	smp_call_function_single(cpu, kvm_enable_host_haltpoll, NULL, 1);
>  }
>  EXPORT_SYMBOL_GPL(arch_haltpoll_disable);
>  #endif
> diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c
> index 9ac093dcbb01..8baade23f8d0 100644
> --- a/drivers/cpuidle/cpuidle-haltpoll.c
> +++ b/drivers/cpuidle/cpuidle-haltpoll.c
> @@ -11,12 +11,15 @@
>   */
>  
>  #include <linux/init.h>
> +#include <linux/cpu.h>
>  #include <linux/cpuidle.h>
>  #include <linux/module.h>
>  #include <linux/sched/idle.h>
>  #include <linux/kvm_para.h>
>  #include <linux/cpuidle_haltpoll.h>
>  
> +static struct cpuidle_device __percpu *haltpoll_cpuidle_devices;
> +
>  static int default_enter_idle(struct cpuidle_device *dev,
>  			      struct cpuidle_driver *drv, int index)
>  {
> @@ -46,6 +49,48 @@ static struct cpuidle_driver haltpoll_driver = {
>  	.state_count = 2,
>  };
>  
> +static int haltpoll_cpu_online(unsigned int cpu)
> +{
> +	struct cpuidle_device *dev;
> +
> +	dev = per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
> +	if (!dev->registered) {
> +		dev->cpu = cpu;
> +		if (cpuidle_register_device(dev)) {
> +			pr_notice("cpuidle_register_device %d failed!\n", cpu);
> +			return -EIO;
> +		}
> +		arch_haltpoll_enable(cpu);
> +	}
> +
> +	return 0;
> +}
> +
> +static void haltpoll_uninit(void)
> +{
> +	unsigned int cpu;
> +
> +	cpus_read_lock();
> +
> +	for_each_online_cpu(cpu) {
> +		struct cpuidle_device *dev =
> +			per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
> +
> +		if (!dev->registered)
> +			continue;
> +
> +		arch_haltpoll_disable(cpu);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	cpus_read_unlock();
> +
> +	cpuidle_unregister_driver(&haltpoll_driver);
> +
> +	free_percpu(haltpoll_cpuidle_devices);
> +	haltpoll_cpuidle_devices = NULL;
> +}
> +
>  static int __init haltpoll_init(void)
>  {
>  	int ret;
> @@ -56,17 +101,29 @@ static int __init haltpoll_init(void)
>  	if (!kvm_para_available())
>  		return 0;
>  
> -	ret = cpuidle_register(&haltpoll_driver, NULL);
> -	if (ret == 0)
> -		arch_haltpoll_enable();
> +	ret = cpuidle_register_driver(drv);
> +	if (ret < 0)
> +		return ret;
> +
> +	haltpoll_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (haltpoll_cpuidle_devices == NULL) {
> +		cpuidle_unregister_driver(drv);
> +		return -ENOMEM;
> +	}
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/haltpoll:online",
> +				haltpoll_cpu_online, NULL);
> +	if (ret < 0)
> +		haltpoll_uninit();
> +	else
> +		ret = 0;
>  
>  	return ret;
>  }
>  
>  static void __exit haltpoll_exit(void)
>  {
> -	arch_haltpoll_disable();
> -	cpuidle_unregister(&haltpoll_driver);
> +	haltpoll_uninit();
>  }
>  
>  module_init(haltpoll_init);
> diff --git a/include/linux/cpuidle_haltpoll.h b/include/linux/cpuidle_haltpoll.h
> index fe5954c2409e..d50c1e0411a2 100644
> --- a/include/linux/cpuidle_haltpoll.h
> +++ b/include/linux/cpuidle_haltpoll.h
> @@ -5,11 +5,11 @@
>  #ifdef CONFIG_ARCH_CPUIDLE_HALTPOLL
>  #include <asm/cpuidle_haltpoll.h>
>  #else
> -static inline void arch_haltpoll_enable(void)
> +static inline void arch_haltpoll_enable(unsigned int cpu)
>  {
>  }
>  
> -static inline void arch_haltpoll_disable(void)
> +static inline void arch_haltpoll_disable(unsigned int cpu)
>  {
>  }
>  #endif
> -- 
> 2.17.1

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Joao Martins]

On 8/29/19 4:10 PM, Joao Martins wrote:
> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
> past the online ones and thus fail to register the idle driver.
> This is because cpuidle_add_sysfs() will return with -ENODEV as a
> consequence from get_cpu_device() return no device for a non-existing
> CPU.
> 
> Instead switch to cpuidle_register_driver() and manually register each
> of the present cpus through cpuhp_setup_state() callback and future
> ones that get onlined. This mimmics similar logic that intel_idle does.
> 
> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---

While testing the above, I found out another issue on the haltpoll series.
But I am not sure what is best suited to cpuidle framework, hence requesting
some advise if below is a reasonable solution or something else is preferred.

Essentially after haltpoll governor got introduced and regardless of the cpuidle
driver the default governor is gonna be haltpoll for a guest (given haltpoll
governor doesn't get registered for baremetal). Right now, for a KVM guest, the
idle governors have these ratings:

 * ladder            -> 10
 * teo               -> 19
 * menu              -> 20
 * haltpoll          -> 21
 * ladder + nohz=off -> 25

When a guest is booted with MWAIT and intel_idle is probed and sucessfully
registered, we will end up with a haltpoll governor being used as opposed to
'menu' (which used to be the default case). This would prevent IIUC that other
C-states get used other than poll_state (state 0) and state 1.

Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
it doesn't look reasonable to be the default? What about using haltpoll governor
as default when haltpoll idle driver registers or modloads.

My idea to achieve the above would be to decrease the rating to 9 (before the
lowest rated governor) and retain old defaults before haltpoll. Then we would
allow a cpuidle driver to define a preferred governor to switch on idle driver
registration. Naturally all of would be ignored if overidden by
cpuidle.governor=.

The diff below the scissors line is an example of that.

Thoughts?

---------------------------------- >8 --------------------------------

From: Joao Martins <joao.m.martins@oracle.com>
Subject: [PATCH] cpuidle: switch to prefered governor on registration

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/cpuidle/cpuidle-haltpoll.c   |  1 +
 drivers/cpuidle/cpuidle.h            |  1 +
 drivers/cpuidle/driver.c             | 26 ++++++++++++++++++++++++++
 drivers/cpuidle/governor.c           |  6 +++---
 drivers/cpuidle/governors/haltpoll.c |  2 +-
 include/linux/cpuidle.h              |  3 +++
 6 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c
index 8baade23f8d0..88a38c3c35e4 100644
--- a/drivers/cpuidle/cpuidle-haltpoll.c
+++ b/drivers/cpuidle/cpuidle-haltpoll.c
@@ -33,6 +33,7 @@ static int default_enter_idle(struct cpuidle_device *dev,

 static struct cpuidle_driver haltpoll_driver = {
 	.name = "haltpoll",
+	.governor = "haltpoll",
 	.owner = THIS_MODULE,
 	.states = {
 		{ /* entry 0 is for polling */ },
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index d6613101af92..c046f49c1920 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -22,6 +22,7 @@ extern void cpuidle_install_idle_handler(void);
 extern void cpuidle_uninstall_idle_handler(void);

 /* governors */
+extern struct cpuidle_governor *cpuidle_find_governor(const char *str);
 extern int cpuidle_switch_governor(struct cpuidle_governor *gov);

 /* sysfs */
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index dc32f34e68d9..8b8b9d89ce58 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -87,6 +87,7 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 #else

 static struct cpuidle_driver *cpuidle_curr_driver;
+static struct cpuidle_governor *cpuidle_default_governor = NULL;

 /**
  * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
@@ -254,12 +255,25 @@ static void __cpuidle_unregister_driver(struct
cpuidle_driver *drv)
  */
 int cpuidle_register_driver(struct cpuidle_driver *drv)
 {
+	struct cpuidle_governor *gov;
 	int ret;

 	spin_lock(&cpuidle_driver_lock);
 	ret = __cpuidle_register_driver(drv);
 	spin_unlock(&cpuidle_driver_lock);

+	if (!ret && !strlen(param_governor) && drv->governor &&
+	    (cpuidle_get_driver() == drv)) {
+		mutex_lock(&cpuidle_lock);
+		gov = cpuidle_find_governor(drv->governor);
+		if (gov) {
+			cpuidle_default_governor = cpuidle_curr_governor;
+			if (cpuidle_switch_governor(gov) < 0)
+				cpuidle_default_governor = NULL;
+		}
+		mutex_unlock(&cpuidle_lock);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpuidle_register_driver);
@@ -274,9 +288,21 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
  */
 void cpuidle_unregister_driver(struct cpuidle_driver *drv)
 {
+	bool enabled = (cpuidle_get_driver() == drv);
+
 	spin_lock(&cpuidle_driver_lock);
 	__cpuidle_unregister_driver(drv);
 	spin_unlock(&cpuidle_driver_lock);
+
+	if (!enabled)
+		return;
+
+	mutex_lock(&cpuidle_lock);
+	if (cpuidle_default_governor) {
+		if (!cpuidle_switch_governor(cpuidle_default_governor))
+			cpuidle_default_governor = NULL;
+	}
+	mutex_unlock(&cpuidle_lock);
 }
 EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);

diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index 2e3e14192bee..e93c11dc8304 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -22,12 +22,12 @@ LIST_HEAD(cpuidle_governors);
 struct cpuidle_governor *cpuidle_curr_governor;

 /**
- * __cpuidle_find_governor - finds a governor of the specified name
+ * cpuidle_find_governor - finds a governor of the specified name
  * @str: the name
  *
  * Must be called with cpuidle_lock acquired.
  */
-static struct cpuidle_governor * __cpuidle_find_governor(const char *str)
+struct cpuidle_governor * cpuidle_find_governor(const char *str)
 {
 	struct cpuidle_governor *gov;

@@ -87,7 +87,7 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
 		return -ENODEV;

 	mutex_lock(&cpuidle_lock);
-	if (__cpuidle_find_governor(gov->name) == NULL) {
+	if (cpuidle_find_governor(gov->name) == NULL) {
 		ret = 0;
 		list_add_tail(&gov->governor_list, &cpuidle_governors);
 		if (!cpuidle_curr_governor ||
diff --git a/drivers/cpuidle/governors/haltpoll.c
b/drivers/cpuidle/governors/haltpoll.c
index 797477bda486..7a703d2e0064 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -133,7 +133,7 @@ static int haltpoll_enable_device(struct cpuidle_driver *drv,

 static struct cpuidle_governor haltpoll_governor = {
 	.name =			"haltpoll",
-	.rating =		21,
+	.rating =		9,
 	.enable =		haltpoll_enable_device,
 	.select =		haltpoll_select,
 	.reflect =		haltpoll_reflect,
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 1a9f54eb3aa1..2dc4c6b19c25 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -121,6 +121,9 @@ struct cpuidle_driver {

 	/* the driver handles the cpus in cpumask */
 	struct cpumask		*cpumask;
+
+	/* preferred governor to switch at register time */
+	const char		*governor;
 };

 #ifdef CONFIG_CPU_IDLE
-- 
2.17.1

Re: Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Marcelo Tosatti]

On Thu, Aug 29, 2019 at 06:16:05PM +0100, Joao Martins wrote:
> On 8/29/19 4:10 PM, Joao Martins wrote:
> > When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
> > past the online ones and thus fail to register the idle driver.
> > This is because cpuidle_add_sysfs() will return with -ENODEV as a
> > consequence from get_cpu_device() return no device for a non-existing
> > CPU.
> > 
> > Instead switch to cpuidle_register_driver() and manually register each
> > of the present cpus through cpuhp_setup_state() callback and future
> > ones that get onlined. This mimmics similar logic that intel_idle does.
> > 
> > Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > ---
> 
> While testing the above, I found out another issue on the haltpoll series.
> But I am not sure what is best suited to cpuidle framework, hence requesting
> some advise if below is a reasonable solution or something else is preferred.
> 
> Essentially after haltpoll governor got introduced and regardless of the cpuidle
> driver the default governor is gonna be haltpoll for a guest (given haltpoll
> governor doesn't get registered for baremetal).

Right.

> Right now, for a KVM guest, the
> idle governors have these ratings:
> 
>  * ladder            -> 10
>  * teo               -> 19
>  * menu              -> 20
>  * haltpoll          -> 21
>  * ladder + nohz=off -> 25

Yes. PowerPC KVM guests crash currently due to the use of the haltpoll
governor (have a patch in my queue to fix this, but your solution
embraces more cases).

> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
> registered, we will end up with a haltpoll governor being used as opposed to
> 'menu' (which used to be the default case). This would prevent IIUC that other
> C-states get used other than poll_state (state 0) and state 1.
> 
> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
> it doesn't look reasonable to be the default? What about using haltpoll governor
> as default when haltpoll idle driver registers or modloads.
> 
> My idea to achieve the above would be to decrease the rating to 9 (before the
> lowest rated governor) and retain old defaults before haltpoll. Then we would
> allow a cpuidle driver to define a preferred governor to switch on idle driver
> registration. Naturally all of would be ignored if overidden by
> cpuidle.governor=.
> 
> The diff below the scissors line is an example of that.
> 
> Thoughts?

Works for me. Rafael?

> 
> ---------------------------------- >8 --------------------------------
> 
> From: Joao Martins <joao.m.martins@oracle.com>
> Subject: [PATCH] cpuidle: switch to prefered governor on registration
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  drivers/cpuidle/cpuidle-haltpoll.c   |  1 +
>  drivers/cpuidle/cpuidle.h            |  1 +
>  drivers/cpuidle/driver.c             | 26 ++++++++++++++++++++++++++
>  drivers/cpuidle/governor.c           |  6 +++---
>  drivers/cpuidle/governors/haltpoll.c |  2 +-
>  include/linux/cpuidle.h              |  3 +++
>  6 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c
> index 8baade23f8d0..88a38c3c35e4 100644
> --- a/drivers/cpuidle/cpuidle-haltpoll.c
> +++ b/drivers/cpuidle/cpuidle-haltpoll.c
> @@ -33,6 +33,7 @@ static int default_enter_idle(struct cpuidle_device *dev,
> 
>  static struct cpuidle_driver haltpoll_driver = {
>  	.name = "haltpoll",
> +	.governor = "haltpoll",
>  	.owner = THIS_MODULE,
>  	.states = {
>  		{ /* entry 0 is for polling */ },
> diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
> index d6613101af92..c046f49c1920 100644
> --- a/drivers/cpuidle/cpuidle.h
> +++ b/drivers/cpuidle/cpuidle.h
> @@ -22,6 +22,7 @@ extern void cpuidle_install_idle_handler(void);
>  extern void cpuidle_uninstall_idle_handler(void);
> 
>  /* governors */
> +extern struct cpuidle_governor *cpuidle_find_governor(const char *str);
>  extern int cpuidle_switch_governor(struct cpuidle_governor *gov);
> 
>  /* sysfs */
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index dc32f34e68d9..8b8b9d89ce58 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -87,6 +87,7 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  #else
> 
>  static struct cpuidle_driver *cpuidle_curr_driver;
> +static struct cpuidle_governor *cpuidle_default_governor = NULL;
> 
>  /**
>   * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
> @@ -254,12 +255,25 @@ static void __cpuidle_unregister_driver(struct
> cpuidle_driver *drv)
>   */
>  int cpuidle_register_driver(struct cpuidle_driver *drv)
>  {
> +	struct cpuidle_governor *gov;
>  	int ret;
> 
>  	spin_lock(&cpuidle_driver_lock);
>  	ret = __cpuidle_register_driver(drv);
>  	spin_unlock(&cpuidle_driver_lock);
> 
> +	if (!ret && !strlen(param_governor) && drv->governor &&
> +	    (cpuidle_get_driver() == drv)) {
> +		mutex_lock(&cpuidle_lock);
> +		gov = cpuidle_find_governor(drv->governor);
> +		if (gov) {
> +			cpuidle_default_governor = cpuidle_curr_governor;
> +			if (cpuidle_switch_governor(gov) < 0)
> +				cpuidle_default_governor = NULL;
> +		}
> +		mutex_unlock(&cpuidle_lock);
> +	}
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cpuidle_register_driver);
> @@ -274,9 +288,21 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
>   */
>  void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  {
> +	bool enabled = (cpuidle_get_driver() == drv);
> +
>  	spin_lock(&cpuidle_driver_lock);
>  	__cpuidle_unregister_driver(drv);
>  	spin_unlock(&cpuidle_driver_lock);
> +
> +	if (!enabled)
> +		return;
> +
> +	mutex_lock(&cpuidle_lock);
> +	if (cpuidle_default_governor) {
> +		if (!cpuidle_switch_governor(cpuidle_default_governor))
> +			cpuidle_default_governor = NULL;
> +	}
> +	mutex_unlock(&cpuidle_lock);
>  }
>  EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
> 
> diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
> index 2e3e14192bee..e93c11dc8304 100644
> --- a/drivers/cpuidle/governor.c
> +++ b/drivers/cpuidle/governor.c
> @@ -22,12 +22,12 @@ LIST_HEAD(cpuidle_governors);
>  struct cpuidle_governor *cpuidle_curr_governor;
> 
>  /**
> - * __cpuidle_find_governor - finds a governor of the specified name
> + * cpuidle_find_governor - finds a governor of the specified name
>   * @str: the name
>   *
>   * Must be called with cpuidle_lock acquired.
>   */
> -static struct cpuidle_governor * __cpuidle_find_governor(const char *str)
> +struct cpuidle_governor * cpuidle_find_governor(const char *str)
>  {
>  	struct cpuidle_governor *gov;
> 
> @@ -87,7 +87,7 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
>  		return -ENODEV;
> 
>  	mutex_lock(&cpuidle_lock);
> -	if (__cpuidle_find_governor(gov->name) == NULL) {
> +	if (cpuidle_find_governor(gov->name) == NULL) {
>  		ret = 0;
>  		list_add_tail(&gov->governor_list, &cpuidle_governors);
>  		if (!cpuidle_curr_governor ||
> diff --git a/drivers/cpuidle/governors/haltpoll.c
> b/drivers/cpuidle/governors/haltpoll.c
> index 797477bda486..7a703d2e0064 100644
> --- a/drivers/cpuidle/governors/haltpoll.c
> +++ b/drivers/cpuidle/governors/haltpoll.c
> @@ -133,7 +133,7 @@ static int haltpoll_enable_device(struct cpuidle_driver *drv,
> 
>  static struct cpuidle_governor haltpoll_governor = {
>  	.name =			"haltpoll",
> -	.rating =		21,
> +	.rating =		9,
>  	.enable =		haltpoll_enable_device,
>  	.select =		haltpoll_select,
>  	.reflect =		haltpoll_reflect,
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 1a9f54eb3aa1..2dc4c6b19c25 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -121,6 +121,9 @@ struct cpuidle_driver {
> 
>  	/* the driver handles the cpus in cpumask */
>  	struct cpumask		*cpumask;
> +
> +	/* preferred governor to switch at register time */
> +	const char		*governor;
>  };
> 
>  #ifdef CONFIG_CPU_IDLE
> -- 
> 2.17.1

Re: Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Daniel Lezcano]

On 29/08/2019 19:16, Joao Martins wrote:
> On 8/29/19 4:10 PM, Joao Martins wrote:
>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>> past the online ones and thus fail to register the idle driver.
>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>> consequence from get_cpu_device() return no device for a non-existing
>> CPU.
>>
>> Instead switch to cpuidle_register_driver() and manually register each
>> of the present cpus through cpuhp_setup_state() callback and future
>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>
>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
> 
> While testing the above, I found out another issue on the haltpoll series.
> But I am not sure what is best suited to cpuidle framework, hence requesting
> some advise if below is a reasonable solution or something else is preferred.
> 
> Essentially after haltpoll governor got introduced and regardless of the cpuidle
> driver the default governor is gonna be haltpoll for a guest (given haltpoll
> governor doesn't get registered for baremetal). Right now, for a KVM guest, the
> idle governors have these ratings:
> 
>  * ladder            -> 10
>  * teo               -> 19
>  * menu              -> 20
>  * haltpoll          -> 21
>  * ladder + nohz=off -> 25
> 
> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
> registered, we will end up with a haltpoll governor being used as opposed to
> 'menu' (which used to be the default case). This would prevent IIUC that other
> C-states get used other than poll_state (state 0) and state 1.
> 
> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
> it doesn't look reasonable to be the default? What about using haltpoll governor
> as default when haltpoll idle driver registers or modload.

Are the guest and host kernel the same? IOW compiled with the same
kernel config?


> My idea to achieve the above would be to decrease the rating to 9 (before the
> lowest rated governor) and retain old defaults before haltpoll. Then we would
> allow a cpuidle driver to define a preferred governor to switch on idle driver
> registration. Naturally all of would be ignored if overidden by
> cpuidle.governor=.
> 




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Joao Martins]

On 8/29/19 6:42 PM, Daniel Lezcano wrote:
> On 29/08/2019 19:16, Joao Martins wrote:
>> On 8/29/19 4:10 PM, Joao Martins wrote:
>>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>>> past the online ones and thus fail to register the idle driver.
>>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>>> consequence from get_cpu_device() return no device for a non-existing
>>> CPU.
>>>
>>> Instead switch to cpuidle_register_driver() and manually register each
>>> of the present cpus through cpuhp_setup_state() callback and future
>>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>>
>>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> ---
>>
>> While testing the above, I found out another issue on the haltpoll series.
>> But I am not sure what is best suited to cpuidle framework, hence requesting
>> some advise if below is a reasonable solution or something else is preferred.
>>
>> Essentially after haltpoll governor got introduced and regardless of the cpuidle
>> driver the default governor is gonna be haltpoll for a guest (given haltpoll
>> governor doesn't get registered for baremetal). Right now, for a KVM guest, the
>> idle governors have these ratings:
>>
>>  * ladder            -> 10
>>  * teo               -> 19
>>  * menu              -> 20
>>  * haltpoll          -> 21
>>  * ladder + nohz=off -> 25
>>
>> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
>> registered, we will end up with a haltpoll governor being used as opposed to
>> 'menu' (which used to be the default case). This would prevent IIUC that other
>> C-states get used other than poll_state (state 0) and state 1.
>>
>> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
>> it doesn't look reasonable to be the default? What about using haltpoll governor
>> as default when haltpoll idle driver registers or modload.
> 
> Are the guest and host kernel the same? IOW compiled with the same
> kernel config?
> 
You just need to toggle this (regardless off CONFIG_HALTPOLL_CPUIDLE):

	CONFIG_CPU_IDLE_GOV_HALTPOLL=y

And *if you are a KVM guest* it will be the default (unless using nohz=off in
which case ladder gets the highest rating -- see the listing right above).

Host will just behave differently because the haltpoll governor is checking if
it is running as kvm guest, and only registering in that case.

> 
>> My idea to achieve the above would be to decrease the rating to 9 (before the
>> lowest rated governor) and retain old defaults before haltpoll. Then we would
>> allow a cpuidle driver to define a preferred governor to switch on idle driver
>> registration. Naturally all of would be ignored if overidden by
>> cpuidle.governor=.
>>
> 
> 
> 
> 

Re: Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Daniel Lezcano]

On 29/08/2019 20:07, Joao Martins wrote:
> On 8/29/19 6:42 PM, Daniel Lezcano wrote:
>> On 29/08/2019 19:16, Joao Martins wrote:
>>> On 8/29/19 4:10 PM, Joao Martins wrote:
>>>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>>>> past the online ones and thus fail to register the idle driver.
>>>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>>>> consequence from get_cpu_device() return no device for a non-existing
>>>> CPU.
>>>>
>>>> Instead switch to cpuidle_register_driver() and manually register each
>>>> of the present cpus through cpuhp_setup_state() callback and future
>>>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>>>
>>>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> ---
>>>
>>> While testing the above, I found out another issue on the haltpoll series.
>>> But I am not sure what is best suited to cpuidle framework, hence requesting
>>> some advise if below is a reasonable solution or something else is preferred.
>>>
>>> Essentially after haltpoll governor got introduced and regardless of the cpuidle
>>> driver the default governor is gonna be haltpoll for a guest (given haltpoll
>>> governor doesn't get registered for baremetal). Right now, for a KVM guest, the
>>> idle governors have these ratings:
>>>
>>>  * ladder            -> 10
>>>  * teo               -> 19
>>>  * menu              -> 20
>>>  * haltpoll          -> 21
>>>  * ladder + nohz=off -> 25
>>>
>>> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
>>> registered, we will end up with a haltpoll governor being used as opposed to
>>> 'menu' (which used to be the default case). This would prevent IIUC that other
>>> C-states get used other than poll_state (state 0) and state 1.
>>>
>>> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
>>> it doesn't look reasonable to be the default? What about using haltpoll governor
>>> as default when haltpoll idle driver registers or modload.
>>
>> Are the guest and host kernel the same? IOW compiled with the same
>> kernel config?
>>
> You just need to toggle this (regardless off CONFIG_HALTPOLL_CPUIDLE):
> 
> 	CONFIG_CPU_IDLE_GOV_HALTPOLL=y
> 
> And *if you are a KVM guest* it will be the default (unless using nohz=off in
> which case ladder gets the highest rating -- see the listing right above).
> 
> Host will just behave differently because the haltpoll governor is checking if
> it is running as kvm guest, and only registering in that case.

I understood the problem. Actually my question was about if the kernels
are compiled for host and guest, and can be run indifferently. In this
case a runtime detection must be done as you propose, otherwise that can
be done at config time. I pretty sure it is the former but before
thinking about the runtime side, I wanted to double check.


>>> My idea to achieve the above would be to decrease the rating to 9 (before the
>>> lowest rated governor) and retain old defaults before haltpoll. Then we would
>>> allow a cpuidle driver to define a preferred governor to switch on idle driver
>>> registration. Naturally all of would be ignored if overidden by
>>> cpuidle.governor=.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Default governor regardless of cpuidle driver

[Joao Martins]

On 8/29/19 7:28 PM, Daniel Lezcano wrote:
> On 29/08/2019 20:07, Joao Martins wrote:
>> On 8/29/19 6:42 PM, Daniel Lezcano wrote:
>>> On 29/08/2019 19:16, Joao Martins wrote:
>>>> On 8/29/19 4:10 PM, Joao Martins wrote:
>>>>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>>>>> past the online ones and thus fail to register the idle driver.
>>>>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>>>>> consequence from get_cpu_device() return no device for a non-existing
>>>>> CPU.
>>>>>
>>>>> Instead switch to cpuidle_register_driver() and manually register each
>>>>> of the present cpus through cpuhp_setup_state() callback and future
>>>>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>>>>
>>>>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> ---
>>>>
>>>> While testing the above, I found out another issue on the haltpoll series.
>>>> But I am not sure what is best suited to cpuidle framework, hence requesting
>>>> some advise if below is a reasonable solution or something else is preferred.
>>>>
>>>> Essentially after haltpoll governor got introduced and regardless of the cpuidle
>>>> driver the default governor is gonna be haltpoll for a guest (given haltpoll
>>>> governor doesn't get registered for baremetal). Right now, for a KVM guest, the
>>>> idle governors have these ratings:
>>>>
>>>>  * ladder            -> 10
>>>>  * teo               -> 19
>>>>  * menu              -> 20
>>>>  * haltpoll          -> 21
>>>>  * ladder + nohz=off -> 25
>>>>
>>>> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
>>>> registered, we will end up with a haltpoll governor being used as opposed to
>>>> 'menu' (which used to be the default case). This would prevent IIUC that other
>>>> C-states get used other than poll_state (state 0) and state 1.
>>>>
>>>> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
>>>> it doesn't look reasonable to be the default? What about using haltpoll governor
>>>> as default when haltpoll idle driver registers or modload.
>>>
>>> Are the guest and host kernel the same? IOW compiled with the same
>>> kernel config?
>>>
>> You just need to toggle this (regardless off CONFIG_HALTPOLL_CPUIDLE):
>>
>> 	CONFIG_CPU_IDLE_GOV_HALTPOLL=y
>>
>> And *if you are a KVM guest* it will be the default (unless using nohz=off in
>> which case ladder gets the highest rating -- see the listing right above).
>>
>> Host will just behave differently because the haltpoll governor is checking if
>> it is running as kvm guest, and only registering in that case.
> 
> I understood the problem. Actually my question was about if the kernels
> are compiled for host and guest, and can be run indifferently. 

/nods Correct.

> In this
> case a runtime detection must be done as you propose, otherwise that can
> be done at config time. I pretty sure it is the former but before
> thinking about the runtime side, I wanted to double check.
> 
Hmm, but even with separate kernels/configs for guest and host I think we would
still have the same issue.

What I was trying to convey is that even when running with a config solely for
KVM guests (that is different than baremetal) you can have today various ways of
idling. An Intel x86 kvm guest can have no idle driver (but arch-specific),
intel_idle (like baremetal config) and haltpoll. There are usecases for these
three, and makes sense to consolidate all.

Say you wanted to have a kvm specific config, you would still see the same
problem if you happen to compile intel_idle together with haltpoll
driver+governor. Creating two separate configs here, with and without haltpoll
for VMs doesn't sound effective for distros. Perhaps decreasing the rating of
haltpoll governor, but while a short term fix it wouldn't give much sensible
defaults without the one-off runtime switch.

Unless ofc I am missing something.

> 
>>>> My idea to achieve the above would be to decrease the rating to 9 (before the
>>>> lowest rated governor) and retain old defaults before haltpoll. Then we would
>>>> allow a cpuidle driver to define a preferred governor to switch on idle driver
>>>> registration. Naturally all of would be ignored if overidden by
>>>> cpuidle.governor=.
> 
> 

Re: Default governor regardless of cpuidle driver

[Daniel Lezcano]

On 29/08/2019 21:11, Joao Martins wrote:
> On 8/29/19 7:28 PM, Daniel Lezcano wrote:
>> On 29/08/2019 20:07, Joao Martins wrote:
>>> On 8/29/19 6:42 PM, Daniel Lezcano wrote:
>>>> On 29/08/2019 19:16, Joao Martins wrote:
>>>>> On 8/29/19 4:10 PM, Joao Martins wrote:
>>>>>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>>>>>> past the online ones and thus fail to register the idle driver.
>>>>>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>>>>>> consequence from get_cpu_device() return no device for a non-existing
>>>>>> CPU.
>>>>>>
>>>>>> Instead switch to cpuidle_register_driver() and manually register each
>>>>>> of the present cpus through cpuhp_setup_state() callback and future
>>>>>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>>>>>
>>>>>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>> ---
>>>>>
>>>>> While testing the above, I found out another issue on the haltpoll series.
>>>>> But I am not sure what is best suited to cpuidle framework, hence requesting
>>>>> some advise if below is a reasonable solution or something else is preferred.
>>>>>
>>>>> Essentially after haltpoll governor got introduced and regardless of the cpuidle
>>>>> driver the default governor is gonna be haltpoll for a guest (given haltpoll
>>>>> governor doesn't get registered for baremetal). Right now, for a KVM guest, the
>>>>> idle governors have these ratings:
>>>>>
>>>>>  * ladder            -> 10
>>>>>  * teo               -> 19
>>>>>  * menu              -> 20
>>>>>  * haltpoll          -> 21
>>>>>  * ladder + nohz=off -> 25
>>>>>
>>>>> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
>>>>> registered, we will end up with a haltpoll governor being used as opposed to
>>>>> 'menu' (which used to be the default case). This would prevent IIUC that other
>>>>> C-states get used other than poll_state (state 0) and state 1.
>>>>>
>>>>> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
>>>>> it doesn't look reasonable to be the default? What about using haltpoll governor
>>>>> as default when haltpoll idle driver registers or modload.
>>>>
>>>> Are the guest and host kernel the same? IOW compiled with the same
>>>> kernel config?
>>>>
>>> You just need to toggle this (regardless off CONFIG_HALTPOLL_CPUIDLE):
>>>
>>> 	CONFIG_CPU_IDLE_GOV_HALTPOLL=y
>>>
>>> And *if you are a KVM guest* it will be the default (unless using nohz=off in
>>> which case ladder gets the highest rating -- see the listing right above).
>>>
>>> Host will just behave differently because the haltpoll governor is checking if
>>> it is running as kvm guest, and only registering in that case.
>>
>> I understood the problem. Actually my question was about if the kernels
>> are compiled for host and guest, and can be run indifferently. 
> 
> /nods Correct.
> 
>> In this
>> case a runtime detection must be done as you propose, otherwise that can
>> be done at config time. I pretty sure it is the former but before
>> thinking about the runtime side, I wanted to double check.
>>
> Hmm, but even with separate kernels/configs for guest and host I think we would
> still have the same issue.
> 
> What I was trying to convey is that even when running with a config solely for
> KVM guests (that is different than baremetal) you can have today various ways of
> idling. An Intel x86 kvm guest can have no idle driver (but arch-specific),
> intel_idle (like baremetal config) and haltpoll. There are usecases for these
> three, and makes sense to consolidate all.
> 
> Say you wanted to have a kvm specific config, you would still see the same
> problem if you happen to compile intel_idle together with haltpoll
> driver+governor. 

Can a guest work with an intel_idle driver?

> Creating two separate configs here, with and without haltpoll
> for VMs doesn't sound effective for distros.

Agree

> Perhaps decreasing the rating of
> haltpoll governor, but while a short term fix it wouldn't give much sensible
> defaults without the one-off runtime switch.




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Default governor regardless of cpuidle driver

[Joao Martins]

On 8/29/19 9:22 PM, Daniel Lezcano wrote:
> On 29/08/2019 21:11, Joao Martins wrote:
>> On 8/29/19 7:28 PM, Daniel Lezcano wrote:
>>> On 29/08/2019 20:07, Joao Martins wrote:
>>>> On 8/29/19 6:42 PM, Daniel Lezcano wrote:
>>>>> On 29/08/2019 19:16, Joao Martins wrote:
>>>>>> On 8/29/19 4:10 PM, Joao Martins wrote:
>>>>>>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>>>>>>> past the online ones and thus fail to register the idle driver.
>>>>>>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>>>>>>> consequence from get_cpu_device() return no device for a non-existing
>>>>>>> CPU.
>>>>>>>
>>>>>>> Instead switch to cpuidle_register_driver() and manually register each
>>>>>>> of the present cpus through cpuhp_setup_state() callback and future
>>>>>>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>>>>>>
>>>>>>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>>>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>>> ---
>>>>>>
>>>>>> While testing the above, I found out another issue on the haltpoll series.
>>>>>> But I am not sure what is best suited to cpuidle framework, hence requesting
>>>>>> some advise if below is a reasonable solution or something else is preferred.
>>>>>>
>>>>>> Essentially after haltpoll governor got introduced and regardless of the cpuidle
>>>>>> driver the default governor is gonna be haltpoll for a guest (given haltpoll
>>>>>> governor doesn't get registered for baremetal). Right now, for a KVM guest, the
>>>>>> idle governors have these ratings:
>>>>>>
>>>>>>  * ladder            -> 10
>>>>>>  * teo               -> 19
>>>>>>  * menu              -> 20
>>>>>>  * haltpoll          -> 21
>>>>>>  * ladder + nohz=off -> 25
>>>>>>
>>>>>> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
>>>>>> registered, we will end up with a haltpoll governor being used as opposed to
>>>>>> 'menu' (which used to be the default case). This would prevent IIUC that other
>>>>>> C-states get used other than poll_state (state 0) and state 1.
>>>>>>
>>>>>> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
>>>>>> it doesn't look reasonable to be the default? What about using haltpoll governor
>>>>>> as default when haltpoll idle driver registers or modload.
>>>>>
>>>>> Are the guest and host kernel the same? IOW compiled with the same
>>>>> kernel config?
>>>>>
>>>> You just need to toggle this (regardless off CONFIG_HALTPOLL_CPUIDLE):
>>>>
>>>> 	CONFIG_CPU_IDLE_GOV_HALTPOLL=y
>>>>
>>>> And *if you are a KVM guest* it will be the default (unless using nohz=off in
>>>> which case ladder gets the highest rating -- see the listing right above).
>>>>
>>>> Host will just behave differently because the haltpoll governor is checking if
>>>> it is running as kvm guest, and only registering in that case.
>>>
>>> I understood the problem. Actually my question was about if the kernels
>>> are compiled for host and guest, and can be run indifferently. 
>>
>> /nods Correct.
>>
>>> In this
>>> case a runtime detection must be done as you propose, otherwise that can
>>> be done at config time. I pretty sure it is the former but before
>>> thinking about the runtime side, I wanted to double check.
>>>
>> Hmm, but even with separate kernels/configs for guest and host I think we would
>> still have the same issue.
>>
>> What I was trying to convey is that even when running with a config solely for
>> KVM guests (that is different than baremetal) you can have today various ways of
>> idling. An Intel x86 kvm guest can have no idle driver (but arch-specific),
>> intel_idle (like baremetal config) and haltpoll. There are usecases for these
>> three, and makes sense to consolidate all.
>>
>> Say you wanted to have a kvm specific config, you would still see the same
>> problem if you happen to compile intel_idle together with haltpoll
>> driver+governor. 
> 
> Can a guest work with an intel_idle driver?
> 
Yes.

If you use Qemu you would add '-overcommit cpu-pm=on' to try it out. ofc,
assuming you're on a relatively recent Qemu (v3.0+) and a fairly recent kernel
version as host (v4.17+).

>> Creating two separate configs here, with and without haltpoll
>> for VMs doesn't sound effective for distros.
> 
> Agree
> 
>> Perhaps decreasing the rating of
>> haltpoll governor, but while a short term fix it wouldn't give much sensible
>> defaults without the one-off runtime switch.
> 

Re: Default governor regardless of cpuidle driver

[Daniel Lezcano]

On 29/08/2019 23:12, Joao Martins wrote:

[ ... ]

>>> Say you wanted to have a kvm specific config, you would still see the same
>>> problem if you happen to compile intel_idle together with haltpoll
>>> driver+governor. 
>>
>> Can a guest work with an intel_idle driver?
>>
> Yes.
> 
> If you use Qemu you would add '-overcommit cpu-pm=on' to try it out. ofc,
> assuming you're on a relatively recent Qemu (v3.0+) and a fairly recent kernel
> version as host (v4.17+).

Ok, thanks for the clarification.

>>> Creating two separate configs here, with and without haltpoll
>>> for VMs doesn't sound effective for distros.
>>
>> Agree
>>
>>> Perhaps decreasing the rating of
>>> haltpoll governor, but while a short term fix it wouldn't give much sensible
>>> defaults without the one-off runtime switch.

The rating has little meaning because each governor fits a specific
situation (server, desktop, etc...) and it would probably make sense to
remove it and add a default governor in the config file like the cpufreq.

May be I missed the point from some previous discussion but IMHO the
problem you are facing is coming from the design: there is no need to
create a halt governor but move the code inside the cpuidle-halt driver
instead and ignore the state asked by the governor and return the state
the driver entered.



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Default governor regardless of cpuidle driver

[Joao Martins]

On 8/29/19 10:51 PM, Daniel Lezcano wrote:
> On 29/08/2019 23:12, Joao Martins wrote:
> 
> [ ... ]
> 
>>>> Say you wanted to have a kvm specific config, you would still see the same
>>>> problem if you happen to compile intel_idle together with haltpoll
>>>> driver+governor. 
>>>
>>> Can a guest work with an intel_idle driver?
>>>
>> Yes.
>>
>> If you use Qemu you would add '-overcommit cpu-pm=on' to try it out. ofc,
>> assuming you're on a relatively recent Qemu (v3.0+) and a fairly recent kernel
>> version as host (v4.17+).
> 
> Ok, thanks for the clarification.
> 
>>>> Creating two separate configs here, with and without haltpoll
>>>> for VMs doesn't sound effective for distros.
>>>
>>> Agree
>>>
>>>> Perhaps decreasing the rating of
>>>> haltpoll governor, but while a short term fix it wouldn't give much sensible
>>>> defaults without the one-off runtime switch.
> 
> The rating has little meaning because each governor fits a specific
> situation (server, desktop, etc...) and it would probably make sense to
> remove it and add a default governor in the config file like the cpufreq.
> 
ICYM, I had attached a patch in the first message of this thread [0] right below
the scissors mark. It's not based on config file, but it's the same thing you're
saying (IIUC) but at runtime and thus allowing a driver to state a 'preferred'
governor to switch to at idle registration -- let me know if you think that
looks a sensible approach. Note that the intent of that patch follows the
thinking of leaving all defaults as before haltpoll governor was introduced, but
once user modloads/uses cpuidle-haltpoll this governor then gets switched on.

[0] https://lore.kernel.org/kvm/c8cf8dcc-76a3-3e15-f514-2cb9df1bbbdc@oracle.com/

I would think a config-based preference on a governor would be good *if* one
could actually switch idle governors at runtime like you can with cpufreq -- in
case userspace wants something else other than the default. Right now we can't
do that unless you toggle 'cpuidle_sysfs_switch', or picking one at boot with
'cpuidle.governor='.

> May be I missed the point from some previous discussion but IMHO the
> problem you are facing is coming from the design: there is no need to
> create a halt governor but move the code inside the cpuidle-halt driver
> instead and ignore the state asked by the governor and return the state
> the driver entered.
> 
Marcello's original patch series (first 3 revisions to be exact) actually had
everything in the idle driver, but after some revisions (v4+) Rafael asked him
to split the logic into a governor and unify it with poll state[1].

[1]
https://lore.kernel.org/kvm/CAJZ5v0gPbSXB3r71XaT-4Q7LsiFO_UVymBwOmU8J1W5+COk_1g@mail.gmail.com/

	Joao

Re: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Joao Martins]

On 8/29/19 4:27 PM, Marcelo Tosatti wrote:
> On Thu, Aug 29, 2019 at 04:10:27PM +0100, Joao Martins wrote:
>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>> past the online ones and thus fail to register the idle driver.
>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>> consequence from get_cpu_device() return no device for a non-existing
>> CPU.
>>
>> Instead switch to cpuidle_register_driver() and manually register each
>> of the present cpus through cpuhp_setup_state() callback and future
>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>
>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> v2:
>> * move cpus_read_unlock() right after unregistering all cpuidle_devices;
>> (Marcello Tosatti)
>> * redundant usage of cpuidle_unregister() when only
>> cpuidle_unregister_driver() suffices; (Marcelo Tosatti)
>> * cpuhp_setup_state() returns a state (> 0) on success with CPUHP_AP_ONLINE_DYN
>> thus we set @ret to 0

[ ... ]

> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
Thanks!

Meanwhile upon re-reading cpuhp_setup_state() I found out the teardown/offlining
and haltpoll_uninit() could be a bit simplified. So I sent out a new version[0],
but didn't add your Rb because there's was some very slight functional changes.

[0] https://lore.kernel.org/kvm/20190902104031.9296-1-joao.m.martins@oracle.com/

	Joao

Re: Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Rafael J. Wysocki]

On Thu, Aug 29, 2019 at 7:24 PM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> On Thu, Aug 29, 2019 at 06:16:05PM +0100, Joao Martins wrote:
> > On 8/29/19 4:10 PM, Joao Martins wrote:
> > > When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
> > > past the online ones and thus fail to register the idle driver.
> > > This is because cpuidle_add_sysfs() will return with -ENODEV as a
> > > consequence from get_cpu_device() return no device for a non-existing
> > > CPU.
> > >
> > > Instead switch to cpuidle_register_driver() and manually register each
> > > of the present cpus through cpuhp_setup_state() callback and future
> > > ones that get onlined. This mimmics similar logic that intel_idle does.
> > >
> > > Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > ---
> >
> > While testing the above, I found out another issue on the haltpoll series.
> > But I am not sure what is best suited to cpuidle framework, hence requesting
> > some advise if below is a reasonable solution or something else is preferred.
> >
> > Essentially after haltpoll governor got introduced and regardless of the cpuidle
> > driver the default governor is gonna be haltpoll for a guest (given haltpoll
> > governor doesn't get registered for baremetal).
>
> Right.
>
> > Right now, for a KVM guest, the
> > idle governors have these ratings:
> >
> >  * ladder            -> 10
> >  * teo               -> 19
> >  * menu              -> 20
> >  * haltpoll          -> 21
> >  * ladder + nohz=off -> 25
>
> Yes. PowerPC KVM guests crash currently due to the use of the haltpoll
> governor (have a patch in my queue to fix this, but your solution
> embraces more cases).
>
> > When a guest is booted with MWAIT and intel_idle is probed and sucessfully
> > registered, we will end up with a haltpoll governor being used as opposed to
> > 'menu' (which used to be the default case). This would prevent IIUC that other
> > C-states get used other than poll_state (state 0) and state 1.
> >
> > Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
> > it doesn't look reasonable to be the default? What about using haltpoll governor
> > as default when haltpoll idle driver registers or modloads.
> >
> > My idea to achieve the above would be to decrease the rating to 9 (before the
> > lowest rated governor) and retain old defaults before haltpoll. Then we would
> > allow a cpuidle driver to define a preferred governor to switch on idle driver
> > registration. Naturally all of would be ignored if overidden by
> > cpuidle.governor=.
> >
> > The diff below the scissors line is an example of that.
> >
> > Thoughts?
>
> Works for me. Rafael?

It works for me too, basically, except that I would rename
cpuidle_default_governor in the patch to cpuidle_prev_governor.

Re: Default governor regardless of cpuidle driver

[Rafael J. Wysocki]

On Fri, Aug 30, 2019 at 1:09 PM Joao Martins <joao.m.martins@oracle.com> wrote:
>
> On 8/29/19 10:51 PM, Daniel Lezcano wrote:
> > On 29/08/2019 23:12, Joao Martins wrote:
> >
> > [ ... ]
> >
> >>>> Say you wanted to have a kvm specific config, you would still see the same
> >>>> problem if you happen to compile intel_idle together with haltpoll
> >>>> driver+governor.
> >>>
> >>> Can a guest work with an intel_idle driver?
> >>>
> >> Yes.
> >>
> >> If you use Qemu you would add '-overcommit cpu-pm=on' to try it out. ofc,
> >> assuming you're on a relatively recent Qemu (v3.0+) and a fairly recent kernel
> >> version as host (v4.17+).
> >
> > Ok, thanks for the clarification.
> >
> >>>> Creating two separate configs here, with and without haltpoll
> >>>> for VMs doesn't sound effective for distros.
> >>>
> >>> Agree
> >>>
> >>>> Perhaps decreasing the rating of
> >>>> haltpoll governor, but while a short term fix it wouldn't give much sensible
> >>>> defaults without the one-off runtime switch.
> >
> > The rating has little meaning because each governor fits a specific
> > situation (server, desktop, etc...) and it would probably make sense to
> > remove it and add a default governor in the config file like the cpufreq.
> >
> ICYM, I had attached a patch in the first message of this thread [0] right below
> the scissors mark. It's not based on config file, but it's the same thing you're
> saying (IIUC) but at runtime and thus allowing a driver to state a 'preferred'
> governor to switch to at idle registration -- let me know if you think that
> looks a sensible approach. Note that the intent of that patch follows the
> thinking of leaving all defaults as before haltpoll governor was introduced, but
> once user modloads/uses cpuidle-haltpoll this governor then gets switched on.
>
> [0] https://lore.kernel.org/kvm/c8cf8dcc-76a3-3e15-f514-2cb9df1bbbdc@oracle.com/
>
> I would think a config-based preference on a governor would be good *if* one
> could actually switch idle governors at runtime like you can with cpufreq -- in
> case userspace wants something else other than the default. Right now we can't
> do that unless you toggle 'cpuidle_sysfs_switch', or picking one at boot with
> 'cpuidle.governor='.

FWIW, I've been thinking about getting rid of the cpuidle_sysfs_switch
command line option and always allowing user space to switch cpuidle
governors at run time.  At least I see no reason why that would not
work ATM.

Re: Is: Default governor regardless of cpuidle driver Was: [PATCH v2] cpuidle-haltpoll: vcpu hotplug support

[Joao Martins]

On 9/2/19 10:55 PM, Rafael J. Wysocki wrote:
> On Thu, Aug 29, 2019 at 7:24 PM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>>
>> On Thu, Aug 29, 2019 at 06:16:05PM +0100, Joao Martins wrote:
>>> On 8/29/19 4:10 PM, Joao Martins wrote:
>>>> When cpus != maxcpus cpuidle-haltpoll will fail to register all vcpus
>>>> past the online ones and thus fail to register the idle driver.
>>>> This is because cpuidle_add_sysfs() will return with -ENODEV as a
>>>> consequence from get_cpu_device() return no device for a non-existing
>>>> CPU.
>>>>
>>>> Instead switch to cpuidle_register_driver() and manually register each
>>>> of the present cpus through cpuhp_setup_state() callback and future
>>>> ones that get onlined. This mimmics similar logic that intel_idle does.
>>>>
>>>> Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver")
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> ---
>>>
>>> While testing the above, I found out another issue on the haltpoll series.
>>> But I am not sure what is best suited to cpuidle framework, hence requesting
>>> some advise if below is a reasonable solution or something else is preferred.
>>>
>>> Essentially after haltpoll governor got introduced and regardless of the cpuidle
>>> driver the default governor is gonna be haltpoll for a guest (given haltpoll
>>> governor doesn't get registered for baremetal).
>>
>> Right.
>>
>>> Right now, for a KVM guest, the
>>> idle governors have these ratings:
>>>
>>>  * ladder            -> 10
>>>  * teo               -> 19
>>>  * menu              -> 20
>>>  * haltpoll          -> 21
>>>  * ladder + nohz=off -> 25
>>
>> Yes. PowerPC KVM guests crash currently due to the use of the haltpoll
>> governor (have a patch in my queue to fix this, but your solution
>> embraces more cases).
>>
>>> When a guest is booted with MWAIT and intel_idle is probed and sucessfully
>>> registered, we will end up with a haltpoll governor being used as opposed to
>>> 'menu' (which used to be the default case). This would prevent IIUC that other
>>> C-states get used other than poll_state (state 0) and state 1.
>>>
>>> Given that haltpoll governor is largely only useful with a cpuidle-haltpoll
>>> it doesn't look reasonable to be the default? What about using haltpoll governor
>>> as default when haltpoll idle driver registers or modloads.
>>>
>>> My idea to achieve the above would be to decrease the rating to 9 (before the
>>> lowest rated governor) and retain old defaults before haltpoll. Then we would
>>> allow a cpuidle driver to define a preferred governor to switch on idle driver
>>> registration. Naturally all of would be ignored if overidden by
>>> cpuidle.governor=.
>>>
>>> The diff below the scissors line is an example of that.
>>>
>>> Thoughts?
>>
>> Works for me. Rafael?
> 
> It works for me too, basically, except that I would rename
> cpuidle_default_governor in the patch to cpuidle_prev_governor.
> 

Great! I'll send over a series with this then (splitted accordingly).

Also, In the course of hotplug/hotunplug testing, I found two small issues with
modload/modunload -- regardless of the hotplug patch. So I am gonna add that to
the series too.

	Joao