c/s 21118: Magny-Coure breakage

c/s 21118: Magny-Coure breakage

[Christoph Egger]

Hi!

Changeset 21118 breaks NUMA on AMD Magny-Coure due to introduction of
'sockets_per_node'. On AMD Magny-Coure we have two nodes on one socket, hence
the existence of that field member introduces a breake on design level.

Please revert changeset 21118 or rework the patch to get rid of this field 
member.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: c/s 21118: Magny-Coure breakage

[Keir Fraser]

We're at an intermediate state in 4.1 NUMA implementation I would say. That
changeset can be revised as necessary in future patches, but I think it is
broadly correct in that it's adding the right kind of extra things to the
control interfaces. Since we have 6-9 months before 4.1 is released we don't
need panic about regressions just yet.

 -- Keir

On 12/04/2010 11:07, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> 
> Hi!
> 
> Changeset 21118 breaks NUMA on AMD Magny-Coure due to introduction of
> 'sockets_per_node'. On AMD Magny-Coure we have two nodes on one socket, hence
> the existence of that field member introduces a breake on design level.
> 
> Please revert changeset 21118 or rework the patch to get rid of this field
> member.
> 
> Christoph
> 

Re: Re: c/s 21118: Magny-Coure breakage

[Keir Fraser]

It is worth discussing whether it makes sense to have the concept of sockets
in the NUMA interfaces. Would threads/cores/nodes suffice? Not sure sockets
give us any more than a hint about possible caching hierarchy (possible
socket-wide L3) and communication latency. That may be too vague to be any
use even where the concept of sockets-per-node is applicable.

 -- Keir

On 12/04/2010 11:41, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> We're at an intermediate state in 4.1 NUMA implementation I would say. That
> changeset can be revised as necessary in future patches, but I think it is
> broadly correct in that it's adding the right kind of extra things to the
> control interfaces. Since we have 6-9 months before 4.1 is released we don't
> need panic about regressions just yet.
> 
>  -- Keir
> 
> On 12/04/2010 11:07, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> 
>> 
>> Hi!
>> 
>> Changeset 21118 breaks NUMA on AMD Magny-Coure due to introduction of
>> 'sockets_per_node'. On AMD Magny-Coure we have two nodes on one socket, hence
>> the existence of that field member introduces a breake on design level.
>> 
>> Please revert changeset 21118 or rework the patch to get rid of this field
>> member.
>> 
>> Christoph
>> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: Re: c/s 21118: Magny-Coure breakage

[Dan Magenheimer]

A large number of software licensing beancounters in the industry
are going to be busy on this one ;-)

Christophe, can you comment on how Linux and/or other bare metal
operating systems will be reporting the Magny Cours hierarchy?
I'd hope that Xen could follow their lead on this rather
than forge new ground, which may result in an incompatible
implementation.

Dan

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Monday, April 12, 2010 5:01 AM
> To: Christoph Egger; Nitin A Kamble
> Cc: Andre Przywara; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] Re: c/s 21118: Magny-Coure breakage
> 
> It is worth discussing whether it makes sense to have the concept of
> sockets
> in the NUMA interfaces. Would threads/cores/nodes suffice? Not sure
> sockets
> give us any more than a hint about possible caching hierarchy (possible
> socket-wide L3) and communication latency. That may be too vague to be
> any
> use even where the concept of sockets-per-node is applicable.
> 
>  -- Keir
> 
> On 12/04/2010 11:41, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
> 
> > We're at an intermediate state in 4.1 NUMA implementation I would
> say. That
> > changeset can be revised as necessary in future patches, but I think
> it is
> > broadly correct in that it's adding the right kind of extra things to
> the
> > control interfaces. Since we have 6-9 months before 4.1 is released
> we don't
> > need panic about regressions just yet.
> >
> >  -- Keir
> >
> > On 12/04/2010 11:07, "Christoph Egger" <Christoph.Egger@amd.com>
> wrote:
> >
> >>
> >> Hi!
> >>
> >> Changeset 21118 breaks NUMA on AMD Magny-Coure due to introduction
> of
> >> 'sockets_per_node'. On AMD Magny-Coure we have two nodes on one
> socket, hence
> >> the existence of that field member introduces a breake on design
> level.
> >>
> >> Please revert changeset 21118 or rework the patch to get rid of this
> field
> >> member.
> >>
> >> Christoph
> >>
> >
> >
> >
> > _______________________________________________
> > 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

Re: Re: c/s 21118: Magny-Coure breakage

[Andre Przywara]

Dan Magenheimer wrote:
> A large number of software licensing beancounters in the industry
> are going to be busy on this one ;-)
Yes, but do they use the Xen hypervisor interface for that? And do they 
look at the actual socket number? As far as I remember some companies 
;-) are counting MCMs twice, so actually counting the nodes?

> Christophe, can you comment on how Linux and/or other bare metal
> operating systems will be reporting the Magny Cours hierarchy?
As long as there is no such notion as 'sockets per node' (which isn't on 
Linux), everything is fine. It will just report 8 nodes with 6 cores 
each (in case of a 4 socket M-C). Each core has a associated physical 
package id, which is just the same for 12 cores or 2 nodes.
I have a machine here up and running, so just tell me what sysfs path 
you are interested in and I can send you the output.

> I'd hope that Xen could follow their lead on this rather
> than forge new ground, which may result in an incompatible
> implementation.
Until c/s 21118 Xen was just fine, reporting 8 nodes, 4 sockets and 48 
cores. It is just this socket per node (which got removed some years 
ago), which is doing harm. (BTW: I already complained about that last 
year, when Nitin's patch first appeared on the ML).

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

Re: Re: c/s 21118: Magny-Coure breakage

[Keir Fraser]

On 12/04/2010 14:14, "Andre Przywara" <andre.przywara@amd.com> wrote:

>> I'd hope that Xen could follow their lead on this rather
>> than forge new ground, which may result in an incompatible
>> implementation.
> Until c/s 21118 Xen was just fine, reporting 8 nodes, 4 sockets and 48
> cores. It is just this socket per node (which got removed some years
> ago), which is doing harm. (BTW: I already complained about that last
> year, when Nitin's patch first appeared on the ML).

Okay, well I will revert the patch if we reach no further consensus.

 -- Keir

RE: Re: c/s 21118: Magny-Coure breakage

[Kamble, Nitin A]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Keir,
  Attached is the patch to take of the sockets_per_node information from the 21118 changeset.

Signed-Off-By: Nitin A Kamble <nitin.a.kamble@intel.com>
Thanks & Regards,
Nitin


> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Monday, April 12, 2010 6:25 AM
> To: Andre Przywara; Dan Magenheimer
> Cc: Christoph Egger; Kamble, Nitin A; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] Re: c/s 21118: Magny-Coure breakage
> 
> On 12/04/2010 14:14, "Andre Przywara" <andre.przywara@amd.com> wrote:
> 
> >> I'd hope that Xen could follow their lead on this rather
> >> than forge new ground, which may result in an incompatible
> >> implementation.
> > Until c/s 21118 Xen was just fine, reporting 8 nodes, 4 sockets and
> 48
> > cores. It is just this socket per node (which got removed some years
> > ago), which is doing harm. (BTW: I already complained about that last
> > year, when Nitin's patch first appeared on the ML).
> 
> Okay, well I will revert the patch if we reach no further consensus.
> 
>  -- Keir
> 


[-- Attachment #2: take_out_sockets_per_node_field.patch --]
[-- Type: application/octet-stream, Size: 2219 bytes --]

diff -r aae7cb2f1841 tools/python/xen/lowlevel/xc/xc.c
--- a/tools/python/xen/lowlevel/xc/xc.c	Fri Apr 09 08:54:25 2010 +0100
+++ b/tools/python/xen/lowlevel/xc/xc.c	Mon Apr 12 10:50:50 2010 -0700
@@ -1179,7 +1179,6 @@
                             "nr_nodes",         pinfo.nr_nodes,
                             "threads_per_core", pinfo.threads_per_core,
                             "cores_per_socket", pinfo.cores_per_socket,
-                            "sockets_per_node", pinfo.sockets_per_node,
                             "nr_cpus",          pinfo.nr_cpus, 
                             "total_memory",     pages_to_kib(pinfo.total_pages),
                             "free_memory",      pages_to_kib(pinfo.free_pages),
diff -r aae7cb2f1841 tools/python/xen/xend/XendNode.py
--- a/tools/python/xen/xend/XendNode.py	Fri Apr 09 08:54:25 2010 +0100
+++ b/tools/python/xen/xend/XendNode.py	Mon Apr 12 10:50:50 2010 -0700
@@ -931,7 +931,6 @@
                       'nr_nodes',
                       'cores_per_socket',
                       'threads_per_core',
-                      'sockets_per_node',
                       'cpu_mhz',
                       'hw_caps',
                       'virt_caps',
diff -r aae7cb2f1841 xen/arch/x86/sysctl.c
--- a/xen/arch/x86/sysctl.c	Fri Apr 09 08:54:25 2010 +0100
+++ b/xen/arch/x86/sysctl.c	Mon Apr 12 10:50:50 2010 -0700
@@ -63,9 +63,6 @@
         pi->nr_nodes = num_online_nodes();
         pi->max_node_id = last_node(node_online_map);
         pi->max_cpu_id = last_cpu(cpu_online_map);
-        pi->sockets_per_node = 
-            pi->nr_cpus / 
-            (pi->nr_nodes * pi->cores_per_socket * pi->threads_per_core);
         pi->total_pages = total_pages;
         pi->free_pages = avail_domheap_pages();
         pi->scrub_pages = 0;
diff -r aae7cb2f1841 xen/include/public/sysctl.h
--- a/xen/include/public/sysctl.h	Fri Apr 09 08:54:25 2010 +0100
+++ b/xen/include/public/sysctl.h	Mon Apr 12 10:50:50 2010 -0700
@@ -93,7 +93,6 @@
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;
-    uint32_t sockets_per_node;
     uint32_t nr_cpus, max_cpu_id;
     uint32_t nr_nodes, max_node_id;
     uint32_t cpu_khz;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Re: c/s 21118: Magny-Coure breakage

[Andre Przywara]

Kamble, Nitin A wrote:
> Keir,
>   Attached is the patch to take of the sockets_per_node information from the 21118 changeset.
Thanks Nitin, that looks better.
Unfortunately the patch itself does not work for me, xend crashes (and 
keeps restarting) due to signal 11. It works though if I just revert 
your patch (rebuilt both hypervisor and tools). I will try to further 
debug this, but in general your patch looks clean to me.

BTW: I found the output of xm info now a bit too verbose, can we
introduce a -v option to show the NUMA information only when explicitly
requested?
On my 48 core/8 nodes machine I get a lot of text, this should be even 
worse with 128 threads, right?

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

RE: Re: c/s 21118: Magny-Coure breakage

[Kamble, Nitin A]

Andre,
We will look into the xend instability issue.

I also find that lots of scrolling happens with xm info output. I think putting the detailed information in the -v option is a good idea. May be you can extend it in this matter.

Thanks & Regards,
Nitin

> -----Original Message-----
> From: Andre Przywara [mailto:andre.przywara@amd.com]
> Sent: Wednesday, April 14, 2010 2:47 AM
> To: Kamble, Nitin A
> Cc: Keir Fraser; Dan Magenheimer; Christoph Egger; xen-
> devel@lists.xensource.com
> Subject: Re: [Xen-devel] Re: c/s 21118: Magny-Coure breakage
> 
> Kamble, Nitin A wrote:
> > Keir,
> >   Attached is the patch to take of the sockets_per_node information
> from the 21118 changeset.
> Thanks Nitin, that looks better.
> Unfortunately the patch itself does not work for me, xend crashes (and
> keeps restarting) due to signal 11. It works though if I just revert
> your patch (rebuilt both hypervisor and tools). I will try to further
> debug this, but in general your patch looks clean to me.
> 
> BTW: I found the output of xm info now a bit too verbose, can we
> introduce a -v option to show the NUMA information only when explicitly
> requested?
> On my 48 core/8 nodes machine I get a lot of text, this should be even
> worse with 128 threads, right?
> 
> Regards,
> Andre.
> 
> --
> Andre Przywara
> AMD-Operating System Research Center (OSRC), Dresden, Germany
> Tel: +49 351 448-3567-12

[PATCH] xend: make NUMA in xm info optional

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

Kamble, Nitin A wrote:
> Andre,
> We will look into the xend instability issue.
I will also do this next, I reverted to using xl for now ;-)

> I also find that lots of scrolling happens with xm info output.
 > I think putting the detailed information in the -v option is a
 > good idea. May be you can extend it in this matter.
OK, see the attached patch. I used -n (or --numa) for clarity.
Tested against an older changeset (with still working xend).

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: xm_info_numa.patch --]
[-- Type: text/x-patch, Size: 4312 bytes --]

diff -urp xen.orig/tools/python/xen/xend/XendNode.py xen-unstable.hg/tools/python/xen/xend/XendNode.py
--- xen.orig/tools/python/xen/xend/XendNode.py	2010-04-15 13:25:40.000000000 +0200
+++ xen-unstable.hg/tools/python/xen/xend/XendNode.py	2010-04-15 12:37:37.000000000 +0200
@@ -833,8 +833,8 @@ class XendNode:
     # Getting host information.
     #
 
-    def info(self):
-        return (self.nodeinfo() + self.physinfo() + self.xeninfo() +
+    def info(self, show_numa = 1):
+        return (self.nodeinfo() + self.physinfo(show_numa) + self.xeninfo() +
                 self.xendinfo())
 
     def nodeinfo(self):
@@ -910,7 +910,7 @@ class XendNode:
             str='none\n'
         return str[:-1];
 
-    def physinfo(self):
+    def physinfo(self, show_numa):
         info = self.xc.physinfo()
         tinfo = self.xc.topologyinfo()
         ninfo = self.xc.numainfo()
@@ -921,12 +921,6 @@ class XendNode:
         info['total_memory'] = info['total_memory'] / 1024
         info['free_memory']  = info['free_memory'] / 1024
 
-        info['cpu_topology']  = \
-             self.format_cpu_to_core_socket_node(tinfo)
-
-        info['numa_info']  = \
-             self.format_numa_info(ninfo)
-
         ITEM_ORDER = ['nr_cpus',
                       'nr_nodes',
                       'cores_per_socket',
@@ -937,10 +931,17 @@ class XendNode:
                       'virt_caps',
                       'total_memory',
                       'free_memory',
-                      'cpu_topology',
-                      'numa_info',
                       ]
 
+        if show_numa != 0:
+            info['cpu_topology']  = \
+                 self.format_cpu_to_core_socket_node(tinfo)
+
+            info['numa_info']  = \
+                 self.format_numa_info(ninfo)
+
+            ITEM_ORDER += [ 'cpu_topology', 'numa_info' ]
+
         return [[k, info[k]] for k in ITEM_ORDER]
 
     def pciinfo(self):
@@ -1050,7 +1051,7 @@ class XendNode:
     def xeninfo_dict(self):
         return dict(self.xeninfo())
     def physinfo_dict(self):
-        return dict(self.physinfo())
+        return dict(self.physinfo(1))
     def info_dict(self):
         return dict(self.info())
 
diff -urp xen.orig/tools/python/xen/xm/main.py xen-unstable.hg/tools/python/xen/xm/main.py
--- xen.orig/tools/python/xen/xm/main.py	2010-04-15 12:51:03.000000000 +0200
+++ xen-unstable.hg/tools/python/xen/xm/main.py	2010-04-15 12:43:44.000000000 +0200
@@ -145,7 +145,8 @@ SUBCOMMAND_HELP = {
     'domname'     : ('<DomId>', 'Convert a domain id to domain name.'),
     'dump-core'   : ('[-L|--live] [-C|--crash] [-R|--reset] <Domain> [Filename]',
                      'Dump core for a specific domain.'),
-    'info'        : ('[-c|--config]', 'Get information about Xen host.'),
+    'info'        : ('[-c|--config] [-n|--numa]',
+                     'Get information about Xen host.'),
     'log'         : ('', 'Print Xend log'),
     'rename'      : ('<Domain> <NewDomainName>', 'Rename a domain.'),
     'sched-sedf'  : ('<Domain> [options]', 'Get/set EDF parameters.'),
@@ -320,6 +321,7 @@ SUBCOMMAND_OPTIONS = {
     ),
     'info': (
        ('-c', '--config', 'List Xend configuration parameters'),
+       ('-n', '--numa', 'List host NUMA topology information'),
     ),
     'tmem-list': (
        ('-l', '--long', 'List tmem stats.'),
@@ -1744,15 +1746,18 @@ def xm_info(args):
     arg_check(args, "info", 0, 1)
     
     try:
-        (options, params) = getopt.gnu_getopt(args, 'c', ['config'])
+        (options, params) = getopt.gnu_getopt(args, 'cn', ['config','numa'])
     except getopt.GetoptError, opterr:
         err(opterr)
         usage('info')
     
     show_xend_config = 0
+    show_numa_topology = 0
     for (k, v) in options:
         if k in ['-c', '--config']:
             show_xend_config = 1
+        if k in ['-n', '--numa']:
+            show_numa_topology = 1
 
     if show_xend_config:
         for name, obj in inspect.getmembers(xoptions):
@@ -1839,7 +1844,7 @@ def xm_info(args):
         for (k, v) in sorted:
            print "%-23s:" % k, v 
     else:
-        info = server.xend.node.info()
+        info = server.xend.node.info(show_numa_topology)
         for x in info[1:]:
             if len(x) < 2: 
                 print "%-23s: (none)" % x[0]

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xend: make NUMA in xm info optional

[Keir Fraser]

On 15/04/2010 12:33, "Andre Przywara" <andre.przywara@amd.com> wrote:

>> We will look into the xend instability issue.
> I will also do this next, I reverted to using xl for now ;-)

This is fixed by xen-unstable:21183.

 -- Keir

Re: [PATCH] xend: make NUMA in xm info optional

[Andre Przywara]

Keir Fraser wrote:
> On 15/04/2010 12:33, "Andre Przywara" <andre.przywara@amd.com> wrote:
> 
>>> We will look into the xend instability issue.
>> I will also do this next, I reverted to using xl for now ;-)
> 
> This is fixed by xen-unstable:21183.
Yes, I just saw (and tested) this a few minutes ago. I will resend a 
rebased version of the patch along with a commit message.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

Re: [PATCH] xend: make NUMA in xm info optional

[Keir Fraser]

On 15/04/2010 13:14, "Andre Przywara" <andre.przywara@amd.com> wrote:

>> This is fixed by xen-unstable:21183.
> Yes, I just saw (and tested) this a few minutes ago. I will resend a
> rebased version of the patch along with a commit message.

I already checked your patch in, since it didn't need rebasing, and you
already sent a signed-off-by line. It's xen-unstable:21186.

 -- Keir