On 10.11.2014 12:52, Prerna Saxena wrote:
>
> From 1ead736712eec3bd098daf222a872c67b67e94ce Mon Sep 17 00:00:00 2001
> From: Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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 @@
> <cpu>
> ...
> <numa>
> - <cell id='0' cpus='0-3' memory='512000'/>
> - <cell id='1' cpus='4-7' memory='512000' memAccess='shared'/>
> + <cell id='0' cpus='0-3' memory='512000' unit='KiB'/>
> + <cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/>
> </numa>
> ...
> </cpu>
> @@ -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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list