From: Pingfan Liu <kernelfans@gmail.com>
To: Baoquan He <bhe@redhat.com>
Cc: linux-kernel@vger.kernel.org,
Eric Biederman <ebiederm@xmission.com>,
Peter Zijlstra <peterz@infradead.org>,
Thomas Gleixner <tglx@linutronix.de>,
Valentin Schneider <valentin.schneider@arm.com>,
Vincent Donnefort <vincent.donnefort@arm.com>,
Ingo Molnar <mingo@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
YueHaibing <yuehaibing@huawei.com>,
Baokun Li <libaokun1@huawei.com>,
Randy Dunlap <rdunlap@infradead.org>,
kexec@lists.infradead.org
Subject: Re: [PATCHv2] kexec: disable cpu hotplug until the rebooting cpu is stable
Date: Wed, 9 Feb 2022 15:31:46 +0800 [thread overview]
Message-ID: <YgNt4slCVxHRex8z@piliu.users.ipa.redhat.com> (raw)
In-Reply-To: <YgI5AFoL4b/5MJEp@MiWiFi-R3L-srv>
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
next prev parent reply other threads:[~2022-02-09 7:32 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-27 9:02 [PATCHv2] kexec: disable cpu hotplug until the rebooting cpu is stable Pingfan Liu
2022-01-27 9:41 ` Baoquan He
2022-01-28 7:41 ` Pingfan Liu
2022-02-08 9:33 ` Baoquan He
2022-02-09 7:31 ` Pingfan Liu [this message]
2022-02-09 8:44 ` Baoquan He
2022-02-14 2:38 ` Pingfan Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YgNt4slCVxHRex8z@piliu.users.ipa.redhat.com \
--to=kernelfans@gmail.com \
--cc=bhe@redhat.com \
--cc=ebiederm@xmission.com \
--cc=kexec@lists.infradead.org \
--cc=libaokun1@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=rdunlap@infradead.org \
--cc=tglx@linutronix.de \
--cc=valentin.schneider@arm.com \
--cc=vincent.donnefort@arm.com \
--cc=yuehaibing@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).