Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH RFC v3 1/3] reboot: support hotplug CPUs before reboot | Date | Mon, 13 Jan 2020 16:57:29 +0100 |
| |
Hsin-Yi Wang <hsinyi@chromium.org> writes:
> On Mon, Jan 13, 2020 at 8:46 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Thanks for your comments. > >> > +config REBOOT_HOTPLUG_CPU >> > + bool "Support for hotplug CPUs before reboot" >> > + depends on HOTPLUG_CPU >> > + help >> > + Say Y to do a full hotplug on secondary CPUs before reboot. >> >> I'm not sure this should be a configurable option, e.g. in case this is >> a good approach in general, why not just use CONFIG_HOTPLUG_CPU in the >> code? >> > In v2 it uses CONFIG_HOTPLUG_CPU, but I think adding another config is > more flexible. Maybe there are some architecture that supports > HOTPLUG_CPU but doesn't want to do full cpu hotplug before reboot. > (Eg. doing cpu hotplug would make reboot process slower.)
In that case this should be an architectural decision, not a selectable option. If you want to enable it for certain arches only (and not the other way around), that would look like
config ARCH_HAS_HOTUNPLUG_CPUS_ON_REBOOT bool
...
config X86 def_bool y ... select ARCH_HAS_HOTUNPLUG_CPUS_ON_REBOOT
because as a user, I really have no idea if I want to 'unplug secondary CPUs on reboot' or not.
>> > + >> > config HAVE_OPROFILE >> > bool >> > >> > diff --git a/include/linux/cpu.h b/include/linux/cpu.h >> > index 1ca2baf817ed..3bf5ab289954 100644 >> > --- a/include/linux/cpu.h >> > +++ b/include/linux/cpu.h >> > @@ -118,6 +118,9 @@ extern void cpu_hotplug_disable(void); >> > extern void cpu_hotplug_enable(void); >> > void clear_tasks_mm_cpumask(int cpu); >> > int cpu_down(unsigned int cpu); >> > +#if IS_ENABLED(CONFIG_REBOOT_HOTPLUG_CPU) >> > +extern void offline_secondary_cpus(int primary); >> > +#endif >> > >> > #else /* CONFIG_HOTPLUG_CPU */ >> > >> > diff --git a/kernel/cpu.c b/kernel/cpu.c >> > index 9c706af713fb..52afc47dd56a 100644 >> > --- a/kernel/cpu.c >> > +++ b/kernel/cpu.c >> > @@ -1057,6 +1057,25 @@ int cpu_down(unsigned int cpu) >> > } >> > EXPORT_SYMBOL(cpu_down); >> > >> > +#if IS_ENABLED(CONFIG_REBOOT_HOTPLUG_CPU) >> > +void offline_secondary_cpus(int primary) >> > +{ >> > + int i, err; >> > + >> > + cpu_maps_update_begin(); >> > + >> > + for_each_online_cpu(i) { >> > + if (i == primary) >> > + continue; >> > + err = _cpu_down(i, 0, CPUHP_OFFLINE); >> > + if (err) >> > + pr_warn("Failed to offline cpu %d\n", i); >> > + } >> > + cpu_hotplug_disabled++; >> > + >> > + cpu_maps_update_done(); >> > +} >> > +#endif >> >> This looks like a simplified version of freeze_secondary_cpus(), can >> they be merged? >> > Comparing to freeze_secondary_cpus(), I think it's not necessary to > check pm_wakeup_pending() before _cpu_down() here. Thus it doesn't > need to depend on CONFIG_PM_SLEEP_SMP. > Also I think it could continue to offline other CPUs even one fails, > while freeze_secondary_cpus() would stop once it fails on offlining > one CPU. > Based on these differences, I didn't use freeze_secondary_cpus() here. > As for merging the common part, it might need additional flags to > handle the difference, which might lower the readability.
I have to admit I'm not convinced (but maintainers may disagree of course): #ifdefs are there to avoid compiling code which we don't need, in case a second user emerges we can drop them or #ifdef just some parts of it, it's not set in stone. Also, in case the only difference is that you don't want to stop if some CPU fails to offline, a single bool flag (e.g. 'force') would solve the problem, I don't see a significant readability change.
-- Vitaly
| |