[libvirt] [PATCH v2 02/14] libxl: populate xenstore memory entries at startup, handle dom0_mem

[Jim Fehlig]

Marek Marczykowski-Górecki wrote:
> libxl uses some xenstore entries for hints in memory management
> (especially when starting new domain). This includes dom0 memory limit
> and Xen free memory margin, based on current system state. Entries are
> created at first function usage, so force such call at daemon startup,
> which most likely will be before any domain startup.
> Also prevent automatic memory management if dom0_mem= option passed to
> xen hypervisor - it is known to be incompatible with autoballoon.
>
> Changes in v2:
>  - disable autoballoon when dom0_mem option detected
>  - rebase on 1.0.6+
>   

As mentioned previously, patch history should not be in the commit message.

> Signed-off-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
> ---
>  src/libxl/libxl_conf.h   |  4 ++++
>  src/libxl/libxl_driver.c | 36 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 44ecd41..e8fb9c4 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -62,6 +62,10 @@ struct _libxlDriverPrivate {
>  
>      virPortAllocatorPtr reservedVNCPorts;
>  
> +    /* should libxl/libvirt take care of getting memory for new domain from
> +     * dom0? */
>   

This comment sounds as though we are not quite sure about autoballoon,
like we need more info to handle autoballoon properly.  Better to just
state its purpose IMO, e.g.

Controls automatic ballooning of domain0.  If true, get memory for new
domains from domain0.

> +    bool autoballoon;
> +
>      size_t nactive;
>      virStateInhibitCallback inhibitCallback;
>      void *inhibitOpaque;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 3990354..0a0690d 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -29,6 +29,7 @@
>  #include <math.h>
>  #include <libxl.h>
>  #include <fcntl.h>
> +#include <regex.h>
>  
>  #include "internal.h"
>  #include "virlog.h"
> @@ -963,7 +964,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>      if (libxlBuildDomainConfig(driver, vm->def, &d_config) < 0)
>          goto error;
>  
> -    if (libxlFreeMem(priv, &d_config) < 0) {
> +    if (driver->autoballoon && libxlFreeMem(priv, &d_config) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("libxenlight failed to get free memory for domain '%s'"),
>                         d_config.c_info.name);
> @@ -1145,6 +1146,28 @@ libxlStateCleanup(void)
>      return 0;
>  }
>  
> +static int auto_autoballoon(libxlDriverPrivatePtr driver)
>   

Please use the pattern I mentioned in patch 1 when declaring/defining
functions.  Also, this function should return a bool, and use a more
libvirt libxl-ish name, e.g. libxlGetAutoballoon :).

> +{
> +    const libxl_version_info *info;
> +    regex_t regex;
> +    int ret;
> +
> +    info = libxl_get_version_info(driver->ctx);
> +    if (!info)
> +        return 1; /* default to on */
> +
> +    ret = regcomp(&regex,
> +            "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )",
> +            REG_NOSUB | REG_EXTENDED);
> +    if (ret)
> +        return 1;
> +
> +    ret = regexec(&regex, info->commandline, 0, NULL, 0);
> +    regfree(&regex);
> +    return ret == REG_NOMATCH;
> +}
> +
> +
>  static int
>  libxlStateInitialize(bool privileged,
>                       virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> @@ -1154,6 +1177,7 @@ libxlStateInitialize(bool privileged,
>      char *log_file = NULL;
>      virCommandPtr cmd;
>      int status, ret = 0;
> +    unsigned int free_mem;
>      char ebuf[1024];
>  
>      /* Disable libxl driver if non-root */
> @@ -1278,6 +1302,16 @@ libxlStateInitialize(bool privileged,
>                                                         NULL)))
>          goto error;
>  
> +    /* This will fill xenstore info about free and dom0 memory if missing,
> +     * should be called before starting first domain */
> +    if (libxl_get_free_memory(libxl_driver->ctx, &free_mem)) {
> +        VIR_ERROR(_("cannot get free memory info"));
>   

I think the error message could be improved since the driver won't load
if libxl_get_free_memory fails.  Something like "Unable to configure
libxl's memory management parameters"?

Regards,
Jim

> +        goto error;
> +    }
> +
> +    /* setup autoballoon */
> +    libxl_driver->autoballoon = auto_autoballoon(libxl_driver);
> +
>      /* Load running domains first. */
>      if (virDomainObjListLoadAllConfigs(libxl_driver->domains,
>                                         libxl_driver->stateDir,
>