[Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch

[Daniel Veillard]

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