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/