| Message ID | 1369727984-21505-2-git-send-email-chenhui.zhao@freescale.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2013-05-28 at 15:59 +0800, Zhao Chenhui wrote: > Some features depend on the boot cpu, for instance, hibernate/suspend. > So disable hotplug for the boot cpu. Don't we have code to "move" the boot CPU around when that happens ? Ben. > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com> > --- > arch/powerpc/kernel/sysfs.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c > index e68a845..294b1c4e 100644 > --- a/arch/powerpc/kernel/sysfs.c > +++ b/arch/powerpc/kernel/sysfs.c > @@ -655,8 +655,10 @@ static int __init topology_init(void) > * CPU. For instance, the boot cpu might never be valid > * for hotplugging. > */ > - if (ppc_md.cpu_die) > + if (ppc_md.cpu_die && cpu != boot_cpuid) > c->hotpluggable = 1; > + else > + c->hotpluggable = 0; > > if (cpu_online(cpu) || c->hotpluggable) { > register_cpu(c, cpu);
On Sat, Jun 01, 2013 at 07:49:44AM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2013-05-28 at 15:59 +0800, Zhao Chenhui wrote: > > Some features depend on the boot cpu, for instance, hibernate/suspend. > > So disable hotplug for the boot cpu. > > Don't we have code to "move" the boot CPU around when that happens ? > > Ben. > Currently, the code in generic_cpu_disable() likes this: if (cpu == boot_cpuid) return -EBUSY; If the dying cpu is the boot cpu, it will return -EBUSY. In the subsequent error handling, cpu_notify_nofail(CPU_DOWN_FAILED) in _cpu_down() will be called. Unfortunately, some cpu notifier callbacks handled CPU_DOWN_PREPARE, but not CPU_DOWN_FAILED, such as sched_cpu_inactive(). So it will cause issues. If we set the hotpluggable for the boot cpu, we can prevent user applications from disabling the boot cpu. -Chenhui > > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com> > > --- > > arch/powerpc/kernel/sysfs.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c > > index e68a845..294b1c4e 100644 > > --- a/arch/powerpc/kernel/sysfs.c > > +++ b/arch/powerpc/kernel/sysfs.c > > @@ -655,8 +655,10 @@ static int __init topology_init(void) > > * CPU. For instance, the boot cpu might never be valid > > * for hotplugging. > > */ > > - if (ppc_md.cpu_die) > > + if (ppc_md.cpu_die && cpu != boot_cpuid) > > c->hotpluggable = 1; > > + else > > + c->hotpluggable = 0; > > > > if (cpu_online(cpu) || c->hotpluggable) { > > register_cpu(c, cpu); > > >
On Mon, 2013-06-03 at 18:43 +0800, Zhao Chenhui wrote: > On Sat, Jun 01, 2013 at 07:49:44AM +1000, Benjamin Herrenschmidt wrote: > > On Tue, 2013-05-28 at 15:59 +0800, Zhao Chenhui wrote: > > > Some features depend on the boot cpu, for instance, hibernate/suspend. > > > So disable hotplug for the boot cpu. > > > > Don't we have code to "move" the boot CPU around when that happens ? > > > > Ben. > > > > Currently, the code in generic_cpu_disable() likes this: > > if (cpu == boot_cpuid) > return -EBUSY; But the code in pseries/hotplug-cpu.c doesn't, we just "move" the boot CPU around when that happens. Any reason we can't do that generically ? Cheers, Ben. > If the dying cpu is the boot cpu, it will return -EBUSY. In the subsequent error handling, > cpu_notify_nofail(CPU_DOWN_FAILED) in _cpu_down() will be called. Unfortunately, some > cpu notifier callbacks handled CPU_DOWN_PREPARE, but not CPU_DOWN_FAILED, such as sched_cpu_inactive(). > So it will cause issues. > > If we set the hotpluggable for the boot cpu, we can prevent user applications from disabling the boot cpu. > > -Chenhui > > > > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com> > > > --- > > > arch/powerpc/kernel/sysfs.c | 4 +++- > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c > > > index e68a845..294b1c4e 100644 > > > --- a/arch/powerpc/kernel/sysfs.c > > > +++ b/arch/powerpc/kernel/sysfs.c > > > @@ -655,8 +655,10 @@ static int __init topology_init(void) > > > * CPU. For instance, the boot cpu might never be valid > > > * for hotplugging. > > > */ > > > - if (ppc_md.cpu_die) > > > + if (ppc_md.cpu_die && cpu != boot_cpuid) > > > c->hotpluggable = 1; > > > + else > > > + c->hotpluggable = 0; > > > > > > if (cpu_online(cpu) || c->hotpluggable) { > > > register_cpu(c, cpu); > > > > > >
On Wed, Jun 12, 2013 at 01:25:22PM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2013-06-03 at 18:43 +0800, Zhao Chenhui wrote: > > On Sat, Jun 01, 2013 at 07:49:44AM +1000, Benjamin Herrenschmidt wrote: > > > On Tue, 2013-05-28 at 15:59 +0800, Zhao Chenhui wrote: > > > > Some features depend on the boot cpu, for instance, hibernate/suspend. > > > > So disable hotplug for the boot cpu. > > > > > > Don't we have code to "move" the boot CPU around when that happens ? > > > > > > Ben. > > > > > > > Currently, the code in generic_cpu_disable() likes this: > > > > if (cpu == boot_cpuid) > > return -EBUSY; > > But the code in pseries/hotplug-cpu.c doesn't, we just "move" the boot > CPU around when that happens. Any reason we can't do that generically ? > > Cheers, > Ben. > Some multicore SoCs firstly boot up the cpu0 after warm reset. In some suspend/resume cases, SoC will do a warm reset when resuming. In order to ensure that the suspending and resuming is running on a same cpu, cpu0 should be the last cpu to suspend. Here, cpu0 is the boot_cpuid. -Chenhui > > If the dying cpu is the boot cpu, it will return -EBUSY. In the subsequent error handling, > > cpu_notify_nofail(CPU_DOWN_FAILED) in _cpu_down() will be called. Unfortunately, some > > cpu notifier callbacks handled CPU_DOWN_PREPARE, but not CPU_DOWN_FAILED, such as sched_cpu_inactive(). > > So it will cause issues. > > > > If we set the hotpluggable for the boot cpu, we can prevent user applications from disabling the boot cpu. > > > > -Chenhui > >
On Thu, 2013-06-13 at 19:25 +0800, Zhao Chenhui wrote: > Some multicore SoCs firstly boot up the cpu0 after warm reset. > In some suspend/resume cases, SoC will do a warm reset when resuming. > In order to ensure that the suspending and resuming is running > on a same cpu, cpu0 should be the last cpu to suspend. Here, cpu0 is > the boot_cpuid. Well, so: - In any case, your patch will break pseries, so it's not acceptable. - Why does it have to absolutely resume from the same CPU it suspended from ? Can't you have a little bit of code on the resuming CPU that checks if it's not online, poke an online one and goes back to sleep ? Cheers, Ben.
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c index e68a845..294b1c4e 100644 --- a/arch/powerpc/kernel/sysfs.c +++ b/arch/powerpc/kernel/sysfs.c @@ -655,8 +655,10 @@ static int __init topology_init(void) * CPU. For instance, the boot cpu might never be valid * for hotplugging. */ - if (ppc_md.cpu_die) + if (ppc_md.cpu_die && cpu != boot_cpuid) c->hotpluggable = 1; + else + c->hotpluggable = 0; if (cpu_online(cpu) || c->hotpluggable) { register_cpu(c, cpu);
Some features depend on the boot cpu, for instance, hibernate/suspend. So disable hotplug for the boot cpu. Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com> --- arch/powerpc/kernel/sysfs.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)