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

Re: [Libvir] [RFC][PATCH 2/2] NUMA memory and topology patches



On Mon, Sep 24, 2007 at 11:30:53PM -0400, beth kon wrote:
> [PATCH 2/2] - add capability to access topology information (cell to cpu 
> mapping) for each numa cell.
> 
> Signed-off-by: Beth Kon (eak us ibm com)
> 
> -- 
> Elizabeth Kon (Beth)
> IBM Linux Technology Center
> Open Hypervisor Team
> email: eak us ibm com
> 

> diff -urpN libvirt.cellsMemory/src/xend_internal.c libvirt.topology/src/xend_internal.c
> --- libvirt.cellsMemory/src/xend_internal.c	2007-09-24 21:51:30.000000000 -0400
> +++ libvirt.topology/src/xend_internal.c	2007-09-24 23:09:20.000000000 -0400
> @@ -30,6 +30,7 @@
>  #include <arpa/inet.h>
>  #include <netdb.h>
>  #include <libxml/uri.h>
> +#include <ctype.h>
>  #include <errno.h>
>  
>  #include "libvirt/libvirt.h"
> @@ -1873,6 +1874,181 @@ sexpr_to_xend_node_info(struct sexpr *ro
>      return (0);
>  }
>  
> +/**
> + * getNumber:
> + * @pointer: pointer to string beginning with  numerical characters
> + * @result: pointer to integer for storing the numerical result
> + *
> + * Internal routine extracting a number from the beginning of a string
> + *
> + * Returns the number of characters that were extracted as digits
> + * or -1 if no digits were found.
> + */
> +static int
> +getNumber (const char * pointer, int * result) {
> +    int len = 0;
> +    while (isdigit(*(pointer + len)))
> +        len++;
> +    if (len == 0)
> +        return -1;
> +    *(result) = atoi(pointer);
> +    return (len);
> +}

  I'm always a bit vary of libc isXXXX since they tend to fluctuate based on
locale while you don't expect so when parsing some input. In that case it's
safe though. 

> +/**
> + * sexpr_to_xend_topology_xml:
> + * @root: an S-Expression describing a node
> + *
> + * Internal routine creating an XML string with the values from
> + * the node root provided.
> + *
> + * Returns 0 in case of success, -1 in case of error
> + */
> +static int 
> +sexpr_to_xend_topology_xml(virConnectPtr conn, struct sexpr *root, virBufferPtr xml)
> +{
> +    const char *nodeToCpu, *offset;
> +    int cellNum, numCells = 0, numCpus, cellCpuCount = 0, nodeCpuCount = 0, start, finish, r;
> +    int i, len, cpuNum, *cpuIdsPtr = NULL, *iCpuIdsPtr = NULL;
> +    char next;
> +
> +    nodeToCpu = sexpr_node(root, "node/node_to_cpu");
> +    if (nodeToCpu == NULL) {
> +        virXendError(conn, VIR_ERR_INTERNAL_ERROR,
> +                     _("failed to parse topology information"));
> +        goto error;
> +    }

  So if the host is not NUMA or an older Xen version, will you get that
error back to the user ? 

> +    numCells = sexpr_int(root, "node/nr_nodes");
> +    numCpus = sexpr_int(root, "node/nr_cpus");
> +
> +    /* array for holding all cpu numbers associated with a single cell. Should never need 
> +       more than numCpus (which is total number of cpus for the node) */
> +    cpuIdsPtr = iCpuIdsPtr = malloc(numCpus * sizeof(int));
> +    if (cpuIdsPtr == NULL) {
> +        goto vir_buffer_failed;
> +    }
> +
> +    /* start filling in xml */
> +    r = virBufferVSprintf (xml,
> +                               "\
> +<topology>\n\
> +  <cells num='%d'>\n",
> +                           numCells);

  I know this indentation was borrowed from other code, but this hurts my love
for proper indentation :-)

