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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

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,
>  
>         /*

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]