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@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