| Message ID | 5608485.T3GFgtNYov@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Mon, Apr 29, 2013 at 02:28:02PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > generic offline/online support introduced previously instead of > its own CPU-specific code. > > For this purpose, modify cpu_subsys to provide offline and online > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > the CPU-specific 'online' sysfs attribute. > > This modification is not supposed to change the user-observable > behavior of the kernel (i.e. the 'online' attribute will be present > in exactly the same place in sysfs and should trigger exactly the > same actions as before). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > 1 file changed, 15 insertions(+), 47 deletions(-) Very nice, I like reductions like this :) greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, April 29, 2013 04:11:06 PM Greg Kroah-Hartman wrote: > On Mon, Apr 29, 2013 at 02:28:02PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > > generic offline/online support introduced previously instead of > > its own CPU-specific code. > > > > For this purpose, modify cpu_subsys to provide offline and online > > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > > the CPU-specific 'online' sysfs attribute. > > > > This modification is not supposed to change the user-observable > > behavior of the kernel (i.e. the 'online' attribute will be present > > in exactly the same place in sysfs and should trigger exactly the > > same actions as before). > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > > 1 file changed, 15 insertions(+), 47 deletions(-) > > Very nice, I like reductions like this :) Thanks! So I guess the patches make sense to you overall? Rafael
On Tue, Apr 30, 2013 at 02:01:10PM +0200, Rafael J. Wysocki wrote: > On Monday, April 29, 2013 04:11:06 PM Greg Kroah-Hartman wrote: > > On Mon, Apr 29, 2013 at 02:28:02PM +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > > > generic offline/online support introduced previously instead of > > > its own CPU-specific code. > > > > > > For this purpose, modify cpu_subsys to provide offline and online > > > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > > > the CPU-specific 'online' sysfs attribute. > > > > > > This modification is not supposed to change the user-observable > > > behavior of the kernel (i.e. the 'online' attribute will be present > > > in exactly the same place in sysfs and should trigger exactly the > > > same actions as before). > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > > > 1 file changed, 15 insertions(+), 47 deletions(-) > > > > Very nice, I like reductions like this :) > > Thanks! > > So I guess the patches make sense to you overall? Overall, yes, I like them a lot. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, April 30, 2013 08:27:53 AM Greg Kroah-Hartman wrote: > On Tue, Apr 30, 2013 at 02:01:10PM +0200, Rafael J. Wysocki wrote: > > On Monday, April 29, 2013 04:11:06 PM Greg Kroah-Hartman wrote: > > > On Mon, Apr 29, 2013 at 02:28:02PM +0200, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > > > > generic offline/online support introduced previously instead of > > > > its own CPU-specific code. > > > > > > > > For this purpose, modify cpu_subsys to provide offline and online > > > > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > > > > the CPU-specific 'online' sysfs attribute. > > > > > > > > This modification is not supposed to change the user-observable > > > > behavior of the kernel (i.e. the 'online' attribute will be present > > > > in exactly the same place in sysfs and should trigger exactly the > > > > same actions as before). > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > > > > 1 file changed, 15 insertions(+), 47 deletions(-) > > > > > > Very nice, I like reductions like this :) > > > > Thanks! > > > > So I guess the patches make sense to you overall? > > Overall, yes, I like them a lot. Cool, thanks! :-) Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-04-29 at 14:28 +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > generic offline/online support introduced previously instead of > its own CPU-specific code. > > For this purpose, modify cpu_subsys to provide offline and online > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > the CPU-specific 'online' sysfs attribute. > > This modification is not supposed to change the user-observable > behavior of the kernel (i.e. the 'online' attribute will be present > in exactly the same place in sysfs and should trigger exactly the > same actions as before). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > 1 file changed, 15 insertions(+), 47 deletions(-) > > Index: linux-pm/drivers/base/cpu.c > =================================================================== > --- linux-pm.orig/drivers/base/cpu.c > +++ linux-pm/drivers/base/cpu.c > @@ -16,66 +16,25 @@ > > #include "base.h" > > -struct bus_type cpu_subsys = { > - .name = "cpu", > - .dev_name = "cpu", > -}; > -EXPORT_SYMBOL_GPL(cpu_subsys); > - > static DEFINE_PER_CPU(struct device *, cpu_sys_devices); > > #ifdef CONFIG_HOTPLUG_CPU > -static ssize_t show_online(struct device *dev, > - struct device_attribute *attr, > - char *buf) > +static int cpu_subsys_online(struct device *dev) > { > - struct cpu *cpu = container_of(dev, struct cpu, dev); > - > - return sprintf(buf, "%u\n", !!cpu_online(cpu->dev.id)); > + return cpu_up(dev->id); > } > > -static ssize_t __ref store_online(struct device *dev, > - struct device_attribute *attr, > - const char *buf, size_t count) > +static int cpu_subsys_offline(struct device *dev) > { > - struct cpu *cpu = container_of(dev, struct cpu, dev); > - ssize_t ret; > - > - cpu_hotplug_driver_lock(); By replacing cpu_hotplug_driver_lock() with lock_device_offline() in patch 1/3, it no longer protects from other places that still use cpu_hotplug_device_lock(), such as save_mc_for_early(). Thanks, -Toshi > - switch (buf[0]) { > - case '0': > - ret = cpu_down(cpu->dev.id); > - if (!ret) > - kobject_uevent(&dev->kobj, KOBJ_OFFLINE); > - break; > - case '1': > - ret = cpu_up(cpu->dev.id); > - if (!ret) > - kobject_uevent(&dev->kobj, KOBJ_ONLINE); > - break; > - default: > - ret = -EINVAL; > - } > - cpu_hotplug_driver_unlock(); > - > - if (ret >= 0) > - ret = count; > - return ret; > + return cpu_down(dev->id); > } > -static DEVICE_ATTR(online, 0644, show_online, store_online); -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, April 30, 2013 05:42:06 PM Toshi Kani wrote: > On Mon, 2013-04-29 at 14:28 +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > > generic offline/online support introduced previously instead of > > its own CPU-specific code. > > > > For this purpose, modify cpu_subsys to provide offline and online > > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > > the CPU-specific 'online' sysfs attribute. > > > > This modification is not supposed to change the user-observable > > behavior of the kernel (i.e. the 'online' attribute will be present > > in exactly the same place in sysfs and should trigger exactly the > > same actions as before). > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > > 1 file changed, 15 insertions(+), 47 deletions(-) > > > > Index: linux-pm/drivers/base/cpu.c > > =================================================================== > > --- linux-pm.orig/drivers/base/cpu.c > > +++ linux-pm/drivers/base/cpu.c > > @@ -16,66 +16,25 @@ > > > > #include "base.h" > > > > -struct bus_type cpu_subsys = { > > - .name = "cpu", > > - .dev_name = "cpu", > > -}; > > -EXPORT_SYMBOL_GPL(cpu_subsys); > > - > > static DEFINE_PER_CPU(struct device *, cpu_sys_devices); > > > > #ifdef CONFIG_HOTPLUG_CPU > > -static ssize_t show_online(struct device *dev, > > - struct device_attribute *attr, > > - char *buf) > > +static int cpu_subsys_online(struct device *dev) > > { > > - struct cpu *cpu = container_of(dev, struct cpu, dev); > > - > > - return sprintf(buf, "%u\n", !!cpu_online(cpu->dev.id)); > > + return cpu_up(dev->id); > > } > > > > -static ssize_t __ref store_online(struct device *dev, > > - struct device_attribute *attr, > > - const char *buf, size_t count) > > +static int cpu_subsys_offline(struct device *dev) > > { > > - struct cpu *cpu = container_of(dev, struct cpu, dev); > > - ssize_t ret; > > - > > - cpu_hotplug_driver_lock(); > > By replacing cpu_hotplug_driver_lock() with lock_device_offline() in > patch 1/3, it no longer protects from other places that still use > cpu_hotplug_device_lock(), such as save_mc_for_early(). Yes. What about taking cpu_hotplug_driver_lock() around cpu_up() and cpu_down() in cpu_subsys_online() and cpu_subsys_offline()? Alternatively, I can just replace cpu_hotplug_driver_lock() with lock_device_offline() everywhere. Thanks, Rafael > > - switch (buf[0]) { > > - case '0': > > - ret = cpu_down(cpu->dev.id); > > - if (!ret) > > - kobject_uevent(&dev->kobj, KOBJ_OFFLINE); > > - break; > > - case '1': > > - ret = cpu_up(cpu->dev.id); > > - if (!ret) > > - kobject_uevent(&dev->kobj, KOBJ_ONLINE); > > - break; > > - default: > > - ret = -EINVAL; > > - } > > - cpu_hotplug_driver_unlock(); > > - > > - if (ret >= 0) > > - ret = count; > > - return ret; > > + return cpu_down(dev->id); > > } > > -static DEVICE_ATTR(online, 0644, show_online, store_online); > >
On Wed, 2013-05-01 at 16:49 +0200, Rafael J. Wysocki wrote: > On Tuesday, April 30, 2013 05:42:06 PM Toshi Kani wrote: > > On Mon, 2013-04-29 at 14:28 +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > > > generic offline/online support introduced previously instead of > > > its own CPU-specific code. > > > > > > For this purpose, modify cpu_subsys to provide offline and online > > > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > > > the CPU-specific 'online' sysfs attribute. > > > > > > This modification is not supposed to change the user-observable > > > behavior of the kernel (i.e. the 'online' attribute will be present > > > in exactly the same place in sysfs and should trigger exactly the > > > same actions as before). > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > > > 1 file changed, 15 insertions(+), 47 deletions(-) > > > > > > Index: linux-pm/drivers/base/cpu.c > > > =================================================================== > > > --- linux-pm.orig/drivers/base/cpu.c > > > +++ linux-pm/drivers/base/cpu.c > > > @@ -16,66 +16,25 @@ > > > > > > #include "base.h" > > > > > > -struct bus_type cpu_subsys = { > > > - .name = "cpu", > > > - .dev_name = "cpu", > > > -}; > > > -EXPORT_SYMBOL_GPL(cpu_subsys); > > > - > > > static DEFINE_PER_CPU(struct device *, cpu_sys_devices); > > > > > > #ifdef CONFIG_HOTPLUG_CPU > > > -static ssize_t show_online(struct device *dev, > > > - struct device_attribute *attr, > > > - char *buf) > > > +static int cpu_subsys_online(struct device *dev) > > > { > > > - struct cpu *cpu = container_of(dev, struct cpu, dev); > > > - > > > - return sprintf(buf, "%u\n", !!cpu_online(cpu->dev.id)); > > > + return cpu_up(dev->id); > > > } > > > > > > -static ssize_t __ref store_online(struct device *dev, > > > - struct device_attribute *attr, > > > - const char *buf, size_t count) > > > +static int cpu_subsys_offline(struct device *dev) > > > { > > > - struct cpu *cpu = container_of(dev, struct cpu, dev); > > > - ssize_t ret; > > > - > > > - cpu_hotplug_driver_lock(); > > > > By replacing cpu_hotplug_driver_lock() with lock_device_offline() in > > patch 1/3, it no longer protects from other places that still use > > cpu_hotplug_device_lock(), such as save_mc_for_early(). > > Yes. > > What about taking cpu_hotplug_driver_lock() around cpu_up() and > cpu_down() in cpu_subsys_online() and cpu_subsys_offline()? Sounds like a reasonable approach to me. > Alternatively, I can just replace cpu_hotplug_driver_lock() with > lock_device_offline() everywhere. That works too. Not sure which way is better. If we go this option, I'd suggest to rename lock_device_offline() since it could be misleading that the lock is only used for offline, i.e. excluding online. lock_device_hotplug() might be less confusing although we distinguish online/offline and hotplug operations. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, May 01, 2013 02:07:45 PM Toshi Kani wrote: > On Wed, 2013-05-01 at 16:49 +0200, Rafael J. Wysocki wrote: > > On Tuesday, April 30, 2013 05:42:06 PM Toshi Kani wrote: > > > On Mon, 2013-04-29 at 14:28 +0200, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Rework the CPU hotplug code in drivers/base/cpu.c to use the > > > > generic offline/online support introduced previously instead of > > > > its own CPU-specific code. > > > > > > > > For this purpose, modify cpu_subsys to provide offline and online > > > > callbacks for CONFIG_HOTPLUG_CPU set and remove the code handling > > > > the CPU-specific 'online' sysfs attribute. > > > > > > > > This modification is not supposed to change the user-observable > > > > behavior of the kernel (i.e. the 'online' attribute will be present > > > > in exactly the same place in sysfs and should trigger exactly the > > > > same actions as before). > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > drivers/base/cpu.c | 62 ++++++++++++----------------------------------------- > > > > 1 file changed, 15 insertions(+), 47 deletions(-) > > > > > > > > Index: linux-pm/drivers/base/cpu.c > > > > =================================================================== > > > > --- linux-pm.orig/drivers/base/cpu.c > > > > +++ linux-pm/drivers/base/cpu.c > > > > @@ -16,66 +16,25 @@ > > > > > > > > #include "base.h" > > > > > > > > -struct bus_type cpu_subsys = { > > > > - .name = "cpu", > > > > - .dev_name = "cpu", > > > > -}; > > > > -EXPORT_SYMBOL_GPL(cpu_subsys); > > > > - > > > > static DEFINE_PER_CPU(struct device *, cpu_sys_devices); > > > > > > > > #ifdef CONFIG_HOTPLUG_CPU > > > > -static ssize_t show_online(struct device *dev, > > > > - struct device_attribute *attr, > > > > - char *buf) > > > > +static int cpu_subsys_online(struct device *dev) > > > > { > > > > - struct cpu *cpu = container_of(dev, struct cpu, dev); > > > > - > > > > - return sprintf(buf, "%u\n", !!cpu_online(cpu->dev.id)); > > > > + return cpu_up(dev->id); > > > > } > > > > > > > > -static ssize_t __ref store_online(struct device *dev, > > > > - struct device_attribute *attr, > > > > - const char *buf, size_t count) > > > > +static int cpu_subsys_offline(struct device *dev) > > > > { > > > > - struct cpu *cpu = container_of(dev, struct cpu, dev); > > > > - ssize_t ret; > > > > - > > > > - cpu_hotplug_driver_lock(); > > > > > > By replacing cpu_hotplug_driver_lock() with lock_device_offline() in > > > patch 1/3, it no longer protects from other places that still use > > > cpu_hotplug_device_lock(), such as save_mc_for_early(). > > > > Yes. > > > > What about taking cpu_hotplug_driver_lock() around cpu_up() and > > cpu_down() in cpu_subsys_online() and cpu_subsys_offline()? > > Sounds like a reasonable approach to me. > > > Alternatively, I can just replace cpu_hotplug_driver_lock() with > > lock_device_offline() everywhere. > > That works too. Not sure which way is better. It turns out that cpu_hotplug_driver_lock() is per-arch, so I'd prefer to just take it in cpu_subsys_online() and cpu_subsys_offline(), at least for the time being. > If we go this option, > I'd suggest to rename lock_device_offline() since it could be misleading > that the lock is only used for offline, i.e. excluding online. > lock_device_hotplug() might be less confusing although we distinguish > online/offline and hotplug operations. Well, I've decided to rename them to lock/unlock_device_hotplug() anyway, because "hotplug" has been used to refer to CPU offline/online for years. Thanks, Rafael
Index: linux-pm/drivers/base/cpu.c =================================================================== --- linux-pm.orig/drivers/base/cpu.c +++ linux-pm/drivers/base/cpu.c @@ -16,66 +16,25 @@ #include "base.h" -struct bus_type cpu_subsys = { - .name = "cpu", - .dev_name = "cpu", -}; -EXPORT_SYMBOL_GPL(cpu_subsys); - static DEFINE_PER_CPU(struct device *, cpu_sys_devices); #ifdef CONFIG_HOTPLUG_CPU -static ssize_t show_online(struct device *dev, - struct device_attribute *attr, - char *buf) +static int cpu_subsys_online(struct device *dev) { - struct cpu *cpu = container_of(dev, struct cpu, dev); - - return sprintf(buf, "%u\n", !!cpu_online(cpu->dev.id)); + return cpu_up(dev->id); } -static ssize_t __ref store_online(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t count) +static int cpu_subsys_offline(struct device *dev) { - struct cpu *cpu = container_of(dev, struct cpu, dev); - ssize_t ret; - - cpu_hotplug_driver_lock(); - switch (buf[0]) { - case '0': - ret = cpu_down(cpu->dev.id); - if (!ret) - kobject_uevent(&dev->kobj, KOBJ_OFFLINE); - break; - case '1': - ret = cpu_up(cpu->dev.id); - if (!ret) - kobject_uevent(&dev->kobj, KOBJ_ONLINE); - break; - default: - ret = -EINVAL; - } - cpu_hotplug_driver_unlock(); - - if (ret >= 0) - ret = count; - return ret; + return cpu_down(dev->id); } -static DEVICE_ATTR(online, 0644, show_online, store_online); -static void __cpuinit register_cpu_control(struct cpu *cpu) -{ - device_create_file(&cpu->dev, &dev_attr_online); -} void unregister_cpu(struct cpu *cpu) { int logical_cpu = cpu->dev.id; unregister_cpu_under_node(logical_cpu, cpu_to_node(logical_cpu)); - device_remove_file(&cpu->dev, &dev_attr_online); - device_unregister(&cpu->dev); per_cpu(cpu_sys_devices, logical_cpu) = NULL; return; @@ -108,6 +67,16 @@ static inline void register_cpu_control( } #endif /* CONFIG_HOTPLUG_CPU */ +struct bus_type cpu_subsys = { + .name = "cpu", + .dev_name = "cpu", +#ifdef CONFIG_HOTPLUG_CPU + .online = cpu_subsys_online, + .offline = cpu_subsys_offline, +#endif +}; +EXPORT_SYMBOL_GPL(cpu_subsys); + #ifdef CONFIG_KEXEC #include <linux/kexec.h> @@ -245,12 +214,11 @@ int __cpuinit register_cpu(struct cpu *c cpu->dev.id = num; cpu->dev.bus = &cpu_subsys; cpu->dev.release = cpu_device_release; + cpu->dev.offline_disabled = !cpu->hotpluggable; #ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE cpu->dev.bus->uevent = arch_cpu_uevent; #endif error = device_register(&cpu->dev); - if (!error && cpu->hotpluggable) - register_cpu_control(cpu); if (!error) per_cpu(cpu_sys_devices, num) = &cpu->dev; if (!error)