Re: [PATCH] libxl: fix dom0 balloon logic

* Re: [PATCH] libxl: fix dom0 balloon logic
       [not found] <1426893040-11342-1-git-send-email-jfehlig@suse.com>
@ 2015-03-23 11:31 ` Ian Campbell
       [not found] ` <1427110272.21742.240.camel@citrix.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Ian Campbell @ 2015-03-23 11:31 UTC (permalink / raw)
  To: Jim Fehlig, Stefano Stabellini, Ian Jackson, Wei Liu
  Cc: libvir-list, mlatimer, xen-devel

(just ccing the other tools maintainers, in particular Stefano who knows
what this stuff is supposed to do...)

On Fri, 2015-03-20 at 17:10 -0600, Jim Fehlig wrote:
> Recent testing on large memory systems revealed a bug in the Xen xl
> tool's freemem() function.  When autoballooning is enabled, freemem()
> is used to ensure enough memory is available to start a domain,
> ballooning dom0 if necessary.  When ballooning large amounts of memory
> from dom0, freemem() would exceed its self-imposed wait time and
> return an error.  Meanwhile, dom0 continued to balloon.  Starting the
> domain later, after sufficient memory was ballooned from dom0, would
> succeed.  The libvirt implementation in libxlDomainFreeMem() suffers
> the same bug since it is modeled after freemem().
> 
> In the end, the best place to fix the bug on the Xen side was to
> slightly change the behavior of libxl_wait_for_memory_target().
> Instead of failing after caller-provided wait_sec, the function now
> blocks as long as dom0 memory ballooning is progressing.  It will return
> failure only when more memory is needed to reach the target and wait_sec
> have expired with no progress being made.  See xen.git commit fd3aa246.
> There was a dicussion on how this would affect other libxl apps like
> libvirt
> 
> http://lists.xen.org/archives/html/xen-devel/2015-03/msg00739.html
> 
> If libvirt containing this patch was build against a Xen containing
> the old libxl_wait_for_memory_target() behavior, libxlDomainFreeMem()
> will fail after 30 sec and domain creation will be terminated.
> Without this patch and with old libxl_wait_for_memory_target() behavior,
> libxlDomainFreeMem() does not succeed after 30 sec, but returns success
> anyway.  Domain creation continues resulting in all sorts of fun stuff
> like cpu soft lockups in the guest OS.  It was decided to properly fix
> libxl_wait_for_memory_target(), and if anything improve the default
> behavior of apps using the freemem reference impl in xl.
> 
> xl was patched to accommodate the change in libxl_wait_for_memory_target()
> with xen.git commit 883b30a0.  This patch does the same in the libxl
> driver.  While at it, I changed the logic to essentially match
> freemem() in $xensrc/tools/libxl/xl_cmdimpl.c.  It was a bit cleaner
> IMO and will make it easier to spot future, potentially interesting
> divergences.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_domain.c | 57 ++++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 407a9bd..ff78133 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1121,38 +1121,41 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, libxl_domain_config *d_config)
>  {
>      uint32_t needed_mem;
>      uint32_t free_mem;
> -    size_t i;
> -    int ret = -1;
> +    int ret;
>      int tries = 3;
>      int wait_secs = 10;
>  
> -    if ((ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
> -                                        &needed_mem)) >= 0) {
> -        for (i = 0; i < tries; ++i) {
> -            if ((ret = libxl_get_free_memory(priv->ctx, &free_mem)) < 0)
> -                break;
> +    ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
> +                                   &needed_mem);
> +    if (ret < 0)
> +        goto error;
>  
> -            if (free_mem >= needed_mem) {
> -                ret = 0;
> -                break;
> -            }
> +    do {
> +        ret = libxl_get_free_memory(priv->ctx, &free_mem);
> +        if (ret < 0)
> +            goto error;
>  
> -            if ((ret = libxl_set_memory_target(priv->ctx, 0,
> -                                               free_mem - needed_mem,
> -                                               /* relative */ 1, 0)) < 0)
> -                break;
> +        if (free_mem >= needed_mem)
> +            return 0;
>  
> -            ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
> -                                             wait_secs);
> -            if (ret == 0 || ret != ERROR_NOMEM)
> -                break;
> +        ret = libxl_set_memory_target(priv->ctx, 0,
> +                                      free_mem - needed_mem,
> +                                      /* relative */ 1, 0);
> +        if (ret < 0)
> +            goto error;
>  
> -            if ((ret = libxl_wait_for_memory_target(priv->ctx, 0, 1)) < 0)
> -                break;
> -        }
> -    }
> +        ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
> +                                         wait_secs);
> +        if (ret < 0)
> +            goto error;
>  
> -    return ret;
> +        tries--;
> +    } while (tries > 0);
> +
> + error:
> +    virReportSystemError(ret, "%s",
> +                         _("Failed to balloon domain0 memory"));
> +    return -1;
>  }
>  
>  static void
> @@ -1271,12 +1274,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>                                 priv->ctx, &d_config) < 0)
>          goto endjob;
>  
> -    if (cfg->autoballoon && libxlDomainFreeMem(priv, &d_config) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("libxenlight failed to get free memory for domain '%s'"),
> -                       d_config.c_info.name);
> +    if (cfg->autoballoon && libxlDomainFreeMem(priv, &d_config) < 0)
>          goto endjob;
> -    }
>  
>      if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
>                                         vm->def, VIR_HOSTDEV_SP_PCI) < 0)

^ permalink raw reply	[flat|nested] 4+ messages in thread