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!