[libvirt] [PATCH 1/2] qemuBuildMemoryBackendStr: Fix hugepages lookup process

[Peter Krempa]

On Thu, Jun 25, 2015 at 18:13:02 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1196644
> 
> So this function is called to construct the guest NUMA part of
> the qemu command line. At the beginning, the configured hugepages

This function constructs the backend (host facing) part of the memory
device.

> are searched to find the best match for given guest NUMA node.
> Configured hugepages can have a @nodeset attribute to specify on
> which guest NUMA nodes should be the hugepages backing used.
> There is, however, one 'corner case'. Users may just tell 'use
> hugepages to back all the nodes'. In other words:
> 
>   <memoryBacking>
>     <hugepages/>
>   </memoryBacking>
> 
>   <cpu>
>     <numa>
>       <cell id='0' cpus='0-1' memory='1024000' unit='KiB'/>
>     </numa>
>   </cpu>
> 
> Our code fails in this case. Well, since there's no @nodeset (nor
> any <page/> child element to <hugepages/>) we fail to lookup the
> default hugepage size to use.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_command.c                            |  41 ++++----
>  .../qemuxml2argv-hugepages-numa.args               |  46 +++++++++
>  .../qemuxml2argv-hugepages-numa.xml                | 107 +++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |   8 ++
>  4 files changed, 183 insertions(+), 19 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 99755f1..cb31fac 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4760,9 +4760,6 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>              hugepage = master_hugepage;
>          }
>  
> -        if (hugepage)
> -            pagesize = hugepage->size;
> -

This hunk is a bit awkward here since you add it back in the next patch.
A rebase mistake perhaps?

>          if (hugepage && hugepage->size == system_page_size) {
>              /* However, if user specified to use "huge" page
>               * of regular system page size, it's as if they
> @@ -4778,23 +4775,29 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>              goto cleanup;
>          }
>  
> -        /* Now lets see, if the huge page we want to use is even mounted
> -         * and ready to use */
> -        for (i = 0; i < cfg->nhugetlbfs; i++) {
> -            if (cfg->hugetlbfs[i].size == hugepage->size)
> -                break;
> -        }
> -
> -        if (i == cfg->nhugetlbfs) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Unable to find any usable hugetlbfs mount for %llu KiB"),
> -                           pagesize);
> -            goto cleanup;
> -        }
> -
>          VIR_FREE(mem_path);

mem_path is NULL prior to this free statement, and it is not in a
loop so the VIR_FREE can be removed here.

> -        if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i])))
> -            goto cleanup;
> +        if (hugepage->size) {
> +            /* Now lets see, if the huge page we want to use is even mounted
> +             * and ready to use */
> +            for (i = 0; i < cfg->nhugetlbfs; i++) {
> +                if (cfg->hugetlbfs[i].size == hugepage->size)
> +                    break;
> +            }
> +
> +            if (i == cfg->nhugetlbfs) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Unable to find any usable hugetlbfs mount for %llu KiB"),
> +                               hugepage->size);
> +                goto cleanup;
> +            }
> +
> +            if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i])))
> +                goto cleanup;
> +        } else {
> +            if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs,
> +                                                    cfg->nhugetlbfs)))
> +                goto cleanup;
> +        }
>  
>          *backendType = "memory-backend-file";
>  

ACK,

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150626/3bc90dee/attachment-0001.sig>