[libvirt] [PATCH 2/2] qemuDomainAttachMemory: Crate hugepage dir if needed

[John Ferlan]

On 06/07/2017 11:50 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1455819
> 
> It may happen that a domain is started without any huge pages.
> However, user might try to attach a DIMM module later. DIMM
> backed by huge pages (why would somebody want to mix regular and
> huge pages is beyond me). Therefore we have to create the dir if
> we haven't done so far.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c |  3 +++
>  src/qemu/qemu_process.c | 20 ++++++++++++++++----
>  src/qemu/qemu_process.h |  5 +++++
>  3 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4a7d99725..62472aef8 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2254,6 +2254,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>                                    priv->qemuCaps, vm->def, mem, NULL, true) < 0)
>          goto cleanup;
>  
> +    if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
> +        goto cleanup;
> +

If this call was done after virDomainMemoryInsert, then the new check
[1] and parameter in qemuProcessBuildDestroyHugepagesPath wouldn't be
necessary, but the goto here would need to be removedef, true?

>      if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
>          goto cleanup;
>      teardowndevice = true;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a219a8080..823a1385f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3283,9 +3283,10 @@ qemuProcessReconnectCheckMemAliasOrderMismatch(virDomainObjPtr vm)
>  }
>  
>  
> -static int
> +int
>  qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
>                                       virDomainObjPtr vm,
> +                                     virDomainMemoryDefPtr mem,
>                                       bool build)
>  {
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> @@ -3314,6 +3315,12 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
>          }
>      }
>  
> +    if (mem &&
> +        mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
> +        mem->pagesize &&
> +        mem->pagesize != system_pagesize)
> +        shouldBuild = true;
> +

[1]

>      if (!build ||
>          (build && shouldBuild)) {
>          for (i = 0; i < cfg->nhugetlbfs; i++) {
> @@ -3324,6 +3331,11 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
>                  goto cleanup;
>  
>              if (build) {
> +                if (virFileExists(hugepagePath)) {
> +                    ret = 0;
> +                    goto cleanup;

I was initially wondering why this wasn't an "if (build &&
virFileExists()) continue;", then it dawned on me that this code will
create the path and label it for all hugetlbfs sizes if any one of those
sizes exists, so there's no need...

> +                }
> +
>                  if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
>                      virReportSystemError(errno,
>                                           _("Unable to create %s"),
> @@ -3497,7 +3509,7 @@ qemuProcessReconnect(void *opaque)
>          goto cleanup;
>      }
>  
> -    if (qemuProcessBuildDestroyHugepagesPath(driver, obj, true) < 0)
> +    if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
>          goto error;
>  
>      if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
> @@ -5541,7 +5553,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>                                 NULL) < 0)
>          goto cleanup;
>  
> -    if (qemuProcessBuildDestroyHugepagesPath(driver, vm, true) < 0)
> +    if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
>          goto cleanup;
>  
>      /* Ensure no historical cgroup for this VM is lying around bogus
> @@ -6225,7 +6237,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>          goto endjob;
>      }
>  
> -    qemuProcessBuildDestroyHugepagesPath(driver, vm, false);
> +    qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
>  
>      vm->def->id = -1;
>  
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 830d8cef8..b63784152 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -192,4 +192,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
>                              virDomainObjPtr vm,
>                              qemuDomainAsyncJob asyncJob);
>  
> +int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
> +                                         virDomainObjPtr vm,
> +                                         virDomainMemoryDefPtr mem,
> +                                         bool build);
> +

Perhaps a personal pet peeve of mine, but order wise in qemu_process.c
the API is between qemuProcessStopCPUs and qemuProcessIncomingDefNew, so
why not list it thusly in the .h file...

John

>  #endif /* __QEMU_PROCESS_H__ */
>