RE: [RFC][PATCH] AMD CPU core topology detection

["Huang2, Wei" <Wei.Huang2@amd.com>]

Hi Jan,

Thanks for your comments. Please my answers below.

-Wei
-----Original Message-----
From: Jan Beulich [mailto:JBeulich@novell.com] 
Sent: Friday, January 07, 2011 2:57 AM
To: Huang2, Wei
Cc: xen-devel@lists.xensource.com; keir@xen.org
Subject: Re: [RFC][PATCH] AMD CPU core topology detection

>>> On 06.01.11 at 17:47, Wei Huang <wei.huang2@amd.com> wrote:
>+#ifdef CONFIG_X86_HT
>+/* This function detects system CPU topology */
>+static void amd_detect_topology(struct cpuinfo_x86 *c)
>+{
>+	u32 eax, ebx, ecx, edx;
>+	int cpu = smp_processor_id();
>+
>+	if (c->x86_max_cores <= 1)
>+		return;
>+
>+	if (cpu_has(c, X86_FEATURE_TOPO_EXT)) {
>+		/* AMD new CPUs introduce a new term called compute unit. But 
>+		 * we still keep the names of existing variables for the 
>+		 * purpose of consistency. Keep in mind that cpu_core_id here 
>+		 * represents the computer unit ID; and we use the node ID as 
>+                 * the processor ID because it is unique across the whole 
>+		 * system. 
>+		 */
>+		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>+		cpu_core_id[cpu] = ebx & 0xFF;
>+		phys_proc_id[cpu] = ecx & 0xFF;
>+
>+		c->x86_num_siblings = ((ebx >> 8) & 0x3) + 1;

Did you check the consequences of re-using these for other than
their original purpose? I'm particularly wondering whether the code
in xen/arch/x86/cpu/mcheck/mce.c won't need updating, but that
isn't the only place that needs looking at.

[From my understanding, cpu_core_id and phys_proc_id are collected during boot (mostly in smpboot.c and under cpu/ directory) for sibling map. The sibling info is used for scheduler later on. Old AMD CPUs don't have HyperThreading, so the cpu_sibling_map isn't so useful. New CPUs will have core/compute unit/node. Using Intel's HT as an analogy, we have the following relationship: core=>hyper-thread, compute unit=>core, node=>processor). From that perspective, the change is reasonable. But I might have missed other parts. 

I will check mce implementation. This is a good point.]

If indeed your intention was to superimpose the new AMD topology
onto the existing one (partly other than Linux, which added a new
field to struct cpuinfo_x86, but uses cpu_{sibling,core}_map like
what results with your patch), won't there be consequences on (at
least) the credit scheduler (as you may not want cores to be
treated like threads in _csched_cpu_pick())?

[This is a tricky question. Based on my comments above, Bulldozer core can be treated as thread because it shares FPU with other cores. But different from HT, it also has many dedicated resources (int scheduler, pipeline and L1 cache). The scheduler needs more information in order to tell VCPUs apart on-the-fly and schedule them accordingly. Maybe George can take this into consideration for his credit2 scheduler (or beyond).

I will take a look at Linux's implementation. If you have new ideas, I would appreciate them too.
]

>@@ -132,6 +132,7 @@
> #define X86_FEATURE_SSE5	(6*32+ 11) /* AMD Streaming SIMD Extensions-5 */
> #define X86_FEATURE_SKINIT	(6*32+ 12) /* SKINIT, STGI/CLGI, DEV */
> #define X86_FEATURE_WDT		(6*32+ 13) /* Watchdog Timer */
>+#define X86_FEATURE_TOPO_EXT    (6*32+ 22) /* Topology Extension Support */

Would be nice to use the same name as Linux does (i.e. without
the last underscore).

[I can surely do so.]

Jan