From:   Thomas Gleixner <tglx@linutronix.de>
To:     David Woodhouse <dwmw2@infradead.org>,
        Usama Arif <usama.arif@bytedance.com>, arjan@linux.intel.com
Cc:     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
Subject: Re: [PATCH v6 01/11] x86/apic/x2apic: Fix parallel handling of
 cluster_mask
In-Reply-To: <921cfe295fcd398168e5454e01193045de312688.camel@infradead.org>
References: <20230202215625.3248306-1-usama.arif@bytedance.com>
 <20230202215625.3248306-2-usama.arif@bytedance.com> <87a61qxtx0.ffs@tglx>
 <d37f3af69df09ff542024ed93a37865b28dfa86e.camel@infradead.org>
 <921cfe295fcd398168e5454e01193045de312688.camel@infradead.org>
Date:   Tue, 07 Feb 2023 15:24:38 +0100
Message-ID: <87v8kdv9i1.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Feb 07 2023 at 11:27, David Woodhouse wrote:
> On Tue, 2023-02-07 at 10:57 +0000, David Woodhouse wrote:
>> =C2=A0=E2=80=A2 This CPU was present but no other CPU in this cluster wa=
s actually
>> =C2=A0=C2=A0 brought up at boot time so the cluster_mask wasn't allocate=
d.
>>=20
>> 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=E2=86=92possible),=
 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.

Indeed.

> 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?

Yes. Virtualization does aweful things....

> 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) {

Looks good!

      tglx