All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: David Woodhouse <dwmw2@infradead.org>,
	Usama Arif <usama.arif@bytedance.com>,
	kim.phillips@amd.com, brgerst@gmail.com
Cc: piotrgorski@cachyos.org, oleksandr@natalenko.name,
	arjan@linux.intel.com, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, hpa@zytor.com, x86@kernel.org,
	pbonzini@redhat.com, paulmck@kernel.org,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	rcu@vger.kernel.org, mimoja@mimoja.de, hewenliang4@huawei.com,
	thomas.lendacky@amd.com, seanjc@google.com,
	pmenzel@molgen.mpg.de, fam.zheng@bytedance.com,
	punit.agrawal@bytedance.com, simon.evans@bytedance.com,
	liangma@liangbit.com, gpiccoli@igalia.com
Subject: Re: [PATCH v16 3/8] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU
Date: Fri, 24 Mar 2023 14:57:13 +0100	[thread overview]
Message-ID: <87pm8y6yme.ffs@tglx> (raw)
In-Reply-To: <115b39e0226915b8f69ea0cce2487588f6010995.camel@infradead.org>

On Fri, Mar 24 2023 at 09:31, David Woodhouse wrote:
> On Fri, 2023-03-24 at 02:16 +0100, Thomas Gleixner wrote:
>> So the proper thing is to split CPUHP_BRINGUP_CPU, which is the bridging
>> state between AP and BP today, into a set of synchronization points
>> between BP and AP.
>
> I feel we're talking past each other a little bit. Because I'd have
> said that's *exactly* what this patch set is doing.
>
> The existing x86 CPUHP_BRINGUP_CPU step in native_cpu_up() has a bunch
> of back-and-forth between BP and AP, which you've enumerated below and
> which I cleaned up and commented in the 'Split up native_cpu_up into
> separate phases and document them' patch.
>
> This patch set literally introduces the new PARALLEL_DYN states to be
> "a set of synchronization points between BP and AP", and then makes the
> x86 code utilise that for the *first* of its back-and-forth exchanges
> between BP and AP.

It provides a dynamic space which is absolutely unspecified and just
opens the door for tinkering.

This first step does not even contain a synchronization point. All it
does is to kick the AP into gear. Not more, not less. Naming this
obscurely as PARALLEL_DYN is a tasteless bandaid.

If you go and look at all __cpu_up() implementations, then you'll notice
that these are very similar. All of them do

   1) Kick AP
   2) Synchronize at least once between BP and AP

There is nothing x86 specific about this. So instead of hiding this
behind a misnomed dynamic space, the obviously correct solution is to
make this an explicit mechanism in the core code and convert _all_
architectures over to this scheme. That's in the first place completely
independent of parallel bringup.

It has a value on it's own as it consolidates the zoo of synchronization
mechanisms (cpumasks, completions, random variables) into one shared
mechanism in the core code.

That's very much different from what your patch is doing. And there is a
very good reason aside of consolidation to do so:

This prepares to handle the parallelism requirements in the core code
instead of letting each architecture come up with its own set of
magic. Which in turn is a prerequisite for allowing the STARTING
callbacks or later the threaded AP states to execute in parallel.

Why? Simply because of this:

  BP			AP              state
  kick()                                BRINGUP_CPU
                        startup()                      
  sync()                sync() 
                        starting()      advances to AP_ONLINE
  sync()                sync()
  TSC_sync()            TSC_sync()
  wait_for_online()     set_online()
                        cpu_startup_entry() AP_ONLINE_IDLE
  wait_for_completion() complete()

This works correctly today because bringup_cpu() does not modify state
and excpects the state to be advanced by the AP once the completion is
done.

So you _cannot_ just throw some magic dynamic states before BRINGUP_CPU
and then expect that the state machine is consistent when the AP is
allowed to run the starting callbacks in parallel.

The sync point after the starting callbacks simply cannot be in that
dynamic state space. It has to be _after_ the AP starting states.

That needs a fundamental change in the way how the state management
at this point works and this needs to be done upfront. Aside of the
general serialization aspects this needs some deep thought whether the
BP control can stay single threaded or if it's required to spawn a
control thread per AP.

