From:   Thomas Gleixner <tglx@linutronix.de>
To:     Usama Arif <usama.arif@bytedance.com>, dwmw2@infradead.org,
        arjan@linux.intel.com
Cc:     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, David Woodhouse <dwmw@amazon.co.uk>
Subject: Re: [PATCH v6 03/11] cpu/hotplug: Add dynamic parallel bringup
 states before CPUHP_BRINGUP_CPU
In-Reply-To: <20230202215625.3248306-4-usama.arif@bytedance.com>
References: <20230202215625.3248306-1-usama.arif@bytedance.com>
 <20230202215625.3248306-4-usama.arif@bytedance.com>
Date:   Tue, 07 Feb 2023 00:43:45 +0100
Message-ID: <874jryxsum.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Feb 02 2023 at 21:56, Usama Arif wrote:
> So any combination of prepare/start calls which depend on A-B ordering
> for each CPU in turn, such as the X2APIC code which used to allocate a
> cluster mask 'just in case' and store it in a global variable in the
> prep stage, then potentially consume that preallocated structure from
> the AP and set the global pointer to NULL to be reallocated in
> CPUHP_X2APIC_PREPARE for the next CPU... would explode horribly.
>
> We believe that X2APIC was the only such case, for x86. But this is why
> it remains an architecture opt-in. For now.

We believe is not really a convincing technical argument.

  On x86 the X2APIC case was established to be the only one which relied
  on the full CPU hotplug serialization. It's unclear whether this
  affects any other architecture, so this optimization is opt-in.

Hmm?

> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6c0a92ca6bb5..5a8f1a93b57c 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1505,6 +1505,24 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
>  void bringup_nonboot_cpus(unsigned int setup_max_cpus)
>  {
>  	unsigned int cpu;
> +	int n =3D setup_max_cpus - num_online_cpus();

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable=
-declarations

Aside of that I detest the mixture between unsigned and signed here.

> +
> +	/* =E2=88=80 parallel pre-bringup state, bring N CPUs to it */
> +	if (n > 0) {
> +		enum cpuhp_state st =3D CPUHP_BP_PARALLEL_DYN;
> +
> +		while (st <=3D CPUHP_BP_PARALLEL_DYN_END &&
> +		       cpuhp_hp_states[st].name) {

		while (st <=3D CPUHP_BP_PARALLEL_DYN_END && cpuhp_hp_states[st].name) {

80 character limit has been updated quite some time ago

Thanks,

        tglx