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

Re: [libvirt] [PATCH v3 2/2] Numa : Allow specification of 'units' for numa nodes.



On 10.11.2014 12:52, Prerna Saxena wrote:
> 
>  From 1ead736712eec3bd098daf222a872c67b67e94ce Mon Sep 17 00:00:00 2001
> From: Prerna Saxena <prerna linux vnet ibm com>
> Date: Mon, 3 Nov 2014 07:53:59 +0530
> 
> 
> CPU numa topology implicitly allows memory specification in 'KiB'.
> 
> Enabling this to accept the 'unit' in which memory needs to be specified.
> This now allows users to specify memory in units of choice, and
> lists the same in 'KiB' -- just like other 'memory' elements in XML.
> 
>      <numa>
>        <cell cpus='0-3' memory='1024' unit='MiB' />
>        <cell cpus='4-7' memory='1024' unit='MiB' />
>      </numa>
> 
> Also augment test cases to correctly model NUMA memory specification.
> This adds the tag 'unit="KiB"' for memory attribute in NUMA cells.
> 
> Signed-off-by: Prerna Saxena <prerna linux vnet ibm com>
> ---
>   docs/formatdomain.html.in                          |  8 +++-
>   docs/schemas/domaincommon.rng                      |  5 +++
>   src/conf/cpu_conf.c                                | 44 ++++++++++++++--------
>   .../qemuxml2argv-cpu-numa-disjoint.xml             |  4 +-
>   .../qemuxml2argv-cpu-numa-memshared.xml            |  4 +-
>   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml  |  4 +-
>   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml  |  4 +-
>   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml  |  4 +-
>   .../qemuxml2argv-hugepages-pages.xml               |  8 ++--
>   .../qemuxml2argv-hugepages-pages2.xml              |  4 +-
>   .../qemuxml2argv-hugepages-pages3.xml              |  4 +-
>   .../qemuxml2argv-hugepages-pages4.xml              |  8 ++--
>   .../qemuxml2argv-hugepages-shared.xml              |  8 ++--
>   .../qemuxml2argv-numatune-auto-prefer.xml          |  2 +-
>   .../qemuxml2argv-numatune-memnode-no-memory.xml    |  4 +-
>   .../qemuxml2argv-numatune-memnode.xml              |  6 +--
>   .../qemuxml2argv-numatune-memnodes-problematic.xml |  4 +-
>   .../qemuxml2xmlout-cpu-numa1.xml                   |  4 +-
>   .../qemuxml2xmlout-cpu-numa2.xml                   |  4 +-
>   .../qemuxml2xmlout-numatune-auto-prefer.xml        |  2 +-
>   .../qemuxml2xmlout-numatune-memnode.xml            |  6 +--
>   21 files changed, 81 insertions(+), 60 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 7196e75..f103a13 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1153,8 +1153,8 @@
>     &lt;cpu&gt;
>       ...
>       &lt;numa&gt;
> -      &lt;cell id='0' cpus='0-3' memory='512000'/&gt;
> -      &lt;cell id='1' cpus='4-7' memory='512000' memAccess='shared'/&gt;
> +      &lt;cell id='0' cpus='0-3' memory='512000' unit='KiB'/&gt;
> +      &lt;cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/&gt;
>       &lt;/numa&gt;
>       ...
>     &lt;/cpu&gt;
> @@ -1165,6 +1165,10 @@
>         <code>cpus</code> specifies the CPU or range of CPUs that are
>         part of the node. <code>memory</code> specifies the node memory
>         in kibibytes (i.e. blocks of 1024 bytes).
> +      <span class="since">Since 1.2.11</span> one can specify an additional
> +      <code>unit</code> attribute to describe the node memory unit.
> +      The detailed syntax for allocation of memory units follows:
> +      <a href="#elementsMemoryAllocation"><code>unit</code></a>
>         <span class="since">Since 1.2.7</span> all cells should
>         have <code>id</code> attribute in case referring to some cell is
>         necessary in the code, otherwise the cells are
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 20d81ae..44cabad 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4144,6 +4144,11 @@
>           <ref name="memoryKB"/>
>         </attribute>
>         <optional>
> +        <attribute name="unit">
> +          <ref name="unit"/>
> +        </attribute>
> +      </optional>
> +      <optional>
>           <attribute name="memAccess">
>             <choice>
>               <value>shared</value>
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index 1c74c66..d0323b0 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -177,6 +177,30 @@ virCPUDefCopy(const virCPUDef *cpu)
>       return NULL;
>   }
>   
> +static int
> +virCPUNumaCellMemoryParseXML(xmlNodePtr node,
> +                  xmlXPathContextPtr ctxt,
> +                  unsigned long long* cellMemory)
> +{
> +    int ret = -1;
> +    xmlNodePtr oldnode = ctxt->node;
> +
> +    ctxt->node = node;
> +
> +    if (virDomainParseMemory("./@memory", "./@unit", ctxt,
> +                             cellMemory, true, false) < 0) {
> +        virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                       _("Unable to parse NUMA memory size attribute"));

There's no need for virReportError() here. The helper reported error already.

> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    ctxt->node = oldnode;
> +    return ret;
> +
> +}
> +

