[libvirt] [PATCH v2 08/16] conf, schema: add support for memnode elements
Martin Kletzander
mkletzan at redhat.com
Tue Jul 15 09:58:21 UTC 2014
On Fri, Jul 11, 2014 at 05:10:53PM +0200, Michal Privoznik wrote:
>On 08.07.2014 13:50, Martin Kletzander wrote:
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>> docs/formatdomain.html.in | 15 ++
>> docs/schemas/domaincommon.rng | 17 ++
>> src/conf/numatune_conf.c | 183 +++++++++++++++++++--
>> .../qemuxml2argv-numatune-memnode-no-memory.xml | 30 ++++
>> .../qemuxml2argv-numatune-memnode-nocpu.xml | 25 +++
>> .../qemuxml2argv-numatune-memnode.xml | 31 ++++
>> .../qemuxml2argv-numatune-memnodes-problematic.xml | 31 ++++
>> tests/qemuxml2argvtest.c | 2 +
>> .../qemuxml2xmlout-numatune-memnode.xml | 33 ++++
>> tests/qemuxml2xmltest.c | 2 +
>> 10 files changed, 356 insertions(+), 13 deletions(-)
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.xml
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-nocpu.xml
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-memnodes-problematic.xml
>> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index ad87b7c..d845c1b 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -709,6 +709,8 @@
>> ...
>> <numatune>
>> <memory mode="strict" nodeset="1-4,^3"/>
>> + <memnode cellid="0" mode="strict" nodeset="1"/>
>> + <memnode cellid="2" mode="preferred" nodeset="2"/>
>> </numatune>
>> ...
>> </domain>
>> @@ -745,6 +747,19 @@
>>
>> <span class='since'>Since 0.9.3</span>
>> </dd>
>> + <dt><code>memnode</code></dt>
>> + <dd>
>> + Optional <code>memnode</code> elements can specify memory allocation
>> + policies per each guest NUMA node. For those nodes having no
>> + corresponding <code>memnode</code> element, the default from
>> + element <code>memory</code> will be used. Attribute <code>cellid</code>
>> + addresses guest NUMA node for which the settings are applied.
>> + Attributes <code>mode</code> and <code>nodeset</code> have the same
>> + meaning and syntax as in <code>memory</code> element.
>> +
>> + This setting is not compatible with automatic placement.
>> + <span class='since'>QEMU Since 1.2.6</span>
>
>1.2.8 actually
>
Are you suggesting I should wait yet another release or did you mean
1.2.7 by any chance? :)
>> + </dd>
>> </dl>
>>
>>
>
>>
>> +static int
>> +virDomainNumatuneNodeParseXML(virDomainDefPtr def,
>> + xmlXPathContextPtr ctxt)
>> +{
>> + int n = 0;;
>> + int ret = -1;
>> + size_t i = 0;
>> + xmlNodePtr *nodes = NULL;
>> +
>> + if ((n = virXPathNodeSet("./numatune/memnode", ctxt, &nodes)) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot extract memnode nodes"));
>> + goto cleanup;
>> + }
>> +
>> + if (!n)
>> + return 0;
>> +
>> + if (def->numatune && def->numatune->memory.specified &&
>> + def->numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Per-node binding is not compatible with "
>> + "automatic NUMA placement."));
>> + goto cleanup;
>> + }
>> +
>> + if (!def->cpu || !def->cpu->ncells) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Element 'memnode' is invalid without "
>> + "any guest NUMA cells"));
>> + goto cleanup;
>> + }
>> +
>> + if (!def->numatune && VIR_ALLOC(def->numatune) < 0)
>> + goto cleanup;
>
>Here you allow def->numatune to be allocated already.
>
>> +
>> + if (VIR_ALLOC_N(def->numatune->mem_nodes, def->cpu->ncells) < 0)
>> + goto cleanup;
>
>Which means, this can exists too. VIR_REALLOC_N() is safer IMO.
>
But that won't zero the memory. VIR_FREE(); VIR_ALLOC_N(); should
cover all cases.
>> +
>> + def->numatune->nmem_nodes = def->cpu->ncells;
>> +
>> + for (i = 0; i < n; i++) {
>> + const char *tmp = NULL;
>
>No. s/const//.
>
>> + int mode = 0;
>> + unsigned int cellid = 0;
>> + virDomainNumatuneNodePtr mem_node = NULL;
>> + xmlNodePtr cur_node = nodes[i];
>> +
>> + tmp = virXMLPropString(cur_node, "cellid");
>> + if (!tmp) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Missing required cellid attribute "
>> + "in memnode element"));
>> + goto cleanup;
>> + }
>> + if (virStrToLong_uip(tmp, NULL, 10, &cellid) < 0) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Invalid cellid attribute in memnode element"));
>
>Moreover, @tmp is leaked here. And it would be nice to tell users in the error message @tmp somehow.
>
You mean if the user has a typo in an integer? I guess that such user
has more issues that that typo then and needs more than that to make
his life easier. ;) Anyway, I'll add it.
[...]
>> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml
>> new file mode 100644
>> index 0000000..82b5f61
>> --- /dev/null
>> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml
>
>Are you sure this is the correct content?
>
Sure that is...
>> @@ -0,0 +1,33 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest</name>
>> + <uuid>9f4b6512-e73a-4a25-93e8-5307802821ce</uuid>
>> + <memory unit='KiB'>24682468</memory>
>
>In the qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml you have 65MB of RAM while here ~23.5GB.
>
... it's the qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml's
problem (I've screwed up a rebase, I guess), so I'll fix it there
instead so we have at least some weird topology in tests.
[...]
>> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
>> index 3bba565..4beb799 100644
>> --- a/tests/qemuxml2xmltest.c
>> +++ b/tests/qemuxml2xmltest.c
>> @@ -372,6 +372,8 @@ mymain(void)
>> DO_TEST_DIFFERENT("cpu-numa2");
>>
>> DO_TEST_DIFFERENT("numatune-auto-prefer");
>> + DO_TEST_DIFFERENT("numatune-memnode");
>> + DO_TEST("numatune-memnode-no-memory");
>>
>> virObjectUnref(driver.caps);
>> virObjectUnref(driver.xmlopt);
>>
>
>I can't ACK this one, until the issue is resolved. If I squash this in, the test passes again:
>
I suggest squashing this into [08/16]:
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index f1dddb5..1301639 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -758,7 +758,7 @@
meaning and syntax as in <code>memory</code> element.
This setting is not compatible with automatic placement.
- <span class='since'>QEMU Since 1.2.6</span>
+ <span class='since'>QEMU Since 1.2.7</span>
</dd>
</dl>
diff --git i/src/conf/numatune_conf.c w/src/conf/numatune_conf.c
index 4170be8..4f95229 100644
--- i/src/conf/numatune_conf.c
+++ w/src/conf/numatune_conf.c
@@ -67,6 +67,7 @@ static int
virDomainNumatuneNodeParseXML(virDomainDefPtr def,
xmlXPathContextPtr ctxt)
{
+ char *tmp = NULL;
int n = 0;;
int ret = -1;
size_t i = 0;
@@ -99,13 +100,13 @@ virDomainNumatuneNodeParseXML(virDomainDefPtr def,
if (!def->numatune && VIR_ALLOC(def->numatune) < 0)
goto cleanup;
+ VIR_FREE(def->numatune->mem_nodes);
if (VIR_ALLOC_N(def->numatune->mem_nodes, def->cpu->ncells) < 0)
goto cleanup;
def->numatune->nmem_nodes = def->cpu->ncells;
for (i = 0; i < n; i++) {
- const char *tmp = NULL;
int mode = 0;
unsigned int cellid = 0;
virDomainNumatuneNodePtr mem_node = NULL;
@@ -119,8 +120,9 @@ virDomainNumatuneNodeParseXML(virDomainDefPtr def,
goto cleanup;
}
if (virStrToLong_uip(tmp, NULL, 10, &cellid) < 0) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Invalid cellid attribute in memnode element"));
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid cellid attribute in memnode element: %s"),
+ tmp);
goto cleanup;
}
VIR_FREE(tmp);
@@ -170,6 +172,7 @@ virDomainNumatuneNodeParseXML(virDomainDefPtr def,
ret = 0;
cleanup:
VIR_FREE(nodes);
+ VIR_FREE(tmp);
return ret;
}
diff --git i/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml w/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml
index 18b00d8..49b328c 100644
--- i/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml
+++ w/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml
@@ -1,12 +1,13 @@
<domain type='qemu'>
<name>QEMUGuest</name>
<uuid>9f4b6512-e73a-4a25-93e8-5307802821ce</uuid>
- <memory unit='KiB'>65536</memory>
- <currentMemory unit='KiB'>65536</currentMemory>
- <vcpu placement='static'>2</vcpu>
+ <memory unit='KiB'>24682468</memory>
+ <currentMemory unit='KiB'>24682468</currentMemory>
+ <vcpu placement='static'>32</vcpu>
<numatune>
- <memory mode='strict' nodeset='0-3'/>
+ <memory mode='strict' nodeset='0-7'/>
<memnode cellid='0' mode='preferred' nodeset='3'/>
+ <memnode cellid='2' mode='strict' nodeset='1-2,5-7,^6'/>
</numatune>
<os>
<type arch='x86_64' machine='pc'>hvm</type>
@@ -14,8 +15,9 @@
</os>
<cpu>
<numa>
- <cell id='0' cpus='0' memory='32768'/>
- <cell id='1' cpus='1' memory='32768'/>
+ <cell id='0' cpus='0' memory='20002'/>
+ <cell id='1' cpus='1-27,29' memory='660066'/>
+ <cell id='2' cpus='28-31,^29' memory='24002400'/>
</numa>
</cpu>
<clock offset='utc'/>
--
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140715/04ef26b3/attachment-0001.sig>
More information about the libvir-list
mailing list