[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <47150207bfb76bb98aff678cf7c91f245e9f5dd9.camel@infradead.org>
Date: Thu, 23 Mar 2023 22:49:40 +0000
From: David Woodhouse <dwmw2@...radead.org>
To: Thomas Gleixner <tglx@...utronix.de>,
Usama Arif <usama.arif@...edance.com>, kim.phillips@....com,
brgerst@...il.com
Cc: piotrgorski@...hyos.org, oleksandr@...alenko.name,
arjan@...ux.intel.com, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, hpa@...or.com, x86@...nel.org,
pbonzini@...hat.com, paulmck@...nel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
rcu@...r.kernel.org, mimoja@...oja.de, hewenliang4@...wei.com,
thomas.lendacky@....com, seanjc@...gle.com, pmenzel@...gen.mpg.de,
fam.zheng@...edance.com, punit.agrawal@...edance.com,
simon.evans@...edance.com, liangma@...ngbit.com,
gpiccoli@...lia.com
Subject: Re: [PATCH v16 3/8] cpu/hotplug: Add dynamic parallel bringup
states before CPUHP_BRINGUP_CPU
On Thu, 2023-03-23 at 23:36 +0100, 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.
>
There were three paragraphs in the commit message explaining why I
didn't want to do that. It didn't work for x86 before I started, and I
haven't reviewed *every* other architecture to ensure that it will work
there. It was opt-in for a reason. :)
> 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 original patch set did use more than one state; the plan to do
microcode updates in parallel does involve doing at least one more, I
believe.
https://lore.kernel.org/all/eb6717dfc4ceb99803c0396f950db7c3231c75ef.camel@infradead.org/
> 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.
> + */
Not sure I'd conceded that yet; the APs do their *own* bringup from
here, and that really ought to be able to run in parallel.
> CPUHP_BRINGUP_CPU,
>
> /*
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)
Powered by blists - more mailing lists