[PATCH] xend: Fix non-contiguous NUMA node assignment

[PATCH] xend: Fix non-contiguous NUMA node assignment

[Andre Przywara]

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

Hi,

it seems that I missed a point in this whole addition of max_node_id. I 
see the difference in the Xen HV part, so nr_nodes got replaced with 
max_node_id in physinfo_t (and xc_physinfo_t, respectively).
But where does this value help in xend? There is no single Python 
reference to the physinfo()'s max_node_id field, instead all functions 
use the old (but now bogus) nr_nodes variable.
So in the attached patch I kept the xc.physinfo() returned dictionary 
with only a nr_nodes field, calculated by simply adding 1 to max_node_id 
from libxc. Empty nodes can (and will) be detected by iterating through 
the node_to_cpus and node_to_memory lists.
Nodes without memory should not be considered during guest's memory 
allocation, but will be used for further CPU affinity setting if the 
number of VCPUs exceeds the number of cores per node.

Please correct me if I am totally wrong on this, but this seems to work 
much better in my case.

Regards,
Andre.

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

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

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

diff -r db8a882f5515 tools/python/xen/lowlevel/xc/xc.c
--- a/tools/python/xen/lowlevel/xc/xc.c	Thu Jan 14 14:11:25 2010 +0000
+++ b/tools/python/xen/lowlevel/xc/xc.c	Fri Jan 15 14:20:05 2010 +0100
@@ -1078,7 +1078,7 @@
 #define MAX_CPU_ID 255
     xc_physinfo_t info;
     char cpu_cap[128], virt_caps[128], *p;
-    int i, j, max_cpu_id, nr_nodes = 0;
+    int i, j, max_cpu_id;
     uint64_t free_heap;
     PyObject *ret_obj, *node_to_cpu_obj, *node_to_memory_obj;
     PyObject *node_to_dma32_mem_obj;
@@ -1115,7 +1115,6 @@
     node_to_dma32_mem_obj = PyList_New(0);
     for ( i = 0; i <= info.max_node_id; i++ )
     {
-        int node_exists = 0;
         PyObject *pyint;
 
         /* CPUs. */
@@ -1127,14 +1126,12 @@
             pyint = PyInt_FromLong(j);
             PyList_Append(cpus, pyint);
             Py_DECREF(pyint);
-            node_exists = 1;
         }
         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);
         Py_DECREF(pyint);
@@ -1145,13 +1142,10 @@
         PyList_Append(node_to_dma32_mem_obj, pyint);
         Py_DECREF(pyint);
 
-        if ( node_exists )
-            nr_nodes++;
     }
 
