Re: [Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch

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!