[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]