Re: [PATCH] x86, sched: Allow NUMA nodes to share an LLC on Intel platforms

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Feb 10, 2021 at 07:22:03AM -0800, Dave Hansen wrote:
> On 2/10/21 12:05 AM, Peter Zijlstra wrote:
> >> +	if (IS_ENABLED(CONFIG_NUMA))
> >> +		set_cpu_bug(c, X86_BUG_NUMA_SHARES_LLC);
> >>  }
> > This seens wrong too, it shouldn't be allowed pre SKX. And ideally only
> > be allowed when SNC is enabled.
> 
> Originally, this just added a few more models to the list of CPUs with
> SNC.  I was hoping for something a bit more durable that we wouldn't
> have to go back and poke at every year or two.

It's not like we don't have to update a gazillion FMS tables for each
new instance anyway :-(

> > Please make this more specific than: all Intel CPUs. Ofcourse, since you
> > all knew this was an issue, you could've made it discoverable
> > _somewhere_ :-(
> 
> You're totally right, of course.  The hardware could enumerate SNC as a
> feature explicitly somewhere.  But, that's a little silly because all of
> the information that it's enumerating about the CPU caches and NUMA
> nodes present and correct is *correct*.  The secondary information would
> only be for the CPU to say, "yeah, I'm really sure about that other stuff".
> 
> I think this sanity check has outlived its usefulness.

Maybe BIOS monkeys got better, but I'm not sure I trust it all.

So SNC is all on-package, do all those nodes have the same pkg id? That
is, I'm trying to find something to restrict topological madness.


diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 88cd0064d1f8..de1010dd0bba 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -458,6 +458,26 @@ static bool match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
 	return false;
 }
 
+static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if ((c->phys_proc_id == o->phys_proc_id) &&
+		(c->cpu_die_id == o->cpu_die_id))
+		return true;
+	return false;
+}
+
+/*
+ * Unlike the other levels, we do not enforce keeping a
+ * multicore group inside a NUMA node.  If this happens, we will
+ * discard the MC level of the topology later.
+ */
+static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if (c->phys_proc_id == o->phys_proc_id)
+		return true;
+	return false;
+}
+
 /*
  * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
  *
@@ -495,33 +515,12 @@ static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
 	 * means 'c' does not share the LLC of 'o'. This will be
 	 * reflected to userspace.
 	 */
-	if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
+	if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu) && match_pkg(c, o))
 		return false;
 
 	return topology_sane(c, o, "llc");
 }
 
-/*
- * Unlike the other levels, we do not enforce keeping a
- * multicore group inside a NUMA node.  If this happens, we will
- * discard the MC level of the topology later.
- */
-static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
-{
-	if (c->phys_proc_id == o->phys_proc_id)
-		return true;
-	return false;
-}
-
-static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
-{
-	if ((c->phys_proc_id == o->phys_proc_id) &&
-		(c->cpu_die_id == o->cpu_die_id))
-		return true;
-	return false;
-}
-
-
 #if defined(CONFIG_SCHED_SMT) || defined(CONFIG_SCHED_MC)
 static inline int x86_sched_itmt_flags(void)
 {