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

[Andre Przywara <andre.przywara@amd.com>]

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@amd.com>
 >> ...
>>
>> +    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