[libvirt] [PATCH 3/5] qemu: prefer memfd for anonymous memory

[John Ferlan]

On 09/07/2018 07:32 AM, marcandre.lureau at redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> 

Would be nice to have a few more words here. If you provide them I can
add them... The if statement is difficult to read unless you know what
each field really means.

secondary question - should we document what gets used?, e.g.:

https://libvirt.org/formatdomain.html#elementsMemoryBacking

Seems to me the preference to use memfd is for memory backing using
anonymous source for nvdimm's without a defined path, but sometimes my
wording doesn't match reality.

> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>  src/qemu/qemu_command.c | 61 +++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c9e3a91e32..97cfc8a18d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3113,6 +3113,24 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
>      return ret;
>  }
>  
> +static int
> +qemuBuildMemoryBackendPropsShare(virJSONValuePtr props,
> +                                 virDomainMemoryAccess memAccess)
> +{
> +    switch (memAccess) {
> +    case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
> +        return virJSONValueObjectAdd(props, "b:share", true, NULL);
> +
> +    case VIR_DOMAIN_MEMORY_ACCESS_PRIVATE:
> +        return virJSONValueObjectAdd(props, "b:share", false, NULL);
> +
> +    case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT:
> +    case VIR_DOMAIN_MEMORY_ACCESS_LAST:
> +        break;
> +    }
> +
> +    return 0;
> +}
>  
>  /**
>   * qemuBuildMemoryBackendProps:

The comments should have been updated... In particular:

  "Then, if one of the two memory-backend-* should be used..."

> @@ -3259,7 +3277,23 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>      if (!(props = virJSONValueNewObject()))
>          return -1;
>  
> -    if (useHugepage || mem->nvdimmPath || memAccess ||

Is this preference over "-ram" or "-file"?  It would seem to me someone
choosing "file" has a specific case and this is more for those other
options where if capabilities exist, then we try to use them.

> +    if (!mem->nvdimmPath &&
> +        def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS &&
> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD) &&
> +        (!useHugepage || virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD_HUGETLB))) {
> +        backendType = "memory-backend-memfd";
> +
> +        if (useHugepage &&
> +            (virJSONValueObjectAdd(props, "b:hugetlb", useHugepage, NULL) < 0 ||
> +             virJSONValueObjectAdd(props, "U:hugetlbsize", pagesize << 10, NULL) < 0)) {
> +            goto cleanup;
> +        }
> +
> +        if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) {
> +            goto cleanup;
> +        }

Running syntax-check would fail because of the { }

> +
> +    } else if (useHugepage || mem->nvdimmPath || memAccess ||
>          def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
>  
>          if (mem->nvdimmPath) {
> @@ -3297,20 +3331,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>                  goto cleanup;
>          }
>  
> -        switch (memAccess) {
> -        case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
> -            if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
> -                goto cleanup;
> -            break;
> -
> -        case VIR_DOMAIN_MEMORY_ACCESS_PRIVATE:
> -            if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0)
> -                goto cleanup;
> -            break;
> -
> -        case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT:
> -        case VIR_DOMAIN_MEMORY_ACCESS_LAST:
> -            break;
> +        if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) {
> +            goto cleanup;
>          }

Running syntax-check would fail here because of the { }

All this is fix-able without you needing to post another series, but I
need you to provide the verbiage for the intro and perhaps something
that could be added to the web page. I can adjust the patch accordingly.

Assuming of course Michal doesn't have other reservations...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

>      } else {
>          backendType = "memory-backend-ram";
> @@ -7609,7 +7631,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>  
>      if (virDomainNumatuneHasPerNodeBinding(def->numa) &&
>          !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
> -          virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE))) {
> +          virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE) ||
> +          virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD))) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("Per-node memory binding is not supported "
>                           "with this QEMU"));
> @@ -7618,7 +7641,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>  
>      if (def->mem.nhugepages &&
>          def->mem.hugepages[0].size != system_page_size &&
> -        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) {
> +        !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE) ||
> +          virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD_HUGETLB))) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("huge pages per NUMA node are not "
>                           "supported with this QEMU"));
> @@ -7635,7 +7659,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>       * need to check which approach to use */
>      for (i = 0; i < ncells; i++) {
>          if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
> -            virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) {
> +            virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE) ||
> +            virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD)) {
>  
>              if ((rc = qemuBuildMemoryCellBackendStr(def, cfg, i, priv,
>                                                      &nodeBackends[i])) < 0)
>