From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 2/5] [POST-4.0]: HVM NUMA guest: pass NUMA information to libxc Date: Mon, 22 Feb 2010 12:06:45 +0100 Message-ID: <4B826545.4040802@amd.com> References: <4B6B420E.2010104@amd.com> <20100205003229.GA5869@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20100205003229.GA5869@phenom.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Konrad Rzeszutek Wilk Cc: "xen-devel@lists.xensource.com" , Keir Fraser List-Id: xen-devel@lists.xenproject.org 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 >> ... >> >> + 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