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

[Alison Schofield]

On Tue, Feb 16, 2021 at 12:29:02PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 10, 2021 at 02:11:34PM -0800, Alison Schofield wrote:
> 
> > This is equivalent to determining if x86_has_numa_in_package.
> > Do you think there is an opportunity to set x86_has_numa_in_package
> > earlier, and use it here and in set_cpu_sibling_map()?
> 
> Sure. Not sure that's actually clearer though.
> 
> > With that additional info (match_pkg()) how about -
> > 
> > Instead of this:
> > -       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))
> > 
> > Do this:
> > 
> > -       if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
> > +       if (!topology_same_node(c, o) && match_pkg(c, o))
> > 
> > 
> > Looking at Commit 316ad248307f ("sched/x86: Rewrite set_cpu_sibling_map())
> > which reworked topology WARNINGs, the intent was to "make sure to
> > only warn when the check changes the end result"
> > 
> > This check doesn't change the end result. It returns false directly
> > and if it were bypassed completely, it would still return false with
> > a WARNING.
> 
> I'm not following the argument, we explicitly *do* modify the end result
> for those SNC caches. Also, by doing what you propose, we fail to get a
> warning if/when AMD decides to do 'funny' things.

This might be beating a dead horse (me==dead horse) but how do we
modify the end result? That is, I see false returned either way.
Am I missing another code path that considers this info?

> 
> Suppose AMD too thinks this is a swell idea, but they have subtly
> different cache behaviour (just for giggles), then it all goes
> undetected, which would be bad.
>
Understood.

> > If we add that additional match_pkg() check is removing the WARNING for
> > all cases possible?
> 
> How many parts had that Intel Cluster-on-Die thing? Seeing how all the
> new parts have the SNC crud, that seems like a finite list.
> 
> Wikipedia seems to suggest haswell and broadwell were the only onces
> with COD on, skylake and later has the SNC.
> 
> How's something like this then (needs splitting into multiple patches I
> suppose):
> 

This is interesting. Flipping to check COD, instead of SNC would mean
we don't have to revisit this for future SNC CPUs. I don't think it's
worth the risk now (more code changes, more platforms to test).


The list exists. We can add to the list and it makes us think about the
SNC topologies as each new CPU is announced. That is a good nuisance.

Thanks for showing how the COD case would work.


> ---
>  arch/x86/kernel/smpboot.c | 76 +++++++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 117e24fbfd8a..cfe23badf9a3 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -458,8 +458,31 @@ 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.
> + * Define intel_cod_cpu[] for Intel COD (Cluster-on-Die) CPUs.
> + *
> + * Any Intel CPU that has multiple nodes per package and doesn't match this
> + * will have the newer SNC (Sub-NUMA Cluster).
>   *
>   * These are Intel CPUs that enumerate an LLC that is shared by
>   * multiple NUMA nodes. The LLC on these systems is shared for
> @@ -473,14 +496,18 @@ static bool match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>   * NUMA nodes).
>   */
>  
> -static const struct x86_cpu_id snc_cpu[] = {
> -	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
> +static const struct x86_cpu_id intel_cod_cpu[] = {
> +	X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, 0),	/* COD */
> +	X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, 0),	/* COD */
> +	X86_MATCH_INTEL_FAM6_MODEL(ANY, 1),		/* SNC */
>  	{}
>  };
>  
>  static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>  {
> +	const struct x86_cpu_id *id = x86_match_cpu(intel_cod_cpu);
>  	int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
> +	bool intel_snc = id && id->driver_data;
>  
>  	/* Do not match if we do not have a valid APICID for cpu: */
>  	if (per_cpu(cpu_llc_id, cpu1) == BAD_APICID)
> @@ -495,32 +522,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 (match_pkg(c, o) && !topology_same_node(c, o) && intel_snc)
>  		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)
> @@ -592,14 +599,23 @@ void set_cpu_sibling_map(int cpu)
>  	for_each_cpu(i, cpu_sibling_setup_mask) {
>  		o = &cpu_data(i);
>  
> +		if (match_pkg(c, o) && !topology_same_node(c, o))
> +			x86_has_numa_in_package = true;
> +
>  		if ((i == cpu) || (has_smt && match_smt(c, o)))
>  			link_mask(topology_sibling_cpumask, cpu, i);
>  
>  		if ((i == cpu) || (has_mp && match_llc(c, o)))
>  			link_mask(cpu_llc_shared_mask, cpu, i);
>  
> +		if ((i == cpu) || (has_mp && match_die(c, o)))
> +			link_mask(topology_die_cpumask, cpu, i);
>  	}
>  
> +	threads = cpumask_weight(topology_sibling_cpumask(cpu));
> +	if (threads > __max_smt_threads)
> +		__max_smt_threads = threads;
> +
>  	/*
>  	 * This needs a separate iteration over the cpus because we rely on all
>  	 * topology_sibling_cpumask links to be set-up.
> @@ -613,8 +629,7 @@ void set_cpu_sibling_map(int cpu)
>  			/*
>  			 *  Does this new cpu bringup a new core?
>  			 */
> -			if (cpumask_weight(
> -			    topology_sibling_cpumask(cpu)) == 1) {
> +			if (threads == 1) {
>  				/*
>  				 * for each core in package, increment
>  				 * the booted_cores for this new cpu
> @@ -631,16 +646,7 @@ void set_cpu_sibling_map(int cpu)
>  			} else if (i != cpu && !c->booted_cores)
>  				c->booted_cores = cpu_data(i).booted_cores;
>  		}
> -		if (match_pkg(c, o) && !topology_same_node(c, o))
> -			x86_has_numa_in_package = true;
> -
> -		if ((i == cpu) || (has_mp && match_die(c, o)))
> -			link_mask(topology_die_cpumask, cpu, i);
>  	}
> -
> -	threads = cpumask_weight(topology_sibling_cpumask(cpu));
> -	if (threads > __max_smt_threads)
> -		__max_smt_threads = threads;
>  }
>  
>  /* maps the cpu to the sched domain representing multi-core */