Re: [Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch
oops... meant to post to list as well beth kon wrote:
Daniel Veillard wrote:
On Thu, Sep 20, 2007 at 05:10:49PM -0400, beth kon wrote:
Here is my first pass at a patch for accessing the available memory associated with each NUMA cell through libvirt.
Thanks !
Thanks for the quick comments!
+struct xen_v2s4_availheap { + uint32_t min_bitwidth; /* Smallest address width (zero if don't care). */ + uint32_t max_bitwidth; /* Largest address width (zero if don't care). */
I'm a bit puzzled by those 2 fields I still wonder what it is about :-)
I was puzzled too! These fields are related to the case when 32 bit guests need to reserve certain ranges of memory for DMA, for example. I checked with Ryan and he assured me we don't need to worry about those fields for these purposes.
+ int32_t node; /* NUMA node (-1 for sum across all nodes). */ + uint64_t avail_bytes; /* Bytes available in the specified region. */ +}; + +typedef struct xen_v2s4_availheap xen_v2s4_availheap; +
[...]
What does [...] mean? :-)
+xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, long long *freeMems, + int startCell, int maxCells) { - if ((conn == NULL) || (freeMems == NULL) || (nbCells < 0)) - return -1; + xen_op_v2_sys op_sys; + int i, ret, nbCells; + virNodeInfo nodeInfo; + virNodeInfoPtr nodeInfoPtr = &nodeInfo; + xenUnifiedPrivatePtr priv; + + if ((conn == NULL) || (freeMems == NULL) || (maxCells < 0)) + return -1;
Hum, actually, that would catch the (maxCells == -1) so that won't work and won't catch maxCells == 0 which could lead to a segfault. Maybe (maxCells == 0) || (maxCells < -1) should be used instead.
Yes, you're right. I added maxcells = -1 and startCell as an afterthought, and it shows!
+ /* get actual number of cells */ + if (xenDaemonNodeGetInfo(conn, nodeInfoPtr)) {
Hum, since the number of cells is static, maybe this should be stored permanently in a variable on init. The xenDaemon RPC will be orders of magnitude more expensive than the direct hypercall below.
+ virXenError(VIR_ERR_XEN_CALL, " cannot determine actual number of cells", 0); + return -1; + } + nbCells = nodeInfoPtr->nodes; + + if (maxCells > nbCells) + maxCells = nbCells;
I wondered about that. Would you like me to make that change as part of this patch?
Hum ... maxCells is the number of entries in the array, I'm afraid you will need 2 counters or I misunderstood the way the API works (possible :-), I would fill in freeMems[] starting at index 0 for startCell, and going up. That feels more coherent than leaving uninitialized values at the start of the array:
for (i = startCell, j = 0;(i < nbCells) && (j < maxCells);i++,j++) { op_sys.u.availheap.node = i; ret = xenHypervisorDoV2Sys(priv->handle, &op_sys); if (ret < 0) return(-1); freeMems[j] = op_sys.u.availheap.avail_bytes; } return (j);
Yes, right again!
Thanks so much for the great feedback!
-- Elizabeth Kon (Beth) IBM Linux Technology Center Open Hypervisor Team email: eak@us.ibm.com
On Fri, Sep 21, 2007 at 10:59:01AM -0400, beth kon wrote:
oops... meant to post to list as well beth kon wrote:
and my reply, I didn't realized it was off-list :-) On Fri, Sep 21, 2007 at 09:44:36AM -0400, beth kon wrote:
Daniel Veillard wrote:
On Thu, Sep 20, 2007 at 05:10:49PM -0400, beth kon wrote:
Here is my first pass at a patch for accessing the available memory associated with each NUMA cell through libvirt.
Thanks !
Thanks for the quick comments!
+struct xen_v2s4_availheap { + uint32_t min_bitwidth; /* Smallest address width (zero if don't care). */ + uint32_t max_bitwidth; /* Largest address width (zero if don't care). */
I'm a bit puzzled by those 2 fields I still wonder what it is about :-)
I was puzzled too! These fields are related to the case when 32 bit guests need to reserve certain ranges of memory for DMA, for example. I checked with Ryan and he assured me we don't need to worry about those fields for these purposes.
ah, okay
+ int32_t node; /* NUMA node (-1 for sum across all nodes). */ + uint64_t avail_bytes; /* Bytes available in the specified region. */ +}; + +typedef struct xen_v2s4_availheap xen_v2s4_availheap; +
[...]
What does [...] mean? :-)
that I deleted some of the original input from your mail there :-)
+xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, long long *freeMems, + int startCell, int maxCells) { - if ((conn == NULL) || (freeMems == NULL) || (nbCells < 0)) - return -1; + xen_op_v2_sys op_sys; + int i, ret, nbCells; + virNodeInfo nodeInfo; + virNodeInfoPtr nodeInfoPtr = &nodeInfo; + xenUnifiedPrivatePtr priv; +
+ if ((conn == NULL) || (freeMems == NULL) || (maxCells < 0)) + return -1;
Hum, actually, that would catch the (maxCells == -1) so that won't work and won't catch maxCells == 0 which could lead to a segfault. Maybe (maxCells == 0) || (maxCells < -1) should be used instead.
Yes, you're right. I added maxcells = -1 and startCell as an afterthought, and it shows!
okay
+ /* get actual number of cells */ + if (xenDaemonNodeGetInfo(conn, nodeInfoPtr)) {
Hum, since the number of cells is static, maybe this should be stored permanently in a variable on init. The xenDaemon RPC will be orders of magnitude more expensive than the direct hypercall below.
+ virXenError(VIR_ERR_XEN_CALL, " cannot determine actual number of cells", 0); + return -1; + } + nbCells = nodeInfoPtr->nodes; + + if (maxCells > nbCells) + maxCells = nbCells;
I wondered about that. Would you like me to make that change as part of this patch?
yes if you feel it's not too much to add, sure !
Hum ... maxCells is the number of entries in the array, I'm afraid you will need 2 counters or I misunderstood the way the API works (possible :-), I would fill in freeMems[] starting at index 0 for startCell, and going up. That feels more coherent than leaving uninitialized values at the start of the array:
for (i = startCell, j = 0;(i < nbCells) && (j < maxCells);i++,j++) { op_sys.u.availheap.node = i; ret = xenHypervisorDoV2Sys(priv->handle, &op_sys); if (ret < 0) return(-1); freeMems[j] = op_sys.u.availheap.avail_bytes; } return (j);
Yes, right again!
Ah, okay :-) I was wondering if I had misunderstood instead ! 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/
participants (2)
-
beth kon -
Daniel Veillard