Intel x86_64 llc_shared_map/cpu_llc_id anomolies

Intel x86_64 llc_shared_map/cpu_llc_id anomolies

[Dimitri Sivanich]

I'm seeing some behavior on a multiblade Intel x86_64 that does not
appear to be correct.

My x86_64 multiblade configuration has llc_shared_map (last level
cache shared map) showing cpus on different blades sharing a last
level cache.  With 2 cpus/blade and 4 blades (8 cpus total), I show
cpus 1,3,5,7 sharing llc, and 0,2,4,6 sharing llc.

The llc_shared_map is set based on matching values of cpu_llc_id
(last level cache ID).  The cpu_llc_id is set based on
cpuinfo_x86->apicid.  This value is set to cpuinfo_x86->initial_apicid.

The value of cpuinfo_x86->initial_apicid appears to be set twice
during startup:

- In generic_identify(), using the value returned by cpuid_ebx(1).
  This is the initial_apicid set at poweron, and results in
  cpuinfo_x86->initial_apicid having a value that is not unique across
  blades (where the blades do not share a cpu bus).

- Later, in detect_extended_topology() using the value for edx returned
  by cpuid_count(0xb,..).  This results in cpuinfo_x86->initial_apicid
  having a value that appears to be unique across blades.

Between those two assignments, cpuinfo_x86->apicid is set to
cpuinfo_x86->initial_apicid, so it is non-unique.  Then cpu_llc_id gets
set in init_intel_cacheinfo(), so cpu_llc_id winds up with a value that
is non-unique across blades, and llc_shared_map winds up showing cpus
on different blades as sharing last level cache.

Is cpu_llc_id supposed to be unique across blades?  I assume
llc_shared_map should not be showing shared cache across blades.

Re: Intel x86_64 llc_shared_map/cpu_llc_id anomolies

[Yinghai Lu]

On Fri, Dec 12, 2008 at 1:42 PM, Dimitri Sivanich <sivanich@sgi.com> wrote:
> I'm seeing some behavior on a multiblade Intel x86_64 that does not
> appear to be correct.

can you post your boot log?

can you make sure phys_proc_id-->cpu_core_id-->thread_id ===> apic id
is consistent to your SRAT table/MADT etc?

YH

Re: Intel x86_64 llc_shared_map/cpu_llc_id anomolies

[Suresh Siddha]

On Fri, Dec 12, 2008 at 01:42:56PM -0800, Dimitri Sivanich wrote:
> I'm seeing some behavior on a multiblade Intel x86_64 that does not
> appear to be correct.
> 
> My x86_64 multiblade configuration has llc_shared_map (last level
> cache shared map) showing cpus on different blades sharing a last
> level cache.  With 2 cpus/blade and 4 blades (8 cpus total), I show
> cpus 1,3,5,7 sharing llc, and 0,2,4,6 sharing llc.
> 
> The llc_shared_map is set based on matching values of cpu_llc_id
> (last level cache ID).  The cpu_llc_id is set based on
> cpuinfo_x86->apicid.  This value is set to cpuinfo_x86->initial_apicid.
> 
> The value of cpuinfo_x86->initial_apicid appears to be set twice
> during startup:
> 
> - In generic_identify(), using the value returned by cpuid_ebx(1).
>   This is the initial_apicid set at poweron, and results in
>   cpuinfo_x86->initial_apicid having a value that is not unique across
>   blades (where the blades do not share a cpu bus).
> 
> - Later, in detect_extended_topology() using the value for edx returned
>   by cpuid_count(0xb,..).  This results in cpuinfo_x86->initial_apicid
>   having a value that appears to be unique across blades.
> 
> Between those two assignments, cpuinfo_x86->apicid is set to
> cpuinfo_x86->initial_apicid, so it is non-unique.  Then cpu_llc_id gets
> set in init_intel_cacheinfo(), so cpu_llc_id winds up with a value that
> is non-unique across blades, and llc_shared_map winds up showing cpus
> on different blades as sharing last level cache.
> 
> Is cpu_llc_id supposed to be unique across blades?  I assume
> llc_shared_map should not be showing shared cache across blades.

x86 folks, Appended patch fixes the reported issue. Please apply.

thanks,
suresh
---

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: [patch] x86: Fix apicid initialization in per cpu's cpuinfo_x86 struct

In the presence of extended topology eumeration leaf 0xb provided
by cpuid, 32bit extended initial_apicid in cpuinfo_x86 struct will be updated
by detect_extended_topology(). At this instance, we should also
reinit the apicid (which could also potentially be extended to 32bit).

With out this there will potentially be duplicate apicid's populated in the
per cpu's cpuinfo_x86 struct, resulting in wrong cache sharing topology etc
detected by init_intel_cacheinfo().

Impact: wrong cache sharing detection on platforms supporting > 8 bit apicid's

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Reported-by: Dimitri Sivanich <sivanich@sgi.com>
Acked-by: Dimitri Sivanich <sivanich@sgi.com>
Cc: Yinghai Lu <yhlu.kernel@gmail.com>
---
 
diff --git a/arch/x86/kernel/cpu/addon_cpuid_features.c b/arch/x86/kernel/cpu/addon_cpuid_features.c
index ef8f831..2cf2363 100644
--- a/arch/x86/kernel/cpu/addon_cpuid_features.c
+++ b/arch/x86/kernel/cpu/addon_cpuid_features.c
@@ -120,9 +120,17 @@ void __cpuinit detect_extended_topology(struct cpuinfo_x86 *c)
 	c->cpu_core_id = phys_pkg_id(c->initial_apicid, ht_mask_width)
 						 & core_select_mask;
 	c->phys_proc_id = phys_pkg_id(c->initial_apicid, core_plus_mask_width);
+	/*
+	 * Reinit the apicid, now that we have extended initial_apicid.
+	 */
+	c->apicid = phys_pkg_id(c->initial_apicid, 0);
 #else
 	c->cpu_core_id = phys_pkg_id(ht_mask_width) & core_select_mask;
 	c->phys_proc_id = phys_pkg_id(core_plus_mask_width);
+	/*
+	 * Reinit the apicid, now that we have extended initial_apicid.
+	 */
+	c->apicid = phys_pkg_id(0);
 #endif
 	c->x86_max_cores = (core_level_siblings / smp_num_siblings);
 
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index cce0b61..57e3fba 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -242,6 +242,13 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c)
 
 	intel_workarounds(c);
 
+	/*
+	 * Detect the extended topology information if available. This
+	 * will reinitialise the initial_apicid which will be used
+	 * in init_intel_cacheinfo()
+	 */
+	detect_extended_topology(c);
+
 	l2 = init_intel_cacheinfo(c);
 	if (c->cpuid_level > 9) {
 		unsigned eax = cpuid_eax(10);
@@ -313,7 +320,6 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c)
 
 #endif
 
-	detect_extended_topology(c);
 	if (!cpu_has(c, X86_FEATURE_XTOPOLOGY)) {
 		/*
 		 * let's use the legacy cpuid vector 0x1 and 0x4 for topology

Re: Intel x86_64 llc_shared_map/cpu_llc_id anomolies

[Ingo Molnar]

* Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> > Is cpu_llc_id supposed to be unique across blades?  I assume 
> > llc_shared_map should not be showing shared cache across blades.
> 
> x86 folks, Appended patch fixes the reported issue. Please apply.

applied, thanks Suresh!

Not for v2.6.28 because that is going to be released in a few days and 
these sorts of changes can easily cause boot regressions - but i marked it 
for -stable backport so it will hit .28.1 if everything is fine.

	Ingo