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.
>
> 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
|| 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
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
