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@xxxxxxxxxxxxx/
> 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:
smime.p7s
Description: S/MIME cryptographic signature
