Re: [PATCH RFC v3 1/3] reboot: support hotplug CPUs before reboot

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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