-    ret_obj = Py_BuildValue("{s:i,s:i,s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s:s:s}",
-                            "nr_nodes",         nr_nodes,
-                            "max_node_id",      info.max_node_id,
+    ret_obj = Py_BuildValue("{s:i,s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}",
+                            "nr_nodes",         info.max_node_id + 1,
                             "max_cpu_id",       info.max_cpu_id,
                             "threads_per_core", info.threads_per_core,
                             "cores_per_socket", info.cores_per_socket,

[-- 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: Fix non-contiguous NUMA node assignment

[Jiang, Yunhong]

I transfered this issue to Dexuan to have a look last week, because I'm busy on some MCA issue. 
Per my understanding, the refer to nr_nodes in xend python should be replaced by max_node_id, but I didn't look into detail of it.
I willhave a discussion with Dexuan on Monday.

--jyh

>-----Original Message-----
>From: Andre Przywara [mailto:andre.przywara@amd.com]
>Sent: Friday, January 15, 2010 9:29 PM
>To: Keir Fraser
>Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
>Subject: [PATCH] xend: Fix non-contiguous NUMA node assignment
>
>Hi,
>
>it seems that I missed a point in this whole addition of max_node_id. I
>see the difference in the Xen HV part, so nr_nodes got replaced with
>max_node_id in physinfo_t (and xc_physinfo_t, respectively).
>But where does this value help in xend? There is no single Python
>reference to the physinfo()'s max_node_id field, instead all functions
>use the old (but now bogus) nr_nodes variable.
>So in the attached patch I kept the xc.physinfo() returned dictionary
>with only a nr_nodes field, calculated by simply adding 1 to max_node_id
>from libxc. Empty nodes can (and will) be detected by iterating through
>the node_to_cpus and node_to_memory lists.
>Nodes without memory should not be considered during guest's memory
>allocation, but will be used for further CPU affinity setting if the
>number of VCPUs exceeds the number of cores per node.
>
>Please correct me if I am totally wrong on this, but this seems to work
>much better in my case.
>
>Regards,
>Andre.
>
>Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>
>--
>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

Re: [PATCH] xend: Fix non-contiguous NUMA node assignment

[Keir Fraser]

nr_nodes was always num_online_nodes() returned by Xen -- not accounting for
holes in node id space. Hance I emulated that behaviour from the Python
extension package. If what you actually want everywhere in the Python code
is max_node_id, then please remove the nr_nodes code from xc.c and all
references to it from the Python code. I agree that using max_node_id seems
more correct than nr_nodes -- the intention was for someone to plumb that
new field properly into the Python code anyway.

 -- Keir

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

> Hi,
> 
> it seems that I missed a point in this whole addition of max_node_id. I
> see the difference in the Xen HV part, so nr_nodes got replaced with
> max_node_id in physinfo_t (and xc_physinfo_t, respectively).
> But where does this value help in xend? There is no single Python
> reference to the physinfo()'s max_node_id field, instead all functions
> use the old (but now bogus) nr_nodes variable.
> So in the attached patch I kept the xc.physinfo() returned dictionary
> with only a nr_nodes field, calculated by simply adding 1 to max_node_id
> from libxc. Empty nodes can (and will) be detected by iterating through
> the node_to_cpus and node_to_memory lists.
> Nodes without memory should not be considered during guest's memory
> allocation, but will be used for further CPU affinity setting if the
> number of VCPUs exceeds the number of cores per node.
> 
> Please correct me if I am totally wrong on this, but this seems to work
> much better in my case.
> 
> Regards,
> Andre.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>

Re: [PATCH] xend: Fix non-contiguous NUMA node assignment

[Keir Fraser]

I had a go myself: see c/s 20817. This keeps nr_nodes as well as
max_node_id, and continues to use it where it seemed to make sense to do so.

 -- Keir

On 17/01/2010 17:48, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> nr_nodes was always num_online_nodes() returned by Xen -- not accounting for
> holes in node id space. Hance I emulated that behaviour from the Python
> extension package. If what you actually want everywhere in the Python code
> is max_node_id, then please remove the nr_nodes code from xc.c and all
> references to it from the Python code. I agree that using max_node_id seems
> more correct than nr_nodes -- the intention was for someone to plumb that
> new field properly into the Python code anyway.
> 
>  -- Keir
> 
> On 15/01/2010 13:28, "Andre Przywara" <andre.przywara@amd.com> wrote:
> 
>> Hi,
>> 
>> it seems that I missed a point in this whole addition of max_node_id. I
>> see the difference in the Xen HV part, so nr_nodes got replaced with
>> max_node_id in physinfo_t (and xc_physinfo_t, respectively).
>> But where does this value help in xend? There is no single Python
>> reference to the physinfo()'s max_node_id field, instead all functions
>> use the old (but now bogus) nr_nodes variable.
>> So in the attached patch I kept the xc.physinfo() returned dictionary
>> with only a nr_nodes field, calculated by simply adding 1 to max_node_id
>> from libxc. Empty nodes can (and will) be detected by iterating through
>> the node_to_cpus and node_to_memory lists.
>> Nodes without memory should not be considered during guest's memory
>> allocation, but will be used for further CPU affinity setting if the
>> number of VCPUs exceeds the number of cores per node.
>> 
>> Please correct me if I am totally wrong on this, but this seems to work
>> much better in my case.
>> 
>> Regards,
>> Andre.
>> 
>> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] xend: Fix non-contiguous NUMA node assignment

[Andre Przywara]

Keir Fraser wrote:
> I had a go myself: see c/s 20817. This keeps nr_nodes as well as
> max_node_id, and continues to use it where it seemed to make sense to do so.

c/s 20817 seems OK to me. Actually that was how I started, but then 
decided to drop max_node_id because I didn't see the sense in keeping 
two variables which actually differ just by 1. But I didn't consider 
chunk #3 of XendDomainInfo.py, which actually also fixes another bug.

Thanks!
Regards,
Andre.

> 
>  -- Keir
> 
> On 17/01/2010 17:48, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
> 
>> nr_nodes was always num_online_nodes() returned by Xen -- not accounting for
>> holes in node id space. Hance I emulated that behaviour from the Python
>> extension package. If what you actually want everywhere in the Python code
>> is max_node_id, then please remove the nr_nodes code from xc.c and all
>> references to it from the Python code. I agree that using max_node_id seems
>> more correct than nr_nodes -- the intention was for someone to plumb that
>> new field properly into the Python code anyway.
>>
>>  -- Keir
>>
>> On 15/01/2010 13:28, "Andre Przywara" <andre.przywara@amd.com> wrote:
>>
>>> Hi,
>>>
>>> it seems that I missed a point in this whole addition of max_node_id. I
>>> see the difference in the Xen HV part, so nr_nodes got replaced with
>>> max_node_id in physinfo_t (and xc_physinfo_t, respectively).
>>> But where does this value help in xend? There is no single Python
>>> reference to the physinfo()'s max_node_id field, instead all functions
>>> use the old (but now bogus) nr_nodes variable.
>>> So in the attached patch I kept the xc.physinfo() returned dictionary
>>> with only a nr_nodes field, calculated by simply adding 1 to max_node_id
>>> from libxc. Empty nodes can (and will) be detected by iterating through
>>> the node_to_cpus and node_to_memory lists.
>>> Nodes without memory should not be considered during guest's memory
>>> allocation, but will be used for further CPU affinity setting if the
>>> number of VCPUs exceeds the number of cores per node.
>>>
>>> Please correct me if I am totally wrong on this, but this seems to work
>>> much better in my case.
>>>
>>> Regards,
>>> Andre.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> 

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

Re: [PATCH] xend: Fix non-contiguous NUMA node assignment

[Keir Fraser]

On 18/01/2010 08:53, "Andre Przywara" <andre.przywara@amd.com> wrote:

> Keir Fraser wrote:
>> I had a go myself: see c/s 20817. This keeps nr_nodes as well as
>> max_node_id, and continues to use it where it seemed to make sense to do so.
> 
> c/s 20817 seems OK to me. Actually that was how I started, but then
> decided to drop max_node_id because I didn't see the sense in keeping
> two variables which actually differ just by 1.

They don't merely differ by one if there are holes in the node-id namespace.
Can that happen in practice? I'm not sure; but it's easy enough for the
tools to handle that case, so they might as well.

 -- Keir