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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Jan 2020 16:57:29 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Hsin-Yi Wang <hsinyi@...omium.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Pavankumar Kondeti <pkondeti@...eaurora.org>,
        Zhenzhong Duan <zhenzhong.duan@...cle.com>,
        Aaro Koskinen <aaro.koskinen@...ia.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Guenter Roeck <groeck@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH RFC v3 1/3] reboot: support hotplug CPUs before reboot

Hsin-Yi Wang <hsinyi@...omium.org> writes:

> On Mon, Jan 13, 2020 at 8:46 PM Vitaly Kuznetsov <vkuznets@...hat.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