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 ! > The initial framework patch provided by Daniel Veillard is a prereq of > this patch: > https://www.redhat.com/archives/libvir-list/2007-September/msg00069.html That applied fine to my tree using CVS HEAD + the patch. > I have not yet tested this. I'm just distributing for comments. > > A few comments/questions: > 1) I think I got the versioning stuff right but that deserves a close look. I looked at it, it looks fine but need testing for the differentiation between 3.1 which doesn't have the feature and current xen-unstable which does. > 2) In Daniel's patch, libvirt.h and libvirt.h.in were identical. I nearly identical, yes > assumed the patch would be before running autogen.sh, and only contain > changes in libvirt.h.in, so that's how I did mine. Let me know if I > misunderstand something here. That's right, I just provided both as that's what CVS diff gave after autogen, no problem here. > 3) I had to put #ifdef PROXY around calls to virXenErrorFunc in > xenHypervisorNodeGetCellsFreeMemory to get this to build. I haven't > figured out how the proxy code works so am not sure if this is the right > approach. It compiles only a subset of the xen_internal.c code. Don't worry too much about it, the proxy code fate is to disapear at some point. > int virNodeGetCellsFreeMemory(virConnectPtr conn, > - unsigned long *freeMems, > - int nbCells); > + long long *freeMems, > + int startCell, > + int maxCells); okay, so switch to long long and add a start cell number, perfect. > /** > * virNodeGetCellFreeMemory: > + > * @conn: pointer to the hypervisor connection > - * @freeMems: pointer to the array of unsigned long > - * @nbCells: number of entries available in freeMems > + * @freeMems: pointer to the array of long long > + * @startCell: index of first cell to return freeMems info on (0 to > + * maxCells - 1). > + * @maxCells: number of entries available in freeMems (-1 for sum of > + * free memory across all cells, returned in freeMems[0]) > * > - * This call allows to ask the amount of free memory in each NUMA cell. > + * This call is a request for the amount of free memory, either in the whole > + * node, or in each NUMA cell. > * The @freeMems array must be allocated by the caller and will be filled > * with the amounts of free memory in kilobytes for each cell starting > - * from cell #0 and up to @nbCells -1 or the number of cell in the Node > + * from cell #0 and up to @maxCells -1 or the number of cells in the Node > * (which can be found using virNodeGetInfo() see the nodes entry in the > * structure). > + * If maxCells == -1, the free memory will be summed across all cells and > + * returned in freeMems[0]. Hum, that -1 handling is a bit surprizing, but apparently that's what the Xen hypercall does. It's a bit convoluted as an API to get the full free memory on a Node, maybe this should be separated as a different API as people looking for this information may not think about looking at NUMA special entry points. We don't have currently a simple API to get the free memory on a Node, maybe that should be added. > +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 :-) > + 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; > + [...] > @@ -2012,7 +2028,7 @@ xenHypervisorInit(void) > #endif > return(-1); > } > - /* Currently consider RHEL5.0 Fedora7 and xen-unstable */ > + /* Currently consider RHEL5.0 Fedora7, xen-3.1, and xen-unstable */ > sys_interface_version = 2; /* XEN_SYSCTL_INTERFACE_VERSION */ > if (virXen_getdomaininfo(fd, 0, &info) == 1) { > /* RHEL 5.0 */ > @@ -2035,7 +2051,7 @@ xenHypervisorInit(void) > > sys_interface_version = 3; /* XEN_SYSCTL_INTERFACE_VERSION */ > if (virXen_getdomaininfo(fd, 0, &info) == 1) { > - /* xen-unstable */ > + /* xen-3.1 */ > dom_interface_version = 5; /* XEN_DOMCTL_INTERFACE_VERSION */ > if (virXen_getvcpusinfo(fd, 0, 0, ipt, NULL, 0) == 0){ > #ifdef DEBUG > @@ -2045,6 +2061,18 @@ xenHypervisorInit(void) > } > } > > + sys_interface_version = 4; /* XEN_SYSCTL_INTERFACE_VERSION */ > + if (virXen_getdomaininfo(fd, 0, &info) == 1) { > + /* xen-unstable */ > + dom_interface_version = 5; /* XEN_DOMCTL_INTERFACE_VERSION */ > + if (virXen_getvcpusinfo(fd, 0, 0, ipt, NULL, 0) == 0){ > +#ifdef DEBUG > + fprintf(stderr, "Using hypervisor call v2, sys ver4 dom ver5\n"); > +#endif > + goto done; > + } > + } > + > hypervisor_version = 1; > sys_interface_version = -1; > if (virXen_getdomaininfo(fd, 0, &info) == 1) { That looks right, I agree, but need testing for example on a normal F-7 which has 3.1 and one with updated xen. > +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. > + /* 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; 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); > + > + if (startCell > nbCells - 1) > + return -1; > + > + for (i = startCell; i < maxCells; i++) { > + op_sys.u.availheap.node = i; > + ret = xenHypervisorDoV2Sys(priv->handle, &op_sys); > + if (ret < 0) > + return(-1); > + freeMems[i] = op_sys.u.availheap.avail_bytes; > + } > + return (maxCells - startCell); > } > A few things to check, but that's looks like a good start :-) thanks a lot ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list