Re: [PATCH 1/1] qemu: host NUMA hugepage policy without guest NUMA

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

 



On Wed, Oct 12, 2016 at 15:04:53 +1100, Sam Bobroff wrote:
> At the moment, guests that are backed by hugepages in the host are
> only able to use policy to control the placement of those hugepages
> on a per-(guest-)CPU basis. Policy applied globally is ignored.
> 
> Such guests would use <memoryBacking><hugepages/></memoryBacking> and
> a <numatune> block with <memory mode=... nodeset=.../> but no <memnode
> .../> elements.
> 
> This patch corrects this by, in this specific case, changing the QEMU
> command line from "-mem-prealloc -mem-path=..." (which cannot
> specify NUMA policy) to "-object memory-backend-file ..." (which can).
> 
> Note: This is not visible to the guest and does not appear to create
> a migration incompatibility.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@xxxxxxxxxxx>
> ---
> There was some discussion leading up to this patch, here:
> 
> https://www.redhat.com/archives/libvir-list/2016-October/msg00033.html
> 
> During that discussion it seemed that fixing this issue would cause
> migration incompatibility but after some careful testing, it appears
> that it only does when a memory-backend object is attached to a guest
> NUMA node and that is not the case here. If only one is created, and
> used globally (not attached via mem=<id>), the migration data does not
> seem to be changed and so it seems reasonable to try something like
> this patch.
> 
> This patch does work for my test cases but I don't claim a deep
> understanding of the libvirt code so this is at least partly a RFC.
> Comments welcome :-)
> 
> Cheers,
> Sam.
> 
>  src/qemu/qemu_command.c | 55 ++++++++++++++++++++++++++++++++++++++++---------
>  src/qemu/qemu_command.h |  2 +-
>  2 files changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0804133..c28c8f2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3143,7 +3143,7 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>                            int guestNode,
>                            virBitmapPtr userNodeset,
>                            virBitmapPtr autoNodeset,
> -                          virDomainDefPtr def,
> +                          const virDomainDefPtr def,

This does not work as expected. Using 'const' with types that hide the
pointer makes the pointer const and not the structure that the pointer
points to.

>                            virQEMUCapsPtr qemuCaps,
>                            virQEMUDriverConfigPtr cfg,
>                            const char **backendType,
> @@ -7129,12 +7129,18 @@ qemuBuildSmpCommandLine(virCommandPtr cmd,
>  
>  static int
>  qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
> -                    const virDomainDef *def,
> +                    const virDomainDefPtr def,

Same as above. This change is wrong.

>                      virQEMUCapsPtr qemuCaps,
> -                    virCommandPtr cmd)
> +                    virCommandPtr cmd,
> +                    virBitmapPtr auto_nodeset)
>  {
>      const long system_page_size = virGetSystemPageSizeKB();
>      char *mem_path = NULL;
> +    virBitmapPtr nodemask = NULL;
> +    const char *backendType = NULL;
> +    char *backendStr = NULL;
> +    virJSONValuePtr props;
> +    int rv = -1;
>  
>      /*
>       *  No-op if hugepages were not requested.
> @@ -7159,18 +7165,47 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
>      if (qemuGetHupageMemPath(cfg, def->mem.hugepages[0].size, &mem_path) < 0)
>          return -1;
>  
> -    virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL);
> +    if (virDomainNumatuneMaybeGetNodeset(def->numa, auto_nodeset,
> +                                         &nodemask, -1) < 0)
> +        return -1;
> +    if (nodemask && virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) {
> +        props = virJSONValueNewObject();

This is leaked, qemuBuildMemoryBackendStr allocates one itself.

> +        if (qemuBuildMemoryBackendStr(virDomainDefGetMemoryInitial(def),
> +                                      0, -1, NULL, auto_nodeset,
> +                                      def, qemuCaps, cfg, &backendType,
> +                                      &props, false) < 0)
> +            goto cleanup;
> +        if (!(backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType,
> +                                                                 "mem",
> +                                                                 props)))
> +            goto cleanup;

So you create a memory object that does not get used ...

> +        virCommandAddArgList(cmd, "-object", backendStr, NULL);

So does qemu use memory objects that are not used magically as it's
memory? That seems odd. Could you please elaborate on this part and how
it's supposed to work? Without a proper elaboration how this is supposed
to work with qemu this patch is not acceptable.

> +        rv = 0;
> +cleanup:

This won't pass syntax check. Labels have to be spaced out.

> +        VIR_FREE(backendStr);
> +        VIR_FREE(props);

This is a JSON value object, you need to free it with the proper
function.

> +    }
> +    else {

This breaks coding style.

> +        if (nodemask || 1)

Erm, a tautology?

> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Memory file backend objects are "
> +                             "unsupported by QEMU binary. Global NUMA "
> +                             "hugepage policy will be ignored."));

Reporting an error but not terminating the startup is not acceptable.
Errors should not be reported unless something fails.

> +        virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL);
> +        rv = 0;
> +    }
>      VIR_FREE(mem_path);
>  
> -    return 0;
> +    return rv;
>  }
>  

Missing a change to the test files. Please make sure that you run 'make
check' and 'make syntax-check' and send only patches that pass those.

Peter

Attachment: signature.asc
Description: Digital signature

--
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]