[libvirt] [PATCH v2] Memory : Allow specification of 'units' for numa nodes.
Michal Privoznik
mprivozn at redhat.com
Fri Nov 7 15:11:22 UTC 2014
On 07.11.2014 12:17, Prerna Saxena wrote:
> Reference :
> =======
> v1: http://www.spinics.net/linux/fedora/libvir/msg105516.html
>
> Changes since v1:
> ===========
> 1) As suggested by Michal, a new function "virCPUNumaCellMemoryParseXML" has
> been introduced for neat computation of NUMA memory.
> 2) Patches 2 & 3 of v1 have been merged together into this cumulative patch.
> 3) Corresponding documentation added.
>
> From 2199d97b88cf9eab29788fb0e440c4b0a0bb23ec Mon Sep 17 00:00:00 2001
> From: Prerna Saxena <prerna at 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 at linux.vnet.ibm.com>
> ---
> docs/formatdomain.html.in | 6 ++-
> docs/schemas/domaincommon.rng | 5 ++
> src/conf/cpu_conf.c | 62 ++++++++++++++++------
> .../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, 97 insertions(+), 60 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0099ce7..24afc87 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1140,8 +1140,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>
> @@ -1152,6 +1152,8 @@
> <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.
I'd make "<code>unit</code>" a link to that part of docs, which explains
what values are accepted and what scale they refer to.
> <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 5475c07..a0a60c8 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -177,6 +177,48 @@ virCPUDefCopy(const virCPUDef *cpu)
> return NULL;
> }
>
> +static int
> +virCPUNumaCellMemoryParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + unsigned long long* cellMemory)
> +{
> + int ret = -1;
> + xmlNodePtr oldnode = ctxt->node;
> + unsigned long long bytes, max;
> + char *unit = NULL;
> +
> + ctxt->node = node;
> +
> + /* 32 vs 64 bit will differ in value of upper memory bound.
> + * On 32-bit machines, our bound is 0xffffffff * KiB. On 64-bit
> + * machines, our bound is off_t (2^63).
> + */
> + if (sizeof(unsigned long) < sizeof(long long))
> + max = 1024ull * ULONG_MAX;
> + else
> + max = LLONG_MAX;
> +
> + if (virXPathULongLong("string(./@memory)", ctxt, &bytes) < 0) {
> + virReportError(VIR_ERR_XML_DETAIL, "%s",
> + _("unable to parse memory size attribute"));
> + goto cleanup;
> + }
> +
> + unit = virXPathString("string(./@unit)", ctxt);
> +
> + if (virScaleInteger(&bytes, unit, 1024, max) < 0)
> + goto cleanup;
> +
> + *cellMemory = VIR_DIV_UP(bytes, 1024);
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(unit);
> + ctxt->node = oldnode;
> + return ret;
> +
> +}
> +
This is practically a copy of virDomainHugepagesParseXML(). In your
first version I called for making it more general so it can be reused. I
find this code copying contra-productive. This is something I had in my
mind:
https://www.redhat.com/archives/libvir-list/2014-November/msg00234.html
After that patch is merged, you can just drop this function and use
virDomainParseMemory() instead.
> virCPUDefPtr
> virCPUDefParseXML(xmlNodePtr node,
> xmlXPathContextPtr ctxt,
> @@ -440,7 +482,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 +531,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);
You're just ignoring return value of this function. After my patch is
merged you can just go with:
ctxt->node = nodes[i];
if (virDomainParseMemory("./@memory", "./@unit", ctxt,
&def->cells[cur_cell].mem, true,false) < 0)
goto error;
Of course, you'll need to:
1) export the virDomainParseMemory() function
2) keep the original ctxt->node somewhere, so it's the same after the
control returns from the function.
Otherwise looking good.
Michal
More information about the libvir-list
mailing list