[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/5] [POST-4.0]: HVM NUMA guest: pass NUMA information to libxc



Konrad Rzeszutek Wilk wrote:
On Thu, Feb 04, 2010 at 10:54:22PM +0100, Andre Przywara wrote:

Konrad, thanks for your review. Comments inline.

This patch adds a "struct numa_guest_info" to libxc, which allows to specify a guest's NUMA topology along with the appropriate host binding. Here the information will be filled out by the Python wrapper (I left out the libxl part for now), it will fill up the struct with sensible default values (equal distribution), only the number of guest nodes should be specified by the caller. The information is passed on to the hvm_info_table. The respective memory allocation is in another patch.

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

+    if (numanodes > 1)
+    {
+        int vcpu_per_node, remainder;
+        int n;
+        vcpu_per_node = vcpus / numanodes;
+        remainder = vcpus % numanodes;
+        n = remainder * (vcpu_per_node + 1);
+        for (i = 0; i < vcpus; i++)

I don't know the coding style, but you seem to have two different
version of it. Here you do the 'for (i= ..')
It seems that Xen does not have a consistent coding style in this respect, I have seen both versions in Xen already. I usually do coding-style-by-copying, looking at similar nearby statements and applying the style to the new one (useful if you do code changes in Xen HV, tools, ioemu, etc.). That seemed to fail here.

Keir, is there a definite rule for the "space after brace" issue?

+        {
+            if (i < n)
+                numainfo.vcpu_to_node[i] = i / (vcpu_per_node + 1);
+            else
+                numainfo.vcpu_to_node[i] = remainder + (i - n) / vcpu_per_node;
+        }
+        for (i = 0; i < numanodes; i++)
+            numainfo.guest_to_host_node[i] = i % 2;
+        if ( nodemem_handle != NULL && PyList_Check(nodemem_handle) )
+        {

+            PyObject *item;
+            for ( i = 0; i < numanodes; i++)

and here it is ' for ( i = 0 ..')'.

I've failed in the past to find the coding style so I am not exactly
sure which one is correct.
+            {
+                item = PyList_GetItem(nodemem_handle, i);
+                numainfo.node_mem[i] = PyInt_AsLong(item);
+                fprintf(stderr, "NUMA: node %i has %lu kB\n", i,
+                    numainfo.node_mem[i]);

There isn't any other way to print this out? Can't you use xc_dom_printf
routines?
Probably yes, although I am not sure whether this really belongs into upstream code, I think this one is too verbose for most of the users, so I consider this a leftover from debugging. I will remove it in the next version.

+            }
+        } else {
+            unsigned int mempernode;
+            if ( nodemem_handle != NULL && PyInt_Check(nodemem_handle) )
+                mempernode = PyInt_AsLong(nodemem_handle);
+            else
+                mempernode = (memsize << 10) / numanodes;
+            fprintf(stderr, "NUMA: setting all nodes to %i KB\n", mempernode);

dittoo..
+            for (i = 0; i < numanodes; i++)

dittoo for the coding guideline.
+                numainfo.node_mem[i] = mempernode;
+        }
+    }
+
+    if ( xc_hvm_build_target_mem(self->xc_handle, dom, memsize, target,
+                                 &numainfo, image) != 0 )

and I guess here too
I am glad that most of your objections are coding-style related ;-)

         return pyxc_error_to_exception();
#if !defined(__ia64__)
@@ -970,6 +1016,15 @@ static PyObject *pyxc_hvm_build(XcObject *self,
     va_hvm->apic_mode    = apic;
     va_hvm->nr_vcpus     = vcpus;
     memcpy(va_hvm->vcpu_online, vcpu_avail, sizeof(vcpu_avail));
+
+    va_hvm->num_nodes    = numanodes;
+    if (numanodes > 0) {
+        for (i = 0; i < vcpus; i++)
+            va_hvm->vcpu_to_node[i] = numainfo.vcpu_to_node[i];
+        for (i = 0; i < numanodes; i++)
+            va_hvm->node_mem[i] = numainfo.node_mem[i] >> 10;

Would it make sense to have 10 be #defined somewhere?
I think it is not worth, since it is just the conversion from MB to KB. After all it maybe wiser to use a consistent unit in all parts of the code. Will think about it.

+    }
+
     for ( i = 0, sum = 0; i < va_hvm->length; i++ )
         sum += ((uint8_t *)va_hvm)[i];
     va_hvm->checksum -= sum;
@@ -1174,7 +1229,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
             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}",
+    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}",

what changed in that line? My eyes don't seem to be able to find the
difference.
The last comma in the plus line was a colon in the minus line.
The Python doc says that these delimiters are ignored by the parser and are just for the human eye. I was confused on the first glance and found that the next to last colon is wrong. Actually it does not belong in this patch, but I didn't found it worth to be a separate patch.

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


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.