On 29.09.2016 10:56, Jaroslav Safka wrote:
> This first change introduces xml parsing support for preallocated
> shared file descriptor based memory backing.
> It allows vhost-user to be used without hugepages.
>
> New xml elements:
> <memoryBacking>
> <source type='file|anonymous' path='/path/to/qemu/' />
> <access Mode='shared|private'/>
> <allocation mode='immediate|ondemand'/>
> </memoryBacking>
Okay, this is definitely interesting approach (not only because while
reviewing this, I've found an old branch in my git where I've started to
work on this).
Frankly, I don't know if this is a good API or not. Historically, we
required Dan's ACK on XML schema :-)
btw: s/Mode/mode/ ;-)
> ---
> docs/schemas/domaincommon.rng | 37 +++++
> src/conf/domain_conf.c | 149 ++++++++++++++++-----
> src/conf/domain_conf.h | 34 +++++
> .../qemuxml2argv-memorybacking-set.xml | 32 +++++
> .../qemuxml2argv-memorybacking-unset.xml | 32 +++++
> .../qemuxml2xmlout-memorybacking-set.xml | 40 ++++++
> .../qemuxml2xmlout-memorybacking-unset.xml | 40 ++++++
> tests/qemuxml2xmltest.c | 3 +
> 8 files changed, 334 insertions(+), 33 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml
You need to update the docs too. formatdomain.html.in to be more precise.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a06da46..7f73569 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16179,48 +16194,101 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
> VIR_FREE(tmp);
>
> - if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot extract hugepages nodes"));
> - goto error;
> + tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt);
> + if (tmp) {
> + if ((def->mem.source = virDomainMemorySourceTypeFromString(tmp)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown memoryBacking/source/type '%s'"), tmp);
> + goto error;
> + }
> + VIR_FREE(tmp);
> + if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> + tmp = virXPathString("string(./memoryBacking/source/@path)", ctxt);
> + if (!tmp && VIR_STRDUP(tmp, VIR_DOMAIN_MEMORY_DEFAULT_PATH) < 0)
> + goto error;
> +
> + def->mem.mem_path = tmp;
> + tmp = NULL;
> + }
> }
>
> - if (n) {
> - if (VIR_ALLOC_N(def->mem.hugepages, n) < 0)
> + tmp = virXPathString("string(./memoryBacking/access/@mode)", ctxt);
> + if (tmp) {
> + if ((def->mem.access = virDomainMemoryAccessTypeFromString(tmp)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown memoryBacking/access/mode '%s'"), tmp);
> goto error;
> + }
> + VIR_FREE(tmp);
> + }
>
> - for (i = 0; i < n; i++) {
> - if (virDomainHugepagesParseXML(nodes[i], ctxt,
> - &def->mem.hugepages[i]) < 0)
> + tmp = virXPathString("string(./memoryBacking/allocation/@mode)", ctxt);
> + if (tmp) {
> + if ((def->mem.allocation = virDomainMemoryAllocationTypeFromString(tmp)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown memoryBacking/allocation/mode '%s'"), tmp);
> + goto error;
> + }
> + VIR_FREE(tmp);
> + }
> +
> + if (virXPathNode("./memoryBacking/hugepages", ctxt)) {
> + /* hugepages will be used */
> +
> + if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("hugepages are not allowed with memory allocation ondemand"));
> + goto error;
> + }
> +
> + if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("hugepages are not allowed with anonymous memory source"));
> + goto error;
> + }
> +
> + if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot extract hugepages nodes"));
> + goto error;
> + }
These types of checks would normally go to post parse callback.
> +
> + if (n) {
> + if (VIR_ALLOC_N(def->mem.hugepages, n) < 0)
> goto error;
> - def->mem.nhugepages++;
>
> - for (j = 0; j < i; j++) {
> - if (def->mem.hugepages[i].nodemask &&
> - def->mem.hugepages[j].nodemask &&
> - virBitmapOverlaps(def->mem.hugepages[i].nodemask,
> - def->mem.hugepages[j].nodemask)) {
> - virReportError(VIR_ERR_XML_DETAIL,
> - _("nodeset attribute of hugepages "
> - "of sizes %llu and %llu intersect"),
> - def->mem.hugepages[i].size,
> - def->mem.hugepages[j].size);
> - goto error;
> - } else if (!def->mem.hugepages[i].nodemask &&
> - !def->mem.hugepages[j].nodemask) {
> - virReportError(VIR_ERR_XML_DETAIL,
> - _("two master hugepages detected: "
> - "%llu and %llu"),
> - def->mem.hugepages[i].size,
> - def->mem.hugepages[j].size);
> + for (i = 0; i < n; i++) {
> + if (virDomainHugepagesParseXML(nodes[i], ctxt,
> + &def->mem.hugepages[i]) < 0)
> goto error;
> + def->mem.nhugepages++;
> +
> + for (j = 0; j < i; j++) {
> + if (def->mem.hugepages[i].nodemask &&
> + def->mem.hugepages[j].nodemask &&
> + virBitmapOverlaps(def->mem.hugepages[i].nodemask,
> + def->mem.hugepages[j].nodemask)) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("nodeset attribute of hugepages "
> + "of sizes %llu and %llu intersect"),
> + def->mem.hugepages[i].size,
> + def->mem.hugepages[j].size);
> + goto error;
> + } else if (!def->mem.hugepages[i].nodemask &&
> + !def->mem.hugepages[j].nodemask) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("two master hugepages detected: "
> + "%llu and %llu"),
> + def->mem.hugepages[i].size,
> + def->mem.hugepages[j].size);
> + goto error;
> + }
> }
> }
> - }
>
> - VIR_FREE(nodes);
> - } else {
> - if ((node = virXPathNode("./memoryBacking/hugepages", ctxt))) {
> + VIR_FREE(nodes);
> + } else {
> + /* no hugepage pages */
> if (VIR_ALLOC(def->mem.hugepages) < 0)
> goto error;
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index d2065cf..f1290a3 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3073,6 +3104,9 @@ VIR_ENUM_DECL(virDomainTPMModel)
> VIR_ENUM_DECL(virDomainTPMBackend)
> VIR_ENUM_DECL(virDomainMemoryModel)
> VIR_ENUM_DECL(virDomainMemoryBackingModel)
> +VIR_ENUM_DECL(virDomainMemorySource)
> +VIR_ENUM_DECL(virDomainMemoryAccess)
> +VIR_ENUM_DECL(virDomainMemoryAllocation)
These macros declare vir*TypeFromString() and vir*TypeToString()
functions. Because they are in the header file, we should update the
libvirt_private.syms too (even though these functions are not used
anywere else just yet).
The rest of the code looks okay (even in 2/2).
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list