The kick CPU into life state is completely independent of the above and
can be moved just before BRINGUP_CPU without violating anything. But
that's where the easy to solve part stops hard.

You might find my wording offensive, but I perceive your "let's use a
few dynamic states and see what sticks" approach offensive too.

The analysis of all this is not rocket science and could have been done
long ago by yourself.

Thanks,

        tglx

  reply	other threads:[~2023-03-24 13:57 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-21 19:40 [PATCH v16 0/8] Parallel CPU bringup for x86_64 Usama Arif
2023-03-21 19:40 ` [PATCH v16 1/8] cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h> Usama Arif
2023-03-21 19:40 ` [PATCH v16 2/8] cpu/hotplug: Reset task stack state in _cpu_up() Usama Arif
2023-03-22 11:06   ` Mark Rutland
2023-03-21 19:40 ` [PATCH v16 3/8] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU Usama Arif
2023-03-23 22:36   ` Thomas Gleixner
2023-03-23 22:49     ` David Woodhouse
2023-03-23 23:48       ` Thomas Gleixner
2023-03-24  0:06         ` David Woodhouse
2023-03-23 23:05     ` Thomas Gleixner
2023-03-23 23:12       ` David Woodhouse
2023-03-24  1:16         ` Thomas Gleixner
2023-03-24  9:31           ` David Woodhouse
2023-03-24 13:57             ` Thomas Gleixner [this message]
2023-03-24 15:33               ` David Woodhouse
2023-03-24 14:07             ` Thomas Gleixner
2023-03-24  9:46           ` Thomas Gleixner
2023-03-24 10:00             ` David Woodhouse
2023-03-27 18:48       ` David Woodhouse
2023-03-21 19:40 ` [PATCH v16 4/8] x86/smpboot: Split up native_cpu_up into separate phases and document them Usama Arif
2023-03-21 19:40 ` [PATCH v16 5/8] x86/smpboot: Support parallel startup of secondary CPUs Usama Arif
2023-03-21 19:40 ` [PATCH v16 6/8] x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel Usama Arif
2023-03-21 19:40 ` [PATCH v16 7/8] x86/smpboot: Serialize topology updates for secondary bringup Usama Arif
2023-03-21 19:40 ` [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES Usama Arif
2023-03-22 22:47   ` Borislav Petkov
2023-03-23  8:32     ` David Woodhouse
2023-03-23  8:51       ` Borislav Petkov
2023-03-23  9:04         ` David Woodhouse
2023-03-23 14:23           ` Brian Gerst
2023-03-27 17:47             ` Borislav Petkov
2023-03-27 18:14               ` David Woodhouse
2023-03-27 19:14                 ` Tom Lendacky
2023-03-27 19:32                 ` Borislav Petkov
2023-03-23 13:16     ` Tom Lendacky
     [not found]       ` <751f572f940220775054dc09324b20b929d7d66d.camel@amazon.co.uk>
2023-03-23 18:28         ` Tom Lendacky
2023-03-23 22:13           ` Borislav Petkov
2023-03-23 22:30             ` [EXTERNAL][PATCH " David Woodhouse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pm8y6yme.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=arjan@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=brgerst@gmail.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=dwmw2@infradead.org \
    --cc=fam.zheng@bytedance.com \
    --cc=gpiccoli@igalia.com \
    --cc=hewenliang4@huawei.com \
    --cc=hpa@zytor.com \
    --cc=kim.phillips@amd.com \
    --cc=kvm@vger.kernel.org \
    --cc=liangma@liangbit.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mimoja@mimoja.de \
    --cc=mingo@redhat.com \
    --cc=oleksandr@natalenko.name \
    --cc=paulmck@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=piotrgorski@cachyos.org \
    --cc=pmenzel@molgen.mpg.de \
    --cc=punit.agrawal@bytedance.com \
    --cc=rcu@vger.kernel.org \
    --cc=seanjc@google.com \
    --cc=simon.evans@bytedance.com \
    --cc=thomas.lendacky@amd.com \
    --cc=usama.arif@bytedance.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.