[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <921cfe295fcd398168e5454e01193045de312688.camel@infradead.org>
Date: Tue, 07 Feb 2023 11:27:20 +0000
From: David Woodhouse <dwmw2@...radead.org>
To: Thomas Gleixner <tglx@...utronix.de>,
Usama Arif <usama.arif@...edance.com>, arjan@...ux.intel.com
Cc: 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
Subject: Re: [PATCH v6 01/11] x86/apic/x2apic: Fix parallel handling of
cluster_mask
On Tue, 2023-02-07 at 10:57 +0000, David Woodhouse wrote:
>
> > + /*
> > + * On post boot hotplug iterate over the present CPUs to handle the
> > + * case of partial clusters as they might be presented by
> > + * virtualization.
> > + */
> > + for_each_present_cpu(cpu_i) {
>
>
> So... if this CPU was *present* at boot time (and if any other CPU in
> this cluster was present), it will already have a cluster_mask.
>
> Which means we get here in two cases:
>
> • This CPU wasn't actually present (was just 'possible') at boot time.
> (Is that actually a thing that happens?)
>
> • This CPU was present but no other CPU in this cluster was actually
> brought up at boot time so the cluster_mask wasn't allocated.
>
> The code looks right, I don't grok the comment about partial clusters
> and virtualization, and would have worded it something along the above
> lines?
As I get my head around that, I think the code needs to change too.
What if we *unplug* the only CPU in a cluster (present→possible), then
add a new one in the same cluster? The new one would get a new
cluster_mask. Which is kind of OK for now but then if we re-add the
original CPU it'd continue to use its old cluster_mask.
Now, that's kind of weird if it's physical CPUs because that cluster is
within a given chip, isn't it? But with virtualization maybe that's
something that could happen, and it doesn't hurt to be completely safe
by using for_each_possible_cpu() instead?
Now looks like this:
/*
* On post boot hotplug for a CPU which was not present at boot time,
* iterate over all possible CPUs (even those which are not present
* any more) to find any existing cluster mask.
*/
for_each_possible_cpu(cpu_i) {
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)
Powered by blists - more mailing lists