linux-kernel - Re: [PATCH v16 3/8] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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)