8:12 p.m.
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
9:17 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6:33 a.m.
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
7:19 a.m.
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
8:27 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
8:55 a.m.
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
6:58 p.m.
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.
4:09 a.m.
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
6203
days inactive
6205
days old
7 comments
3 participants
participants (3)
-
Daniel Veillard -
Richard W.M. Jones -
Saori Fukuta