AMD CPU core topology detection

Quoted 1 line

Quoted 1 line · Wei Huang

xen/arch/x86/cpu/amd.c +13 -0

+#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 

Quoted 1 line

Quoted 1 line · Wei Huang

xen/arch/x86/cpu/amd.c +13 -0

+#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 

Quoted 1 line

Quoted 1 line · Wei Huang

xen/arch/x86/cpu/amd.c +13 -0

+#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 

Quoted 1 line

Quoted 1 line · Wei

Looking at kernel 2.6.37, I think its implementation is very similar to what I did. Here is a code snip from smpboot.c file. The kernel treats compute_unit_id and cpu_core_id equally.

If you don't like the idea of piggybacking on existing cpu_core_id, I can add a compute_unit_id. Also we might want to move phys_proc_id and cpu_core_id to cpuinfo_x86?

[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. ]

xen/include/asm-x86/cpufeature.h line 135

Quoted 5 lines

xen/include/asm-x86/cpufeature.h +1 -0

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 1 line · Wei

[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.

Wei, can you point me to some documentation about exactly what the "compute unit" is?

[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. ]

xen/include/asm-x86/cpufeature.h line 135

Quoted 5 lines

xen/include/asm-x86/cpufeature.h +1 -0

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 4 lines · George Dunlap

Quoted 1 line · Wei

[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.

Wei, can you point me to some documentation about exactly what the "compute unit" is?

[Sorry, accidentally sent too early]

If there's a strong logical correspondence between Intel's "thread -> core -> socket" and AMD's "core -> compute unit -> node", then I think reusing the maps makes sense; but if there's a fairly significant difference, then I think coming up with a different naming convention would be best. I don't like the idea of having code that says "if(is_intel()) { /* Do things one way */} ; else if (is_amd()) { /* Do things a different way */}". Not only is it ugly, it's a set-up for bugs.

-George