[libvirt] [PATCH V2 2/2] libxl: fix dom0 maximum memory setting

[Joao Martins]

On 02/08/2017 10:24 PM, Jim Fehlig wrote:
> When the libxl driver is initialized, it creates a virDomainDef
> object for dom0 and adds it to the list of domains. Total memory
> for dom0 was being set from the max_memkb field of libxl_dominfo
> struct retrieved from libxl, but this field can be set to
> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
> explicitly set by the user.
> 
> This patch adds some simple parsing of the Xen commandline,
> looking for a dom0_mem parameter that also specifies a 'max' value.
> If not specified, dom0 maximum memory is effectively all physical
> host memory.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>

Argh, sorry. Failed to mention one (non-blocking) observation in the patch about
the parsing of "dom0_mem". Regardless,

Acked-by: Joao Martins <joao.m.martins at oracle.com>

> ---
> 
> V2: Check return value of libxl_get_physinfo and
> libxlDriverGetDom0MaxmemConf.
> 
>  src/libxl/libxl_conf.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_conf.h   |  4 +++
>  src/libxl/libxl_driver.c |  5 +++-
>  3 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 28b05e1..7e30c7d 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -34,6 +34,7 @@
>  #include "internal.h"
>  #include "virlog.h"
>  #include "virerror.h"
> +#include "c-ctype.h"
>  #include "datatypes.h"
>  #include "virconf.h"
>  #include "virfile.h"
> @@ -1533,6 +1534,82 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>  
>  }
>  
> +/*
> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
> + * memory to 8G: dom0_mem=4G,max:8G
> + *
> + * If not constrained by the user, dom0 can effectively use all host memory.
> + * This function returns the configured maximum memory for dom0 in kilobytes,
> + * either the user-specified value or total physical memory as a default.
> + */
> +int
> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
> +                             unsigned long long *maxmem)
> +{
> +    char **cmd_tokens = NULL;
> +    char **mem_tokens = NULL;
> +    size_t i;
> +    size_t j;
> +    libxl_physinfo physinfo;
> +    int ret = -1;
> +
> +    if (cfg->verInfo->commandline == NULL ||
> +        !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
> +        goto physmem;
> +
> +    for (i = 0; cmd_tokens[i] != NULL; i++) {
> +        if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
> +            continue;
> +
> +        if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
> +            break;
> +        for (j = 0; mem_tokens[j] != NULL; j++) {
> +            if (STRPREFIX(mem_tokens[j], "max:")) {
> +                char *p = mem_tokens[j] + 4;
> +                unsigned long long multiplier = 1;
Hmm, perhaps worth a comment here (or in the command at the top of the function)
because if the parameter doesn't have a suffix it defaults to kilobytes (as per
Xen command line docs).

> +
> +                while (c_isdigit(*p))
> +                    p++;
> +                if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, maxmem) < 0)
> +                    break;
> +                if (*p) {
> +                    switch (*p) {
> +                    case 'm':
> +                    case 'M':
> +                        multiplier = 1024;
> +                        break;
> +                    case 'g':
> +                    case 'G':
> +                        multiplier = 1024 * 1024;
> +                        break;
> +                    }
> +                }
> +                *maxmem = *maxmem * multiplier;
> +                ret = 0;
> +                goto cleanup;
This seems to match xen's parsing of the parameter that is for "max=4GG" or
"max=4GM" the latter suffix will be ignored (thus considering 4G only). I was
looking at the Xen's code of dom0_mem parsing and it's documentation - and
noticed that we don't support Terabytes in the parsing above. Though this is
only since Xen 4.5.

> +            }
> +        }
> +    }
> +
> + physmem:
> +    /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
> +    libxl_physinfo_init(&physinfo);
> +    if (libxl_get_physinfo(cfg->ctx, &physinfo)) {
> +        VIR_WARN("libxl_get_physinfo failed");
> +        goto cleanup;
> +    }
> +    *maxmem = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
> +    libxl_physinfo_dispose(&physinfo);
> +    ret = 0;
> +
> + cleanup:
> +    virStringListFree(cmd_tokens);
> +    virStringListFree(mem_tokens);
> +    return ret;
> +}
> +
> +
>  #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>  static int
>  libxlPrepareChannel(virDomainChrDefPtr channel,
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 69d7885..4942171 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -174,6 +174,10 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>                                const char *filename);
>  
>  int
> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
> +                             unsigned long long *maxmem);
> +
> +int
>  libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>  int
>  libxlMakeNic(virDomainDefPtr def,
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 040b986..1558d5b 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -576,6 +576,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>      virDomainObjPtr vm = NULL;
>      virDomainDefPtr oldDef = NULL;
>      libxl_dominfo d_info;
> +    unsigned long long maxmem;
>      int ret = -1;
>  
>      libxl_dominfo_init(&d_info);
> @@ -615,7 +616,9 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>      if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
>          goto cleanup;
>      vm->def->mem.cur_balloon = d_info.current_memkb;
> -    virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
> +    if (libxlDriverGetDom0MaxmemConf(cfg, &maxmem) < 0)
> +        maxmem = d_info.current_memkb;
> +    virDomainDefSetMemoryTotal(vm->def, maxmem);
>  
>      ret = 0;
>  
>