> +    if (r == -1) goto vir_buffer_failed;
> +
> +    offset = nodeToCpu;
> +    /* now iterate through all cells and find associated cpu ids */
> +    /* example of string being parsed: "node0:0-3,7,9-10\n   node1:11-14\n" */

  For multi lines comments we usually use
  /*
   * .....
   * .....
   */
> +    while ((offset = strstr(offset, "node")) != NULL) {
    should you just skip blanks there instead ?

> +        cpuIdsPtr = iCpuIdsPtr;
> +        cellCpuCount = 0;
> +        offset +=4;
> +        if ((len = getNumber(offset, &cellNum)) < 0) {
> +            virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +            goto error;
> +        }
> +        offset += len;
> +        if (*(offset) != ':') {
> +            virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +            goto error;
> +        }
> +        offset++;
> +        /* get list of cpus associated w/ single cell */
> +        while (1) {
> +            if ((len = getNumber(offset, &cpuNum)) < 0) {
> +                virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +                goto error;
> +            }
> +            offset += len;
> +            next = *(offset);
> +            if (next == '-') {
> +                offset++;
> +                start = cpuNum;
> +                if ((len = getNumber(offset, &finish)) < 0) {
> +                    virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +                    goto error;
> +                }
> +                if (start > finish) {
> +                    virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +                    goto error;
> +
> +                }
> +                for (i=start; i<=finish && nodeCpuCount<numCpus; i++) {
> +                    *(cpuIdsPtr++) = i;
> +                    cellCpuCount++;
> +                    nodeCpuCount++;
> +                }
> +                if (nodeCpuCount >= numCpus) {
> +                    virXendError(conn, VIR_ERR_XEN_CALL, "conflicting cpu counts");
> +                    goto error;
> +                }
> +                offset += len;
> +                next = *(offset);
> +                offset++;
> +                if (next == ',') {
> +                    continue;
> +                } else if (next == '\n') {
> +                    break;
> +                } else {
> +                    virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +                    goto error;
> +                }
> +            } else {
> +                /* add the single number */
> +                if (nodeCpuCount >= numCpus) {
> +                    virXendError(conn, VIR_ERR_XEN_CALL, "conflicting cpu counts");
> +                    goto error;
> +                }
> +                *(cpuIdsPtr) = cpuNum;
> +                cpuIdsPtr++;
> +                cellCpuCount++;
> +                nodeCpuCount++;
> +                if (next == ',') {
> +                    offset++;
> +                    continue;
> +                } else if (next == '\n') {
> +                    break;
> +                } else {
> +                    virXendError(conn, VIR_ERR_XEN_CALL, " topology string syntax error");
> +                    goto error;
> +                }
> +            }
> +        }    

  Loops look fine from visual inspection

> +        /* add xml for all cpus associated with one cell */
> +        r = virBufferVSprintf (xml, "\
> +     <cell id='%d'>\n\
> +        <cpus num='%d'>\n", cellNum, cellCpuCount);
> +        if (r == -1) goto vir_buffer_failed;
> +
> +        for (i = 0; i < cellCpuCount; i++) {
> +            r = virBufferVSprintf (xml, "\
> +           <cpu id='%d'/>\n", *(iCpuIdsPtr + i));
> +        if (r == -1) goto vir_buffer_failed;
> +        }
> +        r = virBufferAdd (xml, "\
> +      </cpus>\n\
> +    </cell>\n", -1);
> +        if (r == -1) goto vir_buffer_failed;
> +    }            
> +    r = virBufferAdd (xml, "\
> +  </cells>\n\
> +</topology>\n", -1);
> +    if (r == -1) goto vir_buffer_failed;
> +    free(cpuIdsPtr);
> +    return (0);
> +    
> +
> +vir_buffer_failed:
> +    virXendError(conn, VIR_ERR_NO_MEMORY, _("allocate new buffer"));
> +        
> +error:
> +    if (cpuIdsPtr)
> +        free(cpuIdsPtr);
> +    return (-1);
> +}
> +
>  #ifndef PROXY
>  /**
>   * sexpr_to_domain:
> @@ -2601,6 +2777,39 @@ xenDaemonNodeGetInfo(virConnectPtr conn,
>      return (ret);
>  }
>  
> +/**
> + * xenDaemonNodeGetTopology:
> + * @conn: pointer to the Xen Daemon block

   * @xml: buffer where the resulting XML will be stored

> + *
> + * This method retrieves a node's topology information.
> + *
> + * Returns -1 in case of error, 0 otherwise.
> + */
> +int
> +xenDaemonNodeGetTopology(virConnectPtr conn, virBufferPtr xml) {
> +    int ret = -1;
> +    struct sexpr *root;
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virXendError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        return (-1);
> +    }
> +
> +    if (xml == NULL) {
> +        virXendError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        return (-1);
> +        }
> +
> +    root = sexpr_get(conn, "/xend/node/");
> +    if (root == NULL) {
> +        return (-1);
> +    }
> +
> +    ret = sexpr_to_xend_topology_xml(conn, root, xml);
> +    sexpr_free(root);
> +    return (ret);
> +}
> +
>  #ifndef PROXY
>  /**
>   * xenDaemonGetType:
> diff -urpN libvirt.cellsMemory/src/xend_internal.h libvirt.topology/src/xend_internal.h
> --- libvirt.cellsMemory/src/xend_internal.h	2007-09-10 17:35:39.000000000 -0400
> +++ libvirt.topology/src/xend_internal.h	2007-09-24 22:32:53.000000000 -0400
> @@ -19,6 +19,7 @@
>  #include <stdbool.h>
>  
>  #include "libvirt/libvirt.h"
> +#include "buf.h"
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -182,6 +183,7 @@ int xenDaemonOpen(virConnectPtr conn, co
>  int xenDaemonClose(virConnectPtr conn);
>  int xenDaemonGetVersion(virConnectPtr conn, unsigned long *hvVer);
>  int xenDaemonNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
> +int xenDaemonNodeGetTopology(virConnectPtr conn, virBufferPtr xml);
>  int xenDaemonDomainSuspend(virDomainPtr domain);
>  int xenDaemonDomainResume(virDomainPtr domain);
>  int xenDaemonDomainShutdown(virDomainPtr domain);
> diff -urpN libvirt.cellsMemory/src/xen_internal.c libvirt.topology/src/xen_internal.c
> --- libvirt.cellsMemory/src/xen_internal.c	2007-09-24 22:04:05.000000000 -0400
> +++ libvirt.topology/src/xen_internal.c	2007-09-24 22:32:53.000000000 -0400
> @@ -2228,7 +2228,7 @@ xenHypervisorMakeCapabilitiesXML(virConn
>      char line[1024], *str, *token;
>      regmatch_t subs[4];
>      char *saveptr = NULL;
> -    int i, r;
> +    int i, r, topology;
>  
>      char hvm_type[4] = ""; /* "vmx" or "svm" (or "" if not in CPU). */
>      int host_pae = 0;
> @@ -2466,6 +2466,10 @@ xenHypervisorMakeCapabilitiesXML(virConn
>    </guest>\n", -1);
>          if (r == -1) goto vir_buffer_failed;
>      }
> +    topology = xenDaemonNodeGetTopology(conn, xml);
> +    if (topology != 0) 
> +        goto topology_failed;
> +
>      r = virBufferAdd (xml,
>                        "\
>  </capabilities>\n", -1);
> @@ -2478,6 +2482,7 @@ xenHypervisorMakeCapabilitiesXML(virConn
>  
>   vir_buffer_failed:
>      virXenError(VIR_ERR_NO_MEMORY, __FUNCTION__, 0);
> + topology_failed:
>      virBufferFree (xml);
>      return NULL;
>  }

  Okay, end looks fine to me.
Now we just need to check if this works :-)

  thanks !

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]