Re: [PATCH v2] x86,sched: allow topologies where NUMA nodes share an LLC

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Mar 22, 2018 at 05:42:41PM -0700, Tim Chen wrote:
> On 03/22/2018 01:49 PM, Alison Schofield wrote:
> >
> > +	 */
> > +	if (!topology_same_node(c, o) &&
> > +	    (c->x86_vendor == X86_VENDOR_INTEL &&
> > +	     c->x86_model == INTEL_FAM6_SKYLAKE_X)) {
> > +		/* Use NUMA instead of coregroups for scheduling: */
> > +		x86_has_numa_in_package = true;
> 
> x86_has_numa_in_package will only be set true for SKYLAKE in the above? 
> 
> This boolean probably should be set for (!topology_same_node(c, o) && match_die(c, o)) and not
> dependent on cpu family.  Only the return value should depend on cpu family.
> 
> Tim

Tim,

x86_has_numa_in_package is being set in set_cpu_sibling_map()  with the
same criteria you describe: (!topology_same_node(c, o) &&  match_die(c,
o))

Skylake in SNC mode takes that path and it gets set correctly, so I'm
thinking and seeing that the setting of it in match_llc() is redundant.

The intent of the patch is to skip the topology_sane() check to avoid
the warning message it spits out at boot time. 

This has me wondering if, aside from it being redundant, if it may be
incorrect? Should we set that boolean based on vendor/model alone in
match_llc().  I guess I need to understand what happens w a Skylake that
doesn't have SNC turned on. I can try that configuration next.

Thanks for reviewing!
alisons
> 
> 
> > +
> > +		/*
> > +		 * Return value doesn't actually matter because we
> > +		 * are throwing away coregroups for scheduling anyway.
> > +		 * Return false to bypass topology broken bug messages
> > +		 * and fixups in sched_domain().
> > +		 */
> > +		return false;
> > +	}
> > +
> > +	return topology_sane(c, o, "llc");
> >  }
> >  
> >  /*
> > @@ -454,12 +492,6 @@ static struct sched_domain_topology_level x86_topology[] = {
> >  	{ NULL, },
> >  };
> >  
> > -/*
> > - * Set if a package/die has multiple NUMA nodes inside.
> > - * AMD Magny-Cours and Intel Cluster-on-Die have this.
> > - */
> > -static bool x86_has_numa_in_package;
> > -
> >  void set_cpu_sibling_map(int cpu)
> >  {
> >  	bool has_smt = smp_num_siblings > 1;
> > 
>