lkml.org 
[lkml]   [2020]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH RFC v3 1/3] reboot: support hotplug CPUs before reboot
On Mon, Jan 13, 2020 at 11:57 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> 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.
>
Thanks for the example. I would use this way in next version.
> >> > +
> >> > 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.
>
Okay, I will merge them with an additional flag for whether it should
check pm_wakeup_pending() and break on error.
> --
> Vitaly
>

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