lkml.org 
[lkml]   [2020]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH RFC v3 1/3] reboot: support hotplug CPUs before reboot
Date
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

\
 
 \ /
  Last update: 2020-01-13 16:57    [W:0.060 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site