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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 23 Mar 2018 17:17:20 -0700
From:   Alison Schofield <alison.schofield@...el.com>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        "H. Peter Anvin" <hpa@...ux.intel.com>,
        Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <peterz@...radead.org>,
        David Rientjes <rientjes@...gle.com>,
        Igor Mammedov <imammedo@...hat.com>,
        Prarit Bhargava <prarit@...hat.com>, brice.goglin@...il.com,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86,sched: allow topologies where NUMA nodes share an
 LLC

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