[Libvir] [PATCH] BZ#251641: Allow to change the cpu pinning for inactive domain
Hi, I want to change the cpu pinning for inactive domain on RHEL-5.1. So, I just add the xenXMDomainPinVcpu to xm_internal.c. We will be allowed to change "cpus" parameter in configuration file with "vcpupin" command by this patch, like "setmem" or "setvcpus". There is 2 things to note: - This is an effective feature for inactive domain with Xen3.0.3 (less than 3 of xendConfigVersion). - On the above environment, the number which specified as <vcpu> is ignored, because the virtual CPUs is not present when domain is shut off. So, when executing "vcpupin" command with this option # virsh vcpupin Guest 0 1 "0"(vcpu) is ignored and "1"(cpulist) is set to configuration file as "cpus". # cat /etc/xen/Guest | grep cpus cpus = "1" Regards, Saori Fukuta
On Mon, Nov 26, 2007 at 11:12:29AM +0900, Saori Fukuta wrote:
Hi,
I want to change the cpu pinning for inactive domain on RHEL-5.1. So, I just add the xenXMDomainPinVcpu to xm_internal.c. We will be allowed to change "cpus" parameter in configuration file with "vcpupin" command by this patch, like "setmem" or "setvcpus".
There is 2 things to note: - This is an effective feature for inactive domain with Xen3.0.3 (less than 3 of xendConfigVersion). - On the above environment, the number which specified as <vcpu> is ignored, because the virtual CPUs is not present when domain is shut off. So, when executing "vcpupin" command with this option # virsh vcpupin Guest 0 1 "0"(vcpu) is ignored and "1"(cpulist) is set to configuration file as "cpus". # cat /etc/xen/Guest | grep cpus cpus = "1"
Hi Saori, thanks for looking at this, I think this could be fixed but there is just a couple of things i feel a bit uneasy with the patch: - they work with arrays allocated on the stack of fixed dimention (and rather big), i would rather see a dynamic allocation of the array to a more reasonnable size and growing them dynamically - the repeted use of strcat on each cpu pinning found makes it a quadratic algorithm though it could really be linear if one kept a pointer to the current end of the buffer - the ranges variable is defined in the middle of the function, this may look notmal in C++ but this is against the conventions in libvirt C code. I think the code should be reworked a bit around those lines, but I have no doubt we can quickly get to a fix in libvirt, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, 26 Nov 2007 10:17:41 -0500 Daniel Veillard wrote:
On Mon, Nov 26, 2007 at 11:12:29AM +0900, Saori Fukuta wrote:
I want to change the cpu pinning for inactive domain on RHEL-5.1. So, I just add the xenXMDomainPinVcpu to xm_internal.c. We will be allowed to change "cpus" parameter in configuration file with "vcpupin" command by this patch, like "setmem" or "setvcpus".
thanks for looking at this, I think this could be fixed but there is just a couple of things i feel a bit uneasy with the patch: - they work with arrays allocated on the stack of fixed dimention (and rather big), i would rather see a dynamic allocation of the array to a more reasonnable size and growing them dynamically
Yeah, that may be rather big, thanks for the comment. I changed the code to allocate the memory dynamically. + /* allocate memory for mapstr */ + alloc_sz = (maplen * 32); + mapstr = malloc(alloc_sz); + if (mapstr == NULL) + return (-1); + memset(mapstr, 0, alloc_sz); This is why I set (maplen * 32) as size for malloc. - the limited of maplen is 8 - the maxmam cpu value is 2-digit and need to insert a comma for each cpu (i.e. 0,1,2,....,64), so the maximal length will be 192 - as a result, the size we need is around 32 for each maplen
- the repeted use of strcat on each cpu pinning found makes it a quadratic algorithm though it could really be linear if one kept a pointer to the current end of the buffer
I'm not exactly sure what to do, but you may be concerned about overflowing the actual space, right ? If so, how about this one ? + if (cpumap[i] & (1 << j)) { + len = snprintf(mapstr, alloc_sz, "%s%d,", mapstr, (8 * i) + j); + if (len >= alloc_sz) + goto cleanup; + }
- the ranges variable is defined in the middle of the function, this may look notmal in C++ but this is against the conventions in libvirt C code.
Yes, it makes a lot of sense. I reworked with your comment. Could you check it ? Regards, Saori Fukuta
Morning Saori, You need to call the error function for each error condition, eg: if (mapstr == NULL) { xenXMError (domain ? domain->conn : NULL, VIR_ERR_NO_MEMORY, __FUNCTION__); return -1; } (Much of the current libvirt code gets this wrong, and it's a great deal of pain caused by virterror, but there's no sensible way to change things from here.) The inner loop is still quadratic even with snprintf because you're copying the string each time. I've changed it below to use a virBuffer instead. If you ./configure --enable-compile-warnings=error then any warnings will turn into errors. There was one warning in the patch you sent. Modified version of the patch is attached. **NOTE** I've only compiled it, because I don't have any suitable machine to test it on. Rich. PS. Even using the supposedly simplified virBuffer, this is still a good example of why we should not be writing any more code in C. It's not 1985 any more. The language should take care of garbage collection, typing, strings, structures, error handling, etc. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Nov 27, 2007 at 01:19:48PM +0000, Richard W.M. Jones wrote:
Morning Saori,
You need to call the error function for each error condition, eg:
if (mapstr == NULL) { xenXMError (domain ? domain->conn : NULL, VIR_ERR_NO_MEMORY, __FUNCTION__); return -1; }
(Much of the current libvirt code gets this wrong, and it's a great deal of pain caused by virterror, but there's no sensible way to change things from here.)
The inner loop is still quadratic even with snprintf because you're copying the string each time. I've changed it below to use a virBuffer instead.
If you ./configure --enable-compile-warnings=error then any warnings will turn into errors. There was one warning in the patch you sent.
Modified version of the patch is attached. **NOTE** I've only compiled it, because I don't have any suitable machine to test it on.
That patch looks good to me. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Tue, Nov 27, 2007 at 01:19:48PM +0000, Richard W.M. Jones wrote:
Morning Saori,
You need to call the error function for each error condition, eg:
if (mapstr == NULL) { xenXMError (domain ? domain->conn : NULL, VIR_ERR_NO_MEMORY, __FUNCTION__); return -1; }
(Much of the current libvirt code gets this wrong, and it's a great deal of pain caused by virterror, but there's no sensible way to change things from here.)
The inner loop is still quadratic even with snprintf because you're copying the string each time. I've changed it below to use a virBuffer instead.
If you ./configure --enable-compile-warnings=error then any warnings will turn into errors. There was one warning in the patch you sent.
Modified version of the patch is attached. **NOTE** I've only compiled it, because I don't have any suitable machine to test it on.
That patch looks good to me.
Saori: If you can check that the patch works for you, then we can commit it. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, 27 Nov 2007 14:55:22 +0000 "Richard W.M. Jones" wrote:
Daniel Veillard wrote:
On Tue, Nov 27, 2007 at 01:19:48PM +0000, Richard W.M. Jones wrote:
Morning Saori,
Hello Rich,
You need to call the error function for each error condition, eg:
if (mapstr == NULL) { xenXMError (domain ? domain->conn : NULL, VIR_ERR_NO_MEMORY, __FUNCTION__); return -1; }
The inner loop is still quadratic even with snprintf because you're copying the string each time. I've changed it below to use a virBuffer instead.
If you ./configure --enable-compile-warnings=error then any warnings will turn into errors. There was one warning in the patch you sent.
Modified version of the patch is attached. **NOTE** I've only compiled it, because I don't have any suitable machine to test it on.
Oh, that's great. I like this patch !
Saori:
If you can check that the patch works for you, then we can commit it.
I checked the patch and confirmed that is working properly. Many thanks to all for your help ! Regards, Saori.
Saori Fukuta wrote:
I checked the patch and confirmed that is working properly. Many thanks to all for your help !
OK, that's in CVS now. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (3)
-
Daniel Veillard -
Richard W.M. Jones -
Saori Fukuta