[PATCH RFC v2] reboot: hotplug cpus in migrate_to_reboot_cpu()

[PATCH RFC v2] reboot: hotplug cpus in migrate_to_reboot_cpu()

[Hsin-Yi Wang]

Currently system reboots would use arch specific codes (eg. smp_send_stop)
to offline non reboot cpus. Some arch like arm64, arm, and x86... would set
offline masks to cpu without really offline them. Thus causing some race
condition and kernel warning comes out sometimes when system reboots. We
can do cpu hotplug in migrate_to_reboot_cpu() to avoid this issue.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
change log:
v1 --> v2:
Call hotplug function in #ifdef CONFIG instead of IS_ENABLED.
(Reported-by: kbuild test robot <lkp@intel.com>)

kernel warnings at reboot:
[1] https://lore.kernel.org/lkml/20190820100843.3028-1-hsinyi@chromium.org/
[2] https://lore.kernel.org/lkml/20190727164450.GA11726@roeck-us.net/
---
 include/linux/cpu.h |  1 +
 kernel/cpu.c        | 17 +++++++++++++++++
 kernel/reboot.c     |  8 ++++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index d0633ebdaa9c..20a4036f24ad 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -113,6 +113,7 @@ 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);
+extern void offline_secondary_cpus(int primary);
 
 #else /* CONFIG_HOTPLUG_CPU */
 
diff --git a/kernel/cpu.c b/kernel/cpu.c
index fc28e17940e0..00b2be6a662d 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1058,6 +1058,23 @@ int cpu_down(unsigned int cpu)
 }
 EXPORT_SYMBOL(cpu_down);
 
+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();
+}
 #else
 #define takedown_cpu		NULL
 #endif /*CONFIG_HOTPLUG_CPU*/
diff --git a/kernel/reboot.c b/kernel/reboot.c
index c4d472b7f1b4..fdf6730f8a64 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -7,6 +7,7 @@
 
 #define pr_fmt(fmt)	"reboot: " fmt
 
+#include <linux/cpu.h>
 #include <linux/ctype.h>
 #include <linux/export.h>
 #include <linux/kexec.h>
@@ -220,8 +221,6 @@ void migrate_to_reboot_cpu(void)
 	/* The boot cpu is always logical cpu 0 */
 	int cpu = reboot_cpu;
 
-	cpu_hotplug_disable();
-
 	/* Make certain the cpu I'm about to reboot on is online */
 	if (!cpu_online(cpu))
 		cpu = cpumask_first(cpu_online_mask);
@@ -231,6 +230,11 @@ void migrate_to_reboot_cpu(void)
 
 	/* Make certain I only run on the appropriate processor */
 	set_cpus_allowed_ptr(current, cpumask_of(cpu));
+
+	/* Hotplug other cpus if possible */
+#ifdef CONFIG_HOTPLUG_CPU
+	offline_secondary_cpus(cpu);
+#endif
 }
 
 /**
-- 
2.23.0.700.g56cf767bdb-goog

Re: [PATCH RFC v2] reboot: hotplug cpus in migrate_to_reboot_cpu()

[Thomas Gleixner]

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

> @@ -220,8 +221,6 @@ void migrate_to_reboot_cpu(void)
>  	/* The boot cpu is always logical cpu 0 */
>  	int cpu = reboot_cpu;
>  
> -	cpu_hotplug_disable();
> -
>  	/* Make certain the cpu I'm about to reboot on is online */
>  	if (!cpu_online(cpu))
>  		cpu = cpumask_first(cpu_online_mask);
> @@ -231,6 +230,11 @@ void migrate_to_reboot_cpu(void)
>  
>  	/* Make certain I only run on the appropriate processor */
>  	set_cpus_allowed_ptr(current, cpumask_of(cpu));
> +
> +	/* Hotplug other cpus if possible */
> +#ifdef CONFIG_HOTPLUG_CPU
> +	offline_secondary_cpus(cpu);
> +#endif

In general I like the idea, but shouldn't this remove the architecture
code as a follow up?

Also this needs to be explicitely enabled per architecture (opt-in) and
not as an unconditional operation for all architectures which support
CPU hotplug.

Thanks,

        tglx

Re: [PATCH RFC v2] reboot: hotplug cpus in migrate_to_reboot_cpu()

[Hsin-Yi Wang]

On Fri, Jan 10, 2020 at 4:15 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Hsin-Yi Wang <hsinyi@chromium.org> writes:
>
> > @@ -220,8 +221,6 @@ void migrate_to_reboot_cpu(void)
> >       /* The boot cpu is always logical cpu 0 */
> >       int cpu = reboot_cpu;
> >
> > -     cpu_hotplug_disable();
> > -
> >       /* Make certain the cpu I'm about to reboot on is online */
> >       if (!cpu_online(cpu))
> >               cpu = cpumask_first(cpu_online_mask);
> > @@ -231,6 +230,11 @@ void migrate_to_reboot_cpu(void)
> >
> >       /* Make certain I only run on the appropriate processor */
> >       set_cpus_allowed_ptr(current, cpumask_of(cpu));
> > +
> > +     /* Hotplug other cpus if possible */
> > +#ifdef CONFIG_HOTPLUG_CPU
> > +     offline_secondary_cpus(cpu);
> > +#endif
>
> In general I like the idea, but shouldn't this remove the architecture
> code as a follow up?
>
> Also this needs to be explicitely enabled per architecture (opt-in) and
> not as an unconditional operation for all architectures which support
> CPU hotplug.
>
> Thanks,
>
>         tglx

Thanks for your comment.

I'll make this another config and depends on HOTPLUG_CPU.

I did some check on how architectures implement smp_send_stop. Most
architecture would loop through all other cpus in cpu_online_mask and
call ipi stop to each. If they enable this config, the loop would be
empty loop. In case they don't enable this config, they still need
original smp_send_stop (or as a fallback if some cpus fails to
offline).
Currently there are 2 architectures (csky, alpha) that call ipi on
possible cpus (instead of online cpus). Besides this, I'm not sure if
there's other architecture code that should be cleaned up.

Thanks,

Hsin-Yi