* [PATCH v3] cpuidle-haltpoll: vcpu hotplug support
@ 2019-09-02 10:40 Joao Martins
2019-09-02 20:34 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Joao Martins @ 2019-09-02 10:40 UTC (permalink / raw)
To: kvm
Cc: Joao Martins, Marcelo Tosatti, linux-kernel, Paolo Bonzini,
Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Rafael J. Wysocki, Daniel Lezcano, linux-pm,
Boris Ostrovsky
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() callbacks and future
ones that get onlined or offlined. 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>
---
v3:
* register the teardown callback for correct handling of hotunplug
and error cases. In case cpuhp_setup_state calls fails (e.g. in one of
the cpus that it invoked the callback) it will then call the teardown of
the previously enabled devices; so no need to handle that manually in
haltpoll_uninit().
* use the cpuhp_setup_state() returned dyn allocated state when it
succeeds. And use that state in haltpoll_unint() to call
cpuhp_remove_state() instead of looping online cpus manually. This
is because cpuhp_remove_state() invokes the teardown/offline callback.
* fix subsystem name to 'cpuidle' instead of 'idle' in cpuhp_setup_state()
v2:
* move cpus_read_unlock() 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) for CPUHP_AP_ONLINE_DYN
---
arch/x86/include/asm/cpuidle_haltpoll.h | 4 +-
arch/x86/kernel/kvm.c | 18 +++----
drivers/cpuidle/cpuidle-haltpoll.c | 68 +++++++++++++++++++++++--
include/linux/cpuidle_haltpoll.h | 4 +-
4 files changed, 73 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..56d8ab814466 100644
--- a/drivers/cpuidle/cpuidle-haltpoll.c
+++ b/drivers/cpuidle/cpuidle-haltpoll.c
@@ -11,12 +11,16 @@
*/
#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 enum cpuhp_state haltpoll_hp_state;
+
static int default_enter_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index)
{
@@ -46,6 +50,46 @@ 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 int haltpoll_cpu_offline(unsigned int cpu)
+{
+ struct cpuidle_device *dev;
+
+ dev = per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
+ if (dev->registered) {
+ arch_haltpoll_disable(cpu);
+ cpuidle_unregister_device(dev);
+ }
+
+ return 0;
+}
+
+static void haltpoll_uninit(void)
+{
+ if (haltpoll_hp_state)
+ cpuhp_remove_state(haltpoll_hp_state);
+ cpuidle_unregister_driver(&haltpoll_driver);
+
+ free_percpu(haltpoll_cpuidle_devices);
+ haltpoll_cpuidle_devices = NULL;
+}
+
static int __init haltpoll_init(void)
{
int ret;
@@ -56,17 +100,31 @@ 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, "cpuidle/haltpoll:online",
+ haltpoll_cpu_online, haltpoll_cpu_offline);
+ if (ret < 0) {
+ haltpoll_uninit();
+ } else {
+ haltpoll_hp_state = ret;
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] cpuidle-haltpoll: vcpu hotplug support
2019-09-02 10:40 [PATCH v3] cpuidle-haltpoll: vcpu hotplug support Joao Martins
@ 2019-09-02 20:34 ` Rafael J. Wysocki
2019-09-02 22:17 ` Marcelo Tosatti
0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2019-09-02 20:34 UTC (permalink / raw)
To: Joao Martins, Marcelo Tosatti, Paolo Bonzini
Cc: kvm-devel, Linux Kernel Mailing List, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Rafael J. Wysocki, Daniel Lezcano, Linux PM,
Boris Ostrovsky
On Mon, Sep 2, 2019 at 12:43 PM Joao Martins <joao.m.martins@oracle.com> 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() callbacks and future
> ones that get onlined or offlined. 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>
> ---
> v3:
> * register the teardown callback for correct handling of hotunplug
> and error cases. In case cpuhp_setup_state calls fails (e.g. in one of
> the cpus that it invoked the callback) it will then call the teardown of
> the previously enabled devices; so no need to handle that manually in
> haltpoll_uninit().
> * use the cpuhp_setup_state() returned dyn allocated state when it
> succeeds. And use that state in haltpoll_unint() to call
> cpuhp_remove_state() instead of looping online cpus manually. This
> is because cpuhp_remove_state() invokes the teardown/offline callback.
> * fix subsystem name to 'cpuidle' instead of 'idle' in cpuhp_setup_state()
Marcelo, is the R-by still applicable?
Paolo, any comments?
>
> v2:
> * move cpus_read_unlock() 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) for CPUHP_AP_ONLINE_DYN
> ---
> arch/x86/include/asm/cpuidle_haltpoll.h | 4 +-
> arch/x86/kernel/kvm.c | 18 +++----
> drivers/cpuidle/cpuidle-haltpoll.c | 68 +++++++++++++++++++++++--
> include/linux/cpuidle_haltpoll.h | 4 +-
> 4 files changed, 73 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..56d8ab814466 100644
> --- a/drivers/cpuidle/cpuidle-haltpoll.c
> +++ b/drivers/cpuidle/cpuidle-haltpoll.c
> @@ -11,12 +11,16 @@
> */
>
> #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 enum cpuhp_state haltpoll_hp_state;
> +
> static int default_enter_idle(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index)
> {
> @@ -46,6 +50,46 @@ 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 int haltpoll_cpu_offline(unsigned int cpu)
> +{
> + struct cpuidle_device *dev;
> +
> + dev = per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
> + if (dev->registered) {
> + arch_haltpoll_disable(cpu);
> + cpuidle_unregister_device(dev);
> + }
> +
> + return 0;
> +}
> +
> +static void haltpoll_uninit(void)
> +{
> + if (haltpoll_hp_state)
> + cpuhp_remove_state(haltpoll_hp_state);
> + cpuidle_unregister_driver(&haltpoll_driver);
> +
> + free_percpu(haltpoll_cpuidle_devices);
> + haltpoll_cpuidle_devices = NULL;
> +}
> +
> static int __init haltpoll_init(void)
> {
> int ret;
> @@ -56,17 +100,31 @@ 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, "cpuidle/haltpoll:online",
> + haltpoll_cpu_online, haltpoll_cpu_offline);
> + if (ret < 0) {
> + haltpoll_uninit();
> + } else {
> + haltpoll_hp_state = ret;
> + 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] cpuidle-haltpoll: vcpu hotplug support
2019-09-02 20:34 ` Rafael J. Wysocki
@ 2019-09-02 22:17 ` Marcelo Tosatti
2019-09-03 7:38 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Marcelo Tosatti @ 2019-09-02 22:17 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joao Martins, Paolo Bonzini, kvm-devel,
Linux Kernel Mailing List, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Rafael J. Wysocki, Daniel Lezcano, Linux PM,
Boris Ostrovsky
On Mon, Sep 02, 2019 at 10:34:07PM +0200, Rafael J. Wysocki wrote:
> On Mon, Sep 2, 2019 at 12:43 PM Joao Martins <joao.m.martins@oracle.com> 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() callbacks and future
> > ones that get onlined or offlined. 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>
> > ---
> > v3:
> > * register the teardown callback for correct handling of hotunplug
> > and error cases. In case cpuhp_setup_state calls fails (e.g. in one of
> > the cpus that it invoked the callback) it will then call the teardown of
> > the previously enabled devices; so no need to handle that manually in
> > haltpoll_uninit().
> > * use the cpuhp_setup_state() returned dyn allocated state when it
> > succeeds. And use that state in haltpoll_unint() to call
> > cpuhp_remove_state() instead of looping online cpus manually. This
> > is because cpuhp_remove_state() invokes the teardown/offline callback.
> > * fix subsystem name to 'cpuidle' instead of 'idle' in cpuhp_setup_state()
>
> Marcelo, is the R-by still applicable?
>
> Paolo, any comments?
>
> >
> > v2:
> > * move cpus_read_unlock() 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) for CPUHP_AP_ONLINE_DYN
> > ---
> > arch/x86/include/asm/cpuidle_haltpoll.h | 4 +-
> > arch/x86/kernel/kvm.c | 18 +++----
> > drivers/cpuidle/cpuidle-haltpoll.c | 68 +++++++++++++++++++++++--
> > include/linux/cpuidle_haltpoll.h | 4 +-
> > 4 files changed, 73 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..56d8ab814466 100644
> > --- a/drivers/cpuidle/cpuidle-haltpoll.c
> > +++ b/drivers/cpuidle/cpuidle-haltpoll.c
> > @@ -11,12 +11,16 @@
> > */
> >
> > #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 enum cpuhp_state haltpoll_hp_state;
> > +
> > static int default_enter_idle(struct cpuidle_device *dev,
> > struct cpuidle_driver *drv, int index)
> > {
> > @@ -46,6 +50,46 @@ 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 int haltpoll_cpu_offline(unsigned int cpu)
> > +{
> > + struct cpuidle_device *dev;
> > +
> > + dev = per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
> > + if (dev->registered) {
> > + arch_haltpoll_disable(cpu);
> > + cpuidle_unregister_device(dev);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void haltpoll_uninit(void)
> > +{
> > + if (haltpoll_hp_state)
> > + cpuhp_remove_state(haltpoll_hp_state);
> > + cpuidle_unregister_driver(&haltpoll_driver);
> > +
> > + free_percpu(haltpoll_cpuidle_devices);
> > + haltpoll_cpuidle_devices = NULL;
> > +}
> > +
> > static int __init haltpoll_init(void)
> > {
> > int ret;
> > @@ -56,17 +100,31 @@ 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, "cpuidle/haltpoll:online",
> > + haltpoll_cpu_online, haltpoll_cpu_offline);
> > + if (ret < 0) {
> > + haltpoll_uninit();
> > + } else {
> > + haltpoll_hp_state = ret;
> > + 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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] cpuidle-haltpoll: vcpu hotplug support
2019-09-02 22:17 ` Marcelo Tosatti
@ 2019-09-03 7:38 ` Rafael J. Wysocki
0 siblings, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2019-09-03 7:38 UTC (permalink / raw)
To: Marcelo Tosatti, Joao Martins
Cc: Rafael J. Wysocki, Paolo Bonzini, kvm-devel,
Linux Kernel Mailing List, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Rafael J. Wysocki, Daniel Lezcano, Linux PM,
Boris Ostrovsky
On Tue, Sep 3, 2019 at 12:18 AM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> On Mon, Sep 02, 2019 at 10:34:07PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Sep 2, 2019 at 12:43 PM Joao Martins <joao.m.martins@oracle.com> 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() callbacks and future
> > > ones that get onlined or offlined. 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>
> > > ---
> > > v3:
> > > * register the teardown callback for correct handling of hotunplug
> > > and error cases. In case cpuhp_setup_state calls fails (e.g. in one of
> > > the cpus that it invoked the callback) it will then call the teardown of
> > > the previously enabled devices; so no need to handle that manually in
> > > haltpoll_uninit().
> > > * use the cpuhp_setup_state() returned dyn allocated state when it
> > > succeeds. And use that state in haltpoll_unint() to call
> > > cpuhp_remove_state() instead of looping online cpus manually. This
> > > is because cpuhp_remove_state() invokes the teardown/offline callback.
> > > * fix subsystem name to 'cpuidle' instead of 'idle' in cpuhp_setup_state()
> >
> > Marcelo, is the R-by still applicable?
> >
> > Paolo, any comments?
> >
> > >
> > > v2:
> > > * move cpus_read_unlock() 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) for CPUHP_AP_ONLINE_DYN
> > > ---
> > > arch/x86/include/asm/cpuidle_haltpoll.h | 4 +-
> > > arch/x86/kernel/kvm.c | 18 +++----
> > > drivers/cpuidle/cpuidle-haltpoll.c | 68 +++++++++++++++++++++++--
> > > include/linux/cpuidle_haltpoll.h | 4 +-
> > > 4 files changed, 73 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..56d8ab814466 100644
> > > --- a/drivers/cpuidle/cpuidle-haltpoll.c
> > > +++ b/drivers/cpuidle/cpuidle-haltpoll.c
> > > @@ -11,12 +11,16 @@
> > > */
> > >
> > > #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 enum cpuhp_state haltpoll_hp_state;
> > > +
> > > static int default_enter_idle(struct cpuidle_device *dev,
> > > struct cpuidle_driver *drv, int index)
> > > {
> > > @@ -46,6 +50,46 @@ 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 int haltpoll_cpu_offline(unsigned int cpu)
> > > +{
> > > + struct cpuidle_device *dev;
> > > +
> > > + dev = per_cpu_ptr(haltpoll_cpuidle_devices, cpu);
> > > + if (dev->registered) {
> > > + arch_haltpoll_disable(cpu);
> > > + cpuidle_unregister_device(dev);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void haltpoll_uninit(void)
> > > +{
> > > + if (haltpoll_hp_state)
> > > + cpuhp_remove_state(haltpoll_hp_state);
> > > + cpuidle_unregister_driver(&haltpoll_driver);
> > > +
> > > + free_percpu(haltpoll_cpuidle_devices);
> > > + haltpoll_cpuidle_devices = NULL;
> > > +}
> > > +
> > > static int __init haltpoll_init(void)
> > > {
> > > int ret;
> > > @@ -56,17 +100,31 @@ 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, "cpuidle/haltpoll:online",
> > > + haltpoll_cpu_online, haltpoll_cpu_offline);
> > > + if (ret < 0) {
> > > + haltpoll_uninit();
> > > + } else {
> > > + haltpoll_hp_state = ret;
> > > + 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>
OK, queued for v5.4, thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-03 7:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-02 10:40 [PATCH v3] cpuidle-haltpoll: vcpu hotplug support Joao Martins
2019-09-02 20:34 ` Rafael J. Wysocki
2019-09-02 22:17 ` Marcelo Tosatti
2019-09-03 7:38 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).