Huh, I don't think it's necessary to have this as a function, but compiler will surely optimize it. So I can live with this as-is.

>   virCPUDefPtr
>   virCPUDefParseXML(xmlNodePtr node,
>                     xmlXPathContextPtr ctxt,
> @@ -440,7 +464,7 @@ virCPUDefParseXML(xmlNodePtr node,
>           def->ncells = n;
>   
>           for (i = 0; i < n; i++) {
> -            char *cpus, *memory, *memAccessStr;
> +            char *cpus, *memAccessStr;
>               int ret, ncpus = 0;
>               unsigned int cur_cell;
>               char *tmp = NULL;
> @@ -489,21 +513,8 @@ virCPUDefParseXML(xmlNodePtr node,
>                   goto error;
>               def->cells_cpus += ncpus;
>   
> -            memory = virXMLPropString(nodes[i], "memory");
> -            if (!memory) {
> -                virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("Missing 'memory' attribute in NUMA cell"));
> -                goto error;
> -            }
> -
> -            ret = virStrToLong_ull(memory, NULL, 10, &def->cells[cur_cell].mem);
> -            if (ret == -1) {
> -                virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("Invalid 'memory' attribute in NUMA cell"));
> -                VIR_FREE(memory);
> -                goto error;
> -            }
> -            VIR_FREE(memory);
> +            virCPUNumaCellMemoryParseXML(nodes[i],
> +                                          ctxt, &def->cells[cur_cell].mem);

What I can't live with is ignoring return value here.

>   
>               memAccessStr = virXMLPropString(nodes[i], "memAccess");
>               if (memAccessStr) {
> @@ -704,6 +715,7 @@ virCPUDefFormatBuf(virBufferPtr buf,
>               virBufferAsprintf(buf, " id='%zu'", i);
>               virBufferAsprintf(buf, " cpus='%s'", def->cells[i].cpustr);
>               virBufferAsprintf(buf, " memory='%llu'", def->cells[i].mem);
> +            virBufferAddLit(buf, " unit='KiB'");
>               if (memAccess)
>                   virBufferAsprintf(buf, " memAccess='%s'",
>                                     virMemAccessTypeToString(memAccess));

So ACK with this squashed in:
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index d0323b0..0604eab 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -513,8 +513,9 @@ virCPUDefParseXML(xmlNodePtr node,
                 goto error;
             def->cells_cpus += ncpus;
 
-            virCPUNumaCellMemoryParseXML(nodes[i],
-                                          ctxt, &def->cells[cur_cell].mem);
+            if (virCPUNumaCellMemoryParseXML(nodes[i], ctxt,
+                                             &def->cells[cur_cell].mem) < 0)
+                goto error;
 
             memAccessStr = virXMLPropString(nodes[i], "memAccess");
             if (memAccessStr) {

However, since I need to modify the patch anyway, I'm gonna drop the virCPUNumaCellMemoryParseXML() function and call virDomainParseMemory() directly.

ACK once I fix the issues.

Michal


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