Re: [PATCH v16 3/8] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Mar 21 2023 at 19:40, Usama Arif wrote:
>  void bringup_nonboot_cpus(unsigned int setup_max_cpus)
>  {
> +	unsigned int n = setup_max_cpus - num_online_cpus();
>  	unsigned int cpu;
>  
> +	/*
> +	 * An architecture may have registered parallel pre-bringup states to
> +	 * which each CPU may be brought in parallel. For each such state,
> +	 * bring N CPUs to it in turn before the final round of bringing them
> +	 * online.
> +	 */
> +	if (n > 0) {
> +		enum cpuhp_state st = CPUHP_BP_PARALLEL_DYN;
> +
> +		while (st <= CPUHP_BP_PARALLEL_DYN_END && cpuhp_hp_states[st].name) {


There is no point in special casing this. All architectures can invoke
the CPUHP_BP_* states before CPUHP_BRINGUP_CPU for each to be brought up
CPU first. So this can be made unconditional and common exercised code.

Aside of that this dynamic state range is pretty pointless. There is
really nothing dynamic here and there is no real good reason to have
four dynamic parallel states just because.

The only interesting thing after CPUHP_BP_PREPARE_DYN_END and before
CPUHP_BP_BRINGUP is a state which kicks the AP into life, i.e. we can
just hardcode that as CPUHP_BP_PARALLEL_STARTUP.

Thanks,

        tglx
---
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -133,6 +133,20 @@ enum cpuhp_state {
 	CPUHP_MIPS_SOC_PREPARE,
 	CPUHP_BP_PREPARE_DYN,
 	CPUHP_BP_PREPARE_DYN_END		= CPUHP_BP_PREPARE_DYN + 20,
+	/*
+	 * This is an optional state if the architecture supports parallel
+	 * startup. It's used to send the startup IPI so that the APs can
+	 * run in parallel through the low level startup code instead of
+	 * sending the IPIs one by one in CPUHP_BRINGUP_CPU. This avoids
+	 * waiting for the AP to react and shortens the serialized bringup.
+	 */
+	CPUHP_BP_PARALLEL_STARTUP,
+
+	/*
+	 * Fully per AP serialized bringup from here on. If the
+	 * architecture does no register the CPUHP_BP_PARALLEL_STARTUP
+	 * state, this step sends the startup IPI first.
+	 */
 	CPUHP_BRINGUP_CPU,
 
 	/*
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1504,13 +1504,49 @@ int bringup_hibernate_cpu(unsigned int s
 
 void bringup_nonboot_cpus(unsigned int setup_max_cpus)
 {
-	unsigned int cpu;
+	unsigned int cpu, n = 1;
 
+	/*
+	 * All CHUHP_BP* states are invoked on the control CPU (BP). There
+	 * is no requirement that these states need to be invoked
+	 * sequentially for a particular CPU. They can be invoked state by
+	 * state for each to be onlined CPU.
+	 *
+	 * On architectures which have setup the CPUHP_BP_PARALLEL_STARTUP
+	 * state, this sends the startup IPI to each of the to be onlined
+	 * APs. This avoids waiting for each AP to respond to the startup
+	 * IPI in CPUHP_BRINGUP_CPU. The APs proceed through the low level
+	 * bringup code and then wait for the control CPU to release them
+	 * one by one for the final onlining procedure in the loop below.
+	 *
+	 * For architectures which do not support parallel bringup the
+	 * CPUHP_BP_PARALLEL_STARTUP state is a NOOP and the actual startup
+	 * happens in the CPUHP_BRINGUP_CPU state which is fully serialized
+	 * per CPU in the loop below.
+	 */
+	for_each_present_cpu(cpu) {
+		if (n++ >= setup_max_cpus)
+			break;
+		cpu_up(cpu, CPUHP_BP_PARALLEL_STARTUP);
+	}
+
+	/* Do the per CPU serialized bringup to ONLINE state */
 	for_each_present_cpu(cpu) {
 		if (num_online_cpus() >= setup_max_cpus)
 			break;
-		if (!cpu_online(cpu))
-			cpu_up(cpu, CPUHP_ONLINE);
+
+		if (!cpu_online(cpu)) {
+			struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
+			int ret = cpu_up(cpu, CPUHP_ONLINE);
+
+			/*
+			 * Due to the above preparation loop a failed online attempt
+			 * has only rolled back to CPUHP_BP_PARALLEL_STARTUP. Do the
+			 * remaining cleanups.
+			 */
+			if (ret && can_rollback_cpu(st))
+				WARN_ON(cpuhp_invoke_callback_range(false, cpu, st, CPUHP_OFFLINE));
+		}
 	}
 }