Re: [PATCHv2] kexec: disable cpu hotplug until the rebooting cpu is stable

[Baoquan He]

On 02/09/22 at 03:31pm, Pingfan Liu wrote:
> On Tue, Feb 08, 2022 at 05:33:52PM +0800, Baoquan He wrote:
> > On 01/28/22 at 03:41pm, Pingfan Liu wrote:
> > > On Thu, Jan 27, 2022 at 05:41:44PM +0800, Baoquan He wrote:
> > > Hi Baoquan,
> > > 
> > > Thanks for reviewing, please see comment inlined
> > > > Hi Pingfan,
> > > > 
> > > > On 01/27/22 at 05:02pm, Pingfan Liu wrote:
> > > > > The following identical code piece appears in both
> > > > > migrate_to_reboot_cpu() and smp_shutdown_nonboot_cpus():
> > > > > 
> > > > > 	if (!cpu_online(primary_cpu))
> > > > > 		primary_cpu = cpumask_first(cpu_online_mask);
> > > > > 
> > > > > This is due to a breakage like the following:
> > > > >    migrate_to_reboot_cpu();
> > > > >    cpu_hotplug_enable();
> > > > >                           --> comes a cpu_down(this_cpu) on other cpu
> > > > >    machine_shutdown();
> > > > > 
> > > > > Although the kexec-reboot task can get through a cpu_down() on its cpu,
> > > > > this code looks a little confusing.
> > > > > 
> > > > > Make things straight forward by keeping cpu hotplug disabled until
> > > > > smp_shutdown_nonboot_cpus() holds cpu_add_remove_lock. By this way, the
> > > > > breakage is squashed out and the rebooting cpu can keep unchanged.
> > > > 
> > > > If I didn't go through code wrongly, you may miss the x86 case.
> > > > Several ARCHes do call smp_shutdown_nonboot_cpus() in machine_shutdown()
> > > > in kexec reboot code path, while x86 doesn't. If I am right, you may
> > > > need reconsider if this patch is needed or need be adjustd.
> > > > 
> > > Citing the code piece in kernel_kexec()
> > > 
> > >                 migrate_to_reboot_cpu();
> > > 
> > >                 /*
> > >                  * migrate_to_reboot_cpu() disables CPU hotplug assuming that
> > >                  * no further code needs to use CPU hotplug (which is true in
> > >                  * the reboot case). However, the kexec path depends on using
> > >                  * CPU hotplug again; so re-enable it here.
> > >                  */
> > >                 cpu_hotplug_enable();
> > >                 pr_notice("Starting new kernel\n");
> > >                 machine_shutdown();
> > > 
> > > So maybe it can be considered in such way: "cpu_hotplug_enable()" is not
> > > needed by x86 and ppc, so this patch removes it, while re-displace it in
> > > a more appropriate place for arm64/riscv ...
> > 
> > OK, so the thing is:
> > 
> > ==
> > In the current code of kexec, we disable cpu hotplug and check reboot cpu
> > validity in migrate_to_reboot_cpu(), then enable cpu hotplug. Then in
> > machine_shutdown()->smp_shutdown_nonboot_cpus(), check the reboot cpu and
> > disable cpu hotplug again.
>   ^^^
> No, there is no any new call to cpu_hotplug_disable() after
> migrate_to_reboot_cpu(). smp_shutdown_nonboot_cpus() just leaves
> cpu_hotplug_disabled==0 as it is.

Hmm, there's 'cpu_hotplug_disabled++;' at the end of
smp_shutdown_nonboot_cpus(). It's disabling cpu hotplug again, I think.

> > 
> > In this patch, it disables cpu hotplug in migrate_to_reboot_cpu() and
> > keep it till entering into smp_shutdown_nonboot_cpus() to shutdown all
> > other cpu with hotplug mechanism, then disable it again. With this
>                                          ^^^
> Here is enable, i.e. smp_shutdown_nonboot_cpus() makes
> cpu_hotplug_disabled switch from 1 to 0, so the following fn can work

Right, you keep the cpu hotplug disabled till in
smp_shutdown_nonboot_cpus(), then enable it to shut down all other cpus.

> ||	static int cpu_down_maps_locked(unsigned int cpu, enum cpuhp_state target)
> ||	{
> ||		if (cpu_hotplug_disabled)
> ||			return -EBUSY;
> ||		return _cpu_down(cpu, 0, target);
> ||	}
> 
> > change, it won't need to double check the reboot cpu validity. 
> > 
> > This change only affect ARCHes relying on hotplug to shutdown cpu before
> > kexec reboot, e.g arm64, risc-v. Other ARCH like x86 is not affected.
> 
> Yes.
> > ==
> > 
> > Do I got it right?
> > 
> Here, neither the original code nor this patch has another
> cpu_hotplug_disable(), the only change is the shift of cpu_hotplug_enable() to an onwards place.
> 
> And I think you should get my idea except this.
> 
> Thanks,
> 
> 	Pingfan
>