Re: [PATCH v1] cpuidle-haltpoll: vcpu hotplug support
On Thu, Aug 29, 2019 at 03:24:31PM +0100, Joao Martins wrote: > On 8/29/19 2:50 PM, Joao Martins wrote: > > On 8/29/19 12:56 PM, Marcelo Tosatti wrote: > >> Hi Joao, > >> > >> On Wed, Aug 28, 2019 at 07:56:50PM +0100, Joao Martins wrote: > >>> +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); > >>> + } > >> > >> 1) > >> > >>> + > >>> + cpuidle_unregister(&haltpoll_driver); > >> > >> cpuidle_unregister_driver. > > > > Will fix -- this was an oversight. > > > >> > >>> + free_percpu(haltpoll_cpuidle_devices); > >>> + haltpoll_cpuidle_devices = NULL; > >>> + > >>> + cpus_read_unlock(); > >> > >> Any reason you can't cpus_read_unlock() at 1) ? > >> > > No, let me adjust that too. > > > >> Looks good otherwise. > >> > > BTW, should I take this as a Acked-by, Reviewed-by, or neither? :) > > Joao I'll ACK -v2 once you send it.
Re: [PATCH v1] cpuidle-haltpoll: vcpu hotplug support
On 8/29/19 2:50 PM, Joao Martins wrote: > On 8/29/19 12:56 PM, Marcelo Tosatti wrote: >> Hi Joao, >> >> On Wed, Aug 28, 2019 at 07:56:50PM +0100, Joao Martins wrote: >>> +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); >>> + } >> >> 1) >> >>> + >>> + cpuidle_unregister(&haltpoll_driver); >> >> cpuidle_unregister_driver. > > Will fix -- this was an oversight. > >> >>> + free_percpu(haltpoll_cpuidle_devices); >>> + haltpoll_cpuidle_devices = NULL; >>> + >>> + cpus_read_unlock(); >> >> Any reason you can't cpus_read_unlock() at 1) ? >> > No, let me adjust that too. > >> Looks good otherwise. >> BTW, should I take this as a Acked-by, Reviewed-by, or neither? :) Joao
Re: [PATCH v1] cpuidle-haltpoll: vcpu hotplug support
On 8/29/19 12:56 PM, Marcelo Tosatti wrote: > Hi Joao, > > On Wed, Aug 28, 2019 at 07:56:50PM +0100, Joao Martins wrote: >> +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); >> +} > > 1) > >> + >> +cpuidle_unregister(&haltpoll_driver); > > cpuidle_unregister_driver. Will fix -- this was an oversight. > >> +free_percpu(haltpoll_cpuidle_devices); >> +haltpoll_cpuidle_devices = NULL; >> + >> +cpus_read_unlock(); > > Any reason you can't cpus_read_unlock() at 1) ? > No, let me adjust that too. > Looks good otherwise. > > Thanks! > Thanks for the review! Joao
Re: [PATCH v1] cpuidle-haltpoll: vcpu hotplug support
Hi Joao, On Wed, Aug 28, 2019 at 07:56:50PM +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() and future ones that > get onlined. This mimics similar logic as intel_idle. > > Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver") > Signed-off-by: Joao Martins > Signed-off-by: Boris Ostrovsky > --- > arch/x86/include/asm/cpuidle_haltpoll.h | 4 +- > arch/x86/kernel/kvm.c | 18 +++ > drivers/cpuidle/cpuidle-haltpoll.c | 65 +++-- > include/linux/cpuidle_haltpoll.h| 4 +- > 4 files changed, 70 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..0d1853a7185e 100644 > --- a/drivers/cpuidle/cpuidle-haltpoll.c > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > @@ -11,12 +11,15 @@ > */ > > #include > +#include > #include > #include > #include > #include > #include > > +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); > + } 1) > + > + cpuidle_unregister(&haltpoll_driver); cpuidle_unregister_driver. > + free_percpu(haltpoll_cpuidle_devices); > + haltpoll_cpuidle_devices = NULL; > + > + cpus_read_unlock(); Any reason you can't cpus_read_unlock() at 1) ? Looks good otherwise. Thanks!
Re: [PATCH v1] cpuidle-haltpoll: vcpu hotplug support
On Wed, Aug 28, 2019 at 8:58 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() and future ones that > get onlined. This mimics similar logic as intel_idle. > > Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver") > Signed-off-by: Joao Martins > Signed-off-by: Boris Ostrovsky Paolo, what do you think? > --- > arch/x86/include/asm/cpuidle_haltpoll.h | 4 +- > arch/x86/kernel/kvm.c | 18 +++ > drivers/cpuidle/cpuidle-haltpoll.c | 65 +++-- > include/linux/cpuidle_haltpoll.h| 4 +- > 4 files changed, 70 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..0d1853a7185e 100644 > --- a/drivers/cpuidle/cpuidle-haltpoll.c > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > @@ -11,12 +11,15 @@ > */ > > #include > +#include > #include > #include > #include > #include > #include > > +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); > + } > + > + cpuidle_unregister(&haltpoll_driver); > + > + free_percpu(haltpoll_cpuidle_devices); > + haltpoll_cpuidle_devices = NULL; > + > + cpus_read_unlock(); > +} > + > static int __init haltpoll_init(voi
[PATCH v1] cpuidle-haltpoll: vcpu hotplug support
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() and future ones that get onlined. This mimics similar logic as intel_idle. Fixes: fa86ee90eb11 ("add cpuidle-haltpoll driver") Signed-off-by: Joao Martins Signed-off-by: Boris Ostrovsky --- arch/x86/include/asm/cpuidle_haltpoll.h | 4 +- arch/x86/kernel/kvm.c | 18 +++ drivers/cpuidle/cpuidle-haltpoll.c | 65 +++-- include/linux/cpuidle_haltpoll.h| 4 +- 4 files changed, 70 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..0d1853a7185e 100644 --- a/drivers/cpuidle/cpuidle-haltpoll.c +++ b/drivers/cpuidle/cpuidle-haltpoll.c @@ -11,12 +11,15 @@ */ #include +#include #include #include #include #include #include +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); + } + + cpuidle_unregister(&haltpoll_driver); + + free_percpu(haltpoll_cpuidle_devices); + haltpoll_cpuidle_devices = NULL; + + cpus_read_unlock(); +} + static int __init haltpoll_init(void) { int ret; @@ -56,17 +101,27 @@ 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; +