Re: [PATCH v15 03/12] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU

From: Thomas Gleixner <tglx@linutronix.de>
To: Usama Arif <usama.arif@bytedance.com>,
	dwmw2@infradead.org, kim.phillips@amd.com, brgerst@gmail.com
Cc: piotrgorski@cachyos.org, oleksandr@natalenko.name,
	arjan@linux.intel.com, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, hpa@zytor.com, x86@kernel.org,
	pbonzini@redhat.com, paulmck@kernel.org,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	rcu@vger.kernel.org, mimoja@mimoja.de, hewenliang4@huawei.com,
	thomas.lendacky@amd.com, seanjc@google.com,
	pmenzel@molgen.mpg.de, fam.zheng@bytedance.com,
	punit.agrawal@bytedance.com, simon.evans@bytedance.com,
	liangma@liangbit.com, gpiccoli@igalia.com,
	David Woodhouse <dwmw@amazon.co.uk>,
	Usama Arif <usama.arif@bytedance.com>
Subject: Re: [PATCH v15 03/12] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU
Date: Mon, 20 Mar 2023 15:30:40 +0100	[thread overview]
Message-ID: <87r0tja41b.ffs@tglx> (raw)
In-Reply-To: <20230316222109.1940300-4-usama.arif@bytedance.com>

On Thu, Mar 16 2023 at 22:21, Usama Arif wrote:
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6b3dccb4a888..6ccc64defd47 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1497,8 +1497,30 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
>  
>  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) {
> +			int i = n;
> +
> +			for_each_present_cpu(cpu) {
> +				cpu_up(cpu, st);
> +				if (!--i)
> +					break;
> +			}
> +			st++;
> +		}
> +	}
> +
>  	for_each_present_cpu(cpu) {
>  		if (num_online_cpus() >= setup_max_cpus)
>  			break;

This causes a subtle issue. The bringup loop above moves all CPUs to
cpuhp_state == CPUHP_BP_PARALLEL_DYN_END. So the serial bootup will
start from there and bring them fully up.

Now if a bringup fails, then the rollback will only go back down to
CPUHP_BP_PARALLEL_DYN_END, which means that the control CPU won't do any
cleanups below CPUHP_BP_PARALLEL_DYN_END.

That 'fail' is a common case for SMT soft disable via the 'nosmt'
command line parameter. Due to the marvelous MCE broadcast 'feature' we
need to bringup the SMT siblings at least to the CPUHP_AP_ONLINE_IDLE
state once and then roll them back.

While this is not necessarily a fatal problem, it's changing behaviour
and with quite some of the details hidden in the (then not issued)
teardown callbacks might cause some hard to decode subtle surprises.

So that second for_each_present_cpu() loop needs to check the return
value of cpu_up() and issue a full rollback to CPUHP_OFFLINE in case of
fail.

Thanks,

        tglx