Re: [PATCH 1/2] conf: Add support for preallocated fd memory

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

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]