[libvirt] [PATCH 1/2] qemuProcessBuildDestroyHugepagesPath: create path more frequently

[John Ferlan]

On 06/07/2017 11:50 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1455819
> 
> Currently, the per-domain path for huge pages mmap() for qemu is
> created iff domain has memoryBacking and hugepages in it
> configured. However, this alone is not enough because there can
> be a DIMM module with hugepages configured too.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_process.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 32ba8e373..a219a8080 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3289,11 +3289,33 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
>                                       bool build)
>  {
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    const long system_pagesize = virGetSystemPageSizeKB();
>      char *hugepagePath = NULL;
>      size_t i;
> +    bool shouldBuild = false;
>      int ret = -1;
>  

None of the subsequent new checks would be necessary if build was
false...  Maybe it'd be better to have a helper function...

    if (build)
        shouldBuild = qemuProcessNeedHugepagesPath(...);

    if (shouldBuild || !build) {
    ...
    }

> -    if (vm->def->mem.nhugepages) {
> +    if (vm->def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE)
> +        shouldBuild = true;

Not part of the commit message, but that's an easy fix. Actually it
seems to me the algorithm prior to these changes is solely based on
whether hugepages were defined in the guest missing that there's
multiple ways to configure...

> +
> +    for (i = 0; !shouldBuild && i < vm->def->mem.nhugepages; i++) {
> +        if (vm->def->mem.hugepages[i].size != system_pagesize) {
> +            shouldBuild = true;
> +            break;

I would think because of the !shouldBuild as an end condition that the
break; wouldn't be necessary... Even more unnecessary if you have a
helper and it returns true right here.

FWIW: This particular check seems to be "additional" to the commit
message as well and is the much shorter check than found in other places
that use virGetSystemPageSizeKB to compare pagesize values.

> +        }
> +    }
> +
> +    for (i = 0; !shouldBuild &&  vm->def->nmems; i++) {
                                  ^^
Reduce one space...


> +        if (vm->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
> +            vm->def->mems[i]->pagesize &&
> +            vm->def->mems[i]->pagesize != system_pagesize) {
> +            shouldBuild = true;
> +            break;

ditto on break; and helper comment.

John

> +        }
> +    }
> +
> +    if (!build ||
> +        (build && shouldBuild)) {
>          for (i = 0; i < cfg->nhugetlbfs; i++) {
>              VIR_FREE(hugepagePath);
>              hugepagePath = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]);
>