Hi Keir and Jan, This patch is to support the new core pair topology introduced by AMD new CPUs, which introduces a new concept of core/compute unit/node. There is a new feature bit for topology extension in CPUID:0x80000001. Also a new CPUID (0x8000001E) is introduced for CPU topology enumeration. This patch collects the sibling information from a new CPUID and the info will be stored into the sibling map in Xen. Please help review it. Comments are welcome. Signed-off-by: Wei Huang <wei.huang2@amd.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Jan-07 08:56 UTC
[Xen-devel] 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. 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())?>@@ -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). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Huang2, Wei
2011-Jan-07 15:52 UTC
[Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Huang2, Wei
2011-Jan-07 22:56 UTC
[Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
Hi Jan,
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?
Thanks,
-Wei
if (smp_num_siblings > 1) {
for_each_cpu(i, cpu_sibling_setup_mask) {
struct cpuinfo_x86 *o = &cpu_data(i);
if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
if (c->phys_proc_id == o->phys_proc_id
&&
c->compute_unit_id ==
o->compute_unit_id)
link_thread_siblings(cpu, i);
} else if (c->phys_proc_id == o->phys_proc_id
&&
c->cpu_core_id == o->cpu_core_id) {
link_thread_siblings(cpu, i);
}
}
} else {
cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
}
-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Huang2, Wei
Sent: Friday, January 07, 2011 9:52 AM
To: Jan Beulich
Cc: xen-devel@lists.xensource.com; keir@xen.org
Subject: [Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2011-Jan-10 08:20 UTC
[Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
>>> On 07.01.11 at 23:56, "Huang2, Wei" <Wei.Huang2@amd.com> wrote: > 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.Yes, cpu_{core,sibling}_map are being re-used (as I indicated earlier).> 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?That both seems a good idea to me. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2011-Jan-11 10:58 UTC
Re: [Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
On Fri, Jan 7, 2011 at 3:52 PM, Huang2, Wei <Wei.Huang2@amd.com> wrote:> [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?> > 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 > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2011-Jan-11 11:12 UTC
Re: [Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
On Tue, Jan 11, 2011 at 10:58 AM, George Dunlap <dunlapg@umich.edu> wrote:> On Fri, Jan 7, 2011 at 3:52 PM, Huang2, Wei <Wei.Huang2@amd.com> wrote: >> [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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Huang2, Wei
2011-Jan-11 15:38 UTC
RE: [Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
The compute unit is a circuit device which contains 2 cores. Each core has an independent integer execution logic and L1 data cache. But L2 cache, FPU, L1 instruction cache are shared between cores. You can find a description at http://tinyurl.com/ygq8jgu. The CPUID spec is available at http://support.amd.com/us/Processor_TechDocs/25481.pdf. Thanks, -Wei -----Original Message----- From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of George Dunlap Sent: Tuesday, January 11, 2011 4:58 AM To: Huang2, Wei Cc: Jan Beulich; xen-devel@lists.xensource.com; keir@xen.org Subject: Re: [Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection On Fri, Jan 7, 2011 at 3:52 PM, Huang2, Wei <Wei.Huang2@amd.com> wrote:> [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?> > 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 > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Huang
2011-Jan-11 17:05 UTC
Re: [Xen-devel] RE: [RFC][PATCH] AMD CPU core topology detection
Hi George, The analogy holds for most parts. Following with Jan''s recommendation, I will add a new compute_unit_id to each logic processor, with the hope of minimizing the impacts to Xen. No, it won''t use is_amd() style of statements. -Wei On 01/11/2011 05:12 AM, George Dunlap wrote:> On Tue, Jan 11, 2011 at 10:58 AM, George Dunlap<dunlapg@umich.edu> wrote: >> On Fri, Jan 7, 2011 at 3:52 PM, Huang2, Wei<Wei.Huang2@amd.com> wrote: >>> [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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel