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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/