Skip to contentSkip to conversation

c/s 21118: Magny-Coure breakage

PATCH In review Christoph Egger 5 participants 13 messages Apr 12, 2010 – Apr 15, 2010 Arm/actions semi architecture

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

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

Signature · 6 lines
---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
#1 lore raw

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

Quoted 12 lines · 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

#2 lore raw

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

Quoted 29 lines · 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

Quoted 12 lines · 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

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

#3 lore raw

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

Quoted 65 lines

-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Monday, April 12, 2010 5:01 AM
To: Christoph Egger; Nitin A Kamble
Cc: Andre Przywara; [email protected]
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

Quoted 1 line · Keir Fraser

We're at an intermediate state in 4.1 NUMA implementation I would

say. That

Quoted 1 line

changeset can be revised as necessary in future patches, but I think

it is

Quoted 1 line

broadly correct in that it's adding the right kind of extra things to

the

Quoted 1 line

control interfaces. Since we have 6-9 months before 4.1 is released

we don't

Quoted 5 lines

need panic about regressions just yet.

-- Keir

On 12/04/2010 11:07, "Christoph Egger" <[email protected]>

Quoted 5 lines · wrote:
Quoted 5 lines

Hi!

Changeset 21118 breaks NUMA on AMD Magny-Coure due to introduction

of

Quoted 1 line
Quoted 1 line

'sockets_per_node'. On AMD Magny-Coure we have two nodes on one

socket, hence

Quoted 1 line
Quoted 1 line

the existence of that field member introduces a breake on design

level.

Quoted 2 lines
Quoted 2 lines

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

field

Quoted 11 lines
Quoted 4 lines

member.

Christoph

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

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

#4 lore raw
Quoted 2 lines · Dan Magenheimer

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?

Quoted 2 lines

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.

Quoted 3 lines

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.

Signature · 3 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12
#5 lore raw
Quoted 7 lines · Andre Przywara
Quoted 3 lines

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

#6 lore raw

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

Signed-off-by Nitin A Kamble <[email protected]>

Thanks & Regards,
Nitin

Quoted 13 lines

-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Monday, April 12, 2010 6:25 AM
To: Andre Przywara; Dan Magenheimer
Cc: Christoph Egger; Kamble, Nitin A; [email protected]
Subject: Re: [Xen-devel] Re: c/s 21118: Magny-Coure breakage

Quoted 4 lines · Andre Przywara
Quoted 3 lines

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

Quoted 9 lines

48

Quoted 3 lines

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

tools/python/xen/lowlevel/xc/xc.c +0 -1
diff -r aae7cb2f1841 tools/python/xen/lowlevel/xc/xc.c
@@ -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),
tools/python/xen/xend/XendNode.py +0 -1
diff -r aae7cb2f1841 tools/python/xen/xend/XendNode.py
@@ -931,7 +931,6 @@
                       'nr_nodes',
                       'cores_per_socket',
                       'threads_per_core',
-                      'sockets_per_node',
                       'cpu_mhz',
                       'hw_caps',
                       'virt_caps',
xen/arch/x86/sysctl.c +0 -3
diff -r aae7cb2f1841 xen/arch/x86/sysctl.c
@@ -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;
xen/include/public/sysctl.h +0 -1
diff -r aae7cb2f1841 xen/include/public/sysctl.h
@@ -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;

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

#7 lore raw
Quoted 2 lines · Nitin A

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.

Signature · 3 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12
#8 lore raw

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

Quoted 31 lines

-----Original Message-----
From: Andre Przywara [mailto:[email protected]]
Sent: Wednesday, April 14, 2010 2:47 AM
To: Kamble, Nitin A
Cc: Keir Fraser; Dan Magenheimer; Christoph Egger; xen- [email protected]
Subject: Re: [Xen-devel] Re: c/s 21118: Magny-Coure breakage

Quoted 2 lines · Nitin A

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

#9 lore raw

PATCH 1/1 xend: make NUMA in xm info optional

Quoted 2 lines · Nitin A

Andre,
We will look into the xend instability issue.

I will also do this next, I reverted to using xl for now ;-)

Quoted 3 lines

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 <[email protected]>

Regards,
Andre.

Signature · 3 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12
tools/python/xen/xend/XendNode.py +13 -12
diff -urp xen.orig/tools/python/xen/xend/XendNode.py xen-unstable.hg/tools/python/xen/xend/XendNode.py
@@ -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())
tools/python/xen/xm/main.py +8 -3
diff -urp xen.orig/tools/python/xen/xm/main.py xen-unstable.hg/tools/python/xen/xm/main.py
@@ -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]

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

#10 lore raw

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

Quoted 2 lines · Andre Przywara
Quoted 1 line

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

#11 lore raw

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

Quoted 6 lines · Keir Fraser
Quoted 2 lines · Andre Przywara
Quoted 1 line

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.

Signature · 3 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12
#12 lore raw

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

Quoted 3 lines · Andre Przywara
Quoted 1 line

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