numa: fix problems with memory-less nodes

Hi,

If we decided to not report memory-less nodes in physinfo we should also skip them in the node_to_{cpu,memory,dma32_mem} Python lists. Currently Xen will not start guests on machines with memory-less nodes which are not the last ones. On an 8-node machine with empty nodes 4 and 5 "xm info" was reporting wrongly, also the node assignment algorithm crashed with a division by zero error. The attached patch fixes this by skipping empty nodes in the enumeration of resources.

Regards,
Andre.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448 3567 12
----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

diff -r 0e3910f1de64 tools/python/xen/lowlevel/xc/xc.c
@@ -1129,20 +1129,23 @@
             Py_DECREF(pyint);
             node_exists = 1;
         }
-        PyList_Append(node_to_cpu_obj, cpus); 
+        if (node_exists)
+            PyList_Append(node_to_cpu_obj, cpus); 
         Py_DECREF(cpus);
 
         /* Memory. */
         xc_availheap(self->xc_handle, 0, 0, i, &free_heap);
         node_exists = node_exists || (free_heap != 0);
         pyint = PyInt_FromLong(free_heap / 1024);
-        PyList_Append(node_to_memory_obj, pyint);
+        if (node_exists)
+            PyList_Append(node_to_memory_obj, pyint);
         Py_DECREF(pyint);
 
         /* DMA memory. */
         xc_availheap(self->xc_handle, 0, 32, i, &free_heap);
         pyint = PyInt_FromLong(free_heap / 1024);
-        PyList_Append(node_to_dma32_mem_obj, pyint);
+        if (node_exists)
+            PyList_Append(node_to_dma32_mem_obj, pyint);
         Py_DECREF(pyint);
 
         if ( node_exists )

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

Where to begin? Firstly, I thought that the ordering of nodes in the node_to_* lists actually mattered -- the lists are indexed by nodeid (a handle which can be passed to other Xen interfaces) are they not? If you don't include empty entries, then the index position of entries is no longer meaningful.

Secondly, you avoid appending to the node_to_cpu list if the node is cpu-less. But you avoid appending to the node_to_{memory,dma32} lists only if the node is *both* cpu-less and memory-less. That's not even consistent.

Please just fix the crap Python code.

-- Keir

OK, that seems to be an issue.
To be honest I am not a fan of omitting nodes from physinfo, but that is what the current code (RC1!) does and it definitely breaks Xen on my box. So I just made this small patch to make it work again.
Actually I would opt to revert the patch cropping the number of nodes reported by physinfo (20762:a1d0a575b4ba ?). Yes, that would result in nodes reported with zero memory, but in my tests this did not raise problems, as a node's memory can (and will) be exhausted even during normal operation. To illustrate the problem:
My box has 8 nodes, I removed the memory from nodes 4 & 5.
With the unpatched version xm info says:
total_memory : 73712
free_memory : 70865
node_to_cpu : node0:0-5,24-35

                         node1:6-11
                         node2:12-17
                         node3:18-23
                         node4:no cpus
                         node5:no cpus
node_to_memory         : node0:14267
                         node1:8167
                         node2:16335
                         node3:8167
                         node4:0
                         node5:0

So this listing completely omits the last two nodes (CPUs 36-47 and the 24 GB connected to them). The debug key triggered Xen-internal listing is correct, though:
(XEN) idx0 -> NODE0 start->0 size->4423680
(XEN) phys_to_nid(0000000000001000) -> 0 should be 0
(XEN) idx1 -> NODE1 start->4423680 size->2097152
(XEN) phys_to_nid(0000000438001000) -> 1 should be 1
(XEN) idx2 -> NODE2 start->6520832 size->4194304
(XEN) phys_to_nid(0000000638001000) -> 2 should be 2
(XEN) idx3 -> NODE3 start->10715136 size->2097152
(XEN) phys_to_nid(0000000a38001000) -> 3 should be 3
(XEN) idx6 -> NODE6 start->12812288 size->4194304
(XEN) phys_to_nid(0000000c38001000) -> 6 should be 6
(XEN) idx7 -> NODE7 start->17006592 size->2097152
(XEN) phys_to_nid(0000001038001000) -> 7 should be 7
With the patched xc.so xm info reports:
node_to_cpu : node0:0-5,24-35

                         node1:6-11
                         node2:12-17
                         node3:18-23
                         node4:36-41
                         node5:42-47
node_to_memory         : node0:14267
                         node1:8167
                         node2:16335
                         node3:8167
                         node4:16335
                         node5:7590

Although memory less nodes are not very common, it could happen sometimes with our new dual-node processor, where one could (even accidentally) forget to populate certain memory slots, as it has in fact a dual-node dual-channel memory interface.

OK, that's a point. I see that the value of node_exists can change.

What part do you exactly mean? The part triggering the division by zero?

I will see if I can fix this properly.

Regards,
Andre.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448 3567 12
----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

The intention of 20762 was not to change the semantics of the node_to_* lists. It's simply supposed to make available max_node_id to the toolstack (since this can differ from nr_nodes if there are holes in the online node map).

If the node_to_* semantics really have been changed by 20762, then it is a bug. It's not a bug I can eyeball however.

-- Keir

To be clear: I certainly agree that node_to_cpu[4,5] should list the CPUs that are present, even if the nodes are memory-less. But you tell me how 20762 has changed this behaviour.

If it was changed by one of Intel's slightly earlier changesets (which is possible) I'm happy to see it changed back. The new node_to_cpu[] behaviour you are observing is obviously stupid, and I assume unintended.

-- Keir