Re: [External] Re: [PATCH v4 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

On Wed, 2023-02-01 at 08:55 -0800, H. Peter Anvin wrote:
> On February 1, 2023 8:38:14 AM PST, Usama Arif <usama.arif@xxxxxxxxxxxxx> wrote:
> > 
> > 
> > On 01/02/2023 15:08, David Woodhouse wrote:
> > > On Wed, 2023-02-01 at 14:40 +0000, Usama Arif wrote:
> > > > On 01/02/2022 20:53, David Woodhouse wrote:
> > > > > Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
> > > > > them shaves about 80% off the AP bringup time on a 96-thread 2-socket
> > > > > Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
> > > > > 
> > > > > There are more wins to be had with further parallelisation, but this is
> > > > > the simple part.
> > > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > We are interested in reducing the boot time of servers (with kexec), and
> > > > smpboot takes up a significant amount of time while booting. When
> > > > testing the patch series (rebased to v6.1) on a server with 128 CPUs
> > > > split across 2 NUMA nodes, it brought down the smpboot time from ~700ms
> > > > to 100ms. Adding another cpuhp state for do_wait_cpu_initialized to make
> > > > sure cpu_init is reached (as done in v1 of the series + using the
> > > > cpu_finishup_mask) brought it down further to ~30ms.
> > > > 
> > > > I just wanted to check what was needed to progress the patch series
> > > > further for review? There weren't any comments on v4 of the patch so I
> > > > couldn't figure out what more is needed. I think its quite useful to
> > > > have this working so would be really glad help in anything needed to
> > > > restart the review.
> > > 
> > > 
> > > I believe the only thing holding it back was the fact that it broke on
> > > some AMD CPUs.
> > > 
> > > We don't *think* there are any remaining software issues; we think it's
> > > hardware. Either an actual hardware race in CPU or chipset, or perhaps
> > > even something as simple as a voltage regulator which can't cope with
> > > an increase in power draw from *all* the CPUs at the same time.
> > > 
> > > We have prodded AMD a few times to investigate, but so far to no avail.
> > > 
> > > Last time I actually spoke to Thomas in person, I think he agreed that
> > > we should just merge it and disable the parallel mode for the affected
> > > AMD CPUs.
> > > 
> > 
> > From the comments in v3, it seems to affect multiple generations, would it be worth proceeding with the patches by disabling it on all AMD CPUs to be on the safe side, until the actual issue is found and what causes it, and then follow up later if the issue is found by disabling it only on affected cpus. Maybe simply do something like below?
> > 
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index 0f144773a7fc..6b8884592341 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1575,7 +1575,8 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
> >         * for SEV-ES guests because they can't use CPUID that early.
> >         */
> >        if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 0x0B ||
> > -           cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> > +           cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) ||
> > +           boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> >                do_parallel_bringup = false;
> > 
> >        if (do_parallel_bringup) {
> > 
> > 
> > 
> > 
> > > If you've already rebased to a newer kernel and tested it, perhaps now
> > > is the time to do just that.
> > 
> > If you would like me to repost the rebased patches to restart the reviews (with do_parallel_bringup disabled for AMD), please let me know!
> > 

Sounds like you have a far fresher context on it all than I do now, so
yes please that sounds like a great idea.

I think we still need a sign-off from Thomas on the real mode patch but
as I noted in the last cover letter, now we've *fixed* it perhaps we
can persuade him to concede that it's his? Either that or we post it in
email and hope to trick him into adding a S-o-B in transit as he
applies it...

> > Thanks,
> > Usama
> 
> This should be a CPU bug flag in my option.

Yeah, probably true. But I think I agree with Usama that we should do
it for all AMD to start with. Best to err on the side of caution.

Attachment: smime.p7s
Description: S/MIME cryptographic signature