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

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Mar 23 2023 at 23:36, Thomas Gleixner wrote:
> 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.

Bah. There is. We discussed that before. Architectures need to opt in to
make sure that there are no implicit dependencies on the full
serialization.

Still the rest can be simplified as below.

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,45 @@ int bringup_hibernate_cpu(unsigned int s
 
 void bringup_nonboot_cpus(unsigned int setup_max_cpus)
 {
-	unsigned int cpu;
+	unsigned int cpu, n = 1;
 
+	/*
+	 * On architectures which have setup the CPUHP_BP_PARALLEL_STARTUP
+	 * state, this invokes all BP prepare states and the parallel
+	 * startup state 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 all
+	 * states are fully serialized in the loop below.
+	 */
+	if (!cpuhp_step_empty(true, CPUHP_BP_PARALLEL_STARTUP) {
+			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
+			 * might have only rolled back to CPUHP_BP_PARALLEL_STARTUP. Do the
+			 * remaining cleanups. NOOP for the non parallel case.
+			 */
+			if (ret && can_rollback_cpu(st))
+				WARN_ON(cpuhp_invoke_callback_range(false, cpu, st, CPUHP_OFFLINE));
+		}
 	}
 }