[PATCH] libxl: Wait for ballooning if free memory is increasing

[PATCH] libxl: Wait for ballooning if free memory is increasing

[Mike Latimer]

During domain startup, all required memory ballooning must complete
within a maximum window of 33 seconds (3 retries, 11 seconds of delay).
If not, domain creation is aborted with a 'failed to free memory' error.

In order to accommodate large domains or slower hardware (which require
substantially longer to balloon memory) the free memory process should
retry as long as the amount of free memory is increasing on each
iteration of the loop.

---
 tools/libxl/xl_cmdimpl.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0b02a6c..f87efc0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2194,8 +2194,9 @@ static int preserve_domain(uint32_t *r_domid, libxl_event *event,
 
 static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
 {
-    int rc, retries = 3;
-    uint32_t need_memkb, free_memkb;
+    int rc, retries;
+    const int MAX_RETRIES = 3;
+    uint32_t need_memkb, free_memkb, free_memkb_prev = 0;
 
     if (!autoballoon)
         return 0;
@@ -2204,6 +2205,7 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
     if (rc < 0)
         return rc;
 
+    retries = MAX_RETRIES;
     do {
         rc = libxl_get_free_memory(ctx, &free_memkb);
         if (rc < 0)
@@ -2228,7 +2230,13 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
         if (rc < 0)
             return rc;
 
-        retries--;
+        /* only decrement retry count if free_memkb is not increasing */
+        if (free_memkb <= free_memkb_prev) {
+            retries--;
+        } else {
+            retries = MAX_RETRIES;
+            free_memkb_prev = free_memkb;
+        }
     } while (retries > 0);
 
     return ERROR_NOMEM;
-- 
1.8.4.5

Re: [PATCH] libxl: Wait for ballooning if free memory is increasing

[Mike Latimer]

On Wednesday, January 21, 2015 10:22:53 PM Mike Latimer wrote:
> During domain startup, all required memory ballooning must complete
> within a maximum window of 33 seconds (3 retries, 11 seconds of delay).
> If not, domain creation is aborted with a 'failed to free memory' error.
> 
> In order to accommodate large domains or slower hardware (which require
> substantially longer to balloon memory) the free memory process should
> retry as long as the amount of free memory is increasing on each
> iteration of the loop.
> 
> ---
>  tools/libxl/xl_cmdimpl.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 0b02a6c..f87efc0 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2194,8 +2194,9 @@ static int preserve_domain(uint32_t *r_domid,
> libxl_event *event,
> 
>  static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>  {
> -    int rc, retries = 3;
> -    uint32_t need_memkb, free_memkb;
> +    int rc, retries;
> +    const int MAX_RETRIES = 3;
> +    uint32_t need_memkb, free_memkb, free_memkb_prev = 0;
> 
>      if (!autoballoon)
>          return 0;
> @@ -2204,6 +2205,7 @@ static int freemem(uint32_t domid,
> libxl_domain_build_info *b_info) if (rc < 0)
>          return rc;
> 
> +    retries = MAX_RETRIES;
>      do {
>          rc = libxl_get_free_memory(ctx, &free_memkb);
>          if (rc < 0)
> @@ -2228,7 +2230,13 @@ static int freemem(uint32_t domid,
> libxl_domain_build_info *b_info) if (rc < 0)
>              return rc;
> 
> -        retries--;
> +        /* only decrement retry count if free_memkb is not increasing */
> +        if (free_memkb <= free_memkb_prev) {
> +            retries--;
> +        } else {
> +            retries = MAX_RETRIES;
> +            free_memkb_prev = free_memkb;
> +        }
>      } while (retries > 0);
> 
>      return ERROR_NOMEM;

Ping?

Re: [PATCH] libxl: Wait for ballooning if free memory is increasing

[Wei Liu]

You forgot go CC relevant maintainers. Now I've done that for you.

I think last time Ian and George were following.

On Wed, Jan 21, 2015 at 10:22:53PM -0700, Mike Latimer wrote:
> During domain startup, all required memory ballooning must complete
> within a maximum window of 33 seconds (3 retries, 11 seconds of delay).
> If not, domain creation is aborted with a 'failed to free memory' error.
> 
> In order to accommodate large domains or slower hardware (which require
> substantially longer to balloon memory) the free memory process should
> retry as long as the amount of free memory is increasing on each
> iteration of the loop.
> 
> ---
>  tools/libxl/xl_cmdimpl.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 0b02a6c..f87efc0 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2194,8 +2194,9 @@ static int preserve_domain(uint32_t *r_domid, libxl_event *event,
>  
>  static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>  {
> -    int rc, retries = 3;
> -    uint32_t need_memkb, free_memkb;
> +    int rc, retries;
> +    const int MAX_RETRIES = 3;
> +    uint32_t need_memkb, free_memkb, free_memkb_prev = 0;
>  
>      if (!autoballoon)
>          return 0;
> @@ -2204,6 +2205,7 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>      if (rc < 0)
>          return rc;
>  
> +    retries = MAX_RETRIES;
>      do {
>          rc = libxl_get_free_memory(ctx, &free_memkb);
>          if (rc < 0)
> @@ -2228,7 +2230,13 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>          if (rc < 0)
>              return rc;
>  
> -        retries--;
> +        /* only decrement retry count if free_memkb is not increasing */
> +        if (free_memkb <= free_memkb_prev) {
> +            retries--;
> +        } else {
> +            retries = MAX_RETRIES;
> +            free_memkb_prev = free_memkb;
> +        }
>      } while (retries > 0);
>  
>      return ERROR_NOMEM;
> -- 
> 1.8.4.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] libxl: Wait for ballooning if free memory is increasing

[Ian Campbell]

On Wed, 2015-01-21 at 22:22 -0700, Mike Latimer wrote:

Sorry for the delay.

> @@ -2228,7 +2230,13 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>          if (rc < 0)
>              return rc;
>  
> -        retries--;
> +        /* only decrement retry count if free_memkb is not increasing */

This isn't quite true -- you also reset the retry count if progress has
been made.

> +        if (free_memkb <= free_memkb_prev) {
> +            retries--;

I think you need to update prev here, otherwise after one successful
iteration the condition is always true even if progress subsequently
stalls.

> +        } else {
> +            retries = MAX_RETRIES;
> +            free_memkb_prev = free_memkb;

... iow the second assignment here should be after the if/else entirely.

Given that this new loop can take significantly longer to fail I wonder
if we should add some progress logging? xl has an xtl logger instance
available so using xtl_progress might be an easy option. Maybe a
separate patch though.

Ian.

Re: [PATCH] libxl: Wait for ballooning if free memory is increasing

[Mike Latimer]

On Wednesday, January 28, 2015 01:05:25 PM Ian Campbell wrote:
> On Wed, 2015-01-21 at 22:22 -0700, Mike Latimer wrote:
> 
> Sorry for the delay.

No problem! Thanks for the comments.

> > @@ -2228,7 +2230,13 @@ static int freemem(uint32_t domid,
> > libxl_domain_build_info *b_info)> 
> >          if (rc < 0)
> >          
> >              return rc;
> > 
> > -        retries--;
> > +        /* only decrement retry count if free_memkb is not increasing */
> 
> This isn't quite true -- you also reset the retry count if progress has
> been made.

Resetting the counter shouldn't decrement it, but I see your point. ;)

> > +        if (free_memkb <= free_memkb_prev) {
> > +            retries--;
> 
> I think you need to update prev here, otherwise after one successful
> iteration the condition is always true even if progress subsequently
> stalls.

If progress stalls, the above statement needs to be true in order to decrement 
the retry count. The test is comparing free_memkb as set at the beginning of 
the loop though, so it is not completely accurate. The next iteration of the 
loop resets it, so progress should be caught (unless I'm missing something).

> > +        } else {
> > +            retries = MAX_RETRIES;
> > +            free_memkb_prev = free_memkb;
> 
> ... iow the second assignment here should be after the if/else entirely.

If there is a chance that free_memkb could drop lower between iterations, I 
wanted free_memkb_prev to act as a watermark and only be updated once 
free_memkb has gone back above that watermark. If that is not a concern, I can 
set free_memkb_prev outside of the if statement, and just use it to track 
changes between each iteration of the loop.

> Given that this new loop can take significantly longer to fail I wonder
> if we should add some progress logging? xl has an xtl logger instance
> available so using xtl_progress might be an easy option. Maybe a
> separate patch though.

Good idea. I'll look into adding that.

-Mike

Re: [PATCH] libxl: Wait for ballooning if free memory is increasing

[Ian Campbell]

On Wed, 2015-01-28 at 17:14 -0700, Mike Latimer wrote:
> > > +        if (free_memkb <= free_memkb_prev) {
> > > +            retries--;
> > 
> > I think you need to update prev here, otherwise after one successful
> > iteration the condition is always true even if progress subsequently
> > stalls.
> 
> If progress stalls, the above statement needs to be true in order to decrement 
> the retry count. The test is comparing free_memkb as set at the beginning of 
> the loop though, so it is not completely accurate. The next iteration of the 
> loop resets it, so progress should be caught (unless I'm missing something).
> 
> > > +        } else {
> > > +            retries = MAX_RETRIES;
> > > +            free_memkb_prev = free_memkb;
> > 
> > ... iow the second assignment here should be after the if/else entirely.
> 
> If there is a chance that free_memkb could drop lower between iterations, I 
> wanted free_memkb_prev to act as a watermark and only be updated once 
> free_memkb has gone back above that watermark. If that is not a concern, I can 
> set free_memkb_prev outside of the if statement, and just use it to track 
> changes between each iteration of the loop.

It turns out that I was very confused, starting with thinking we wanted
free_memkb to be decreasing for some reason!

I did myself a proper worked example and I now get what you are saying,
and I think your algorithm is indeed correct, sorry for the noise.

I'm thinking it would be clearer if the comment and the condition were
logically inverted. e.g.:

    /*
     * If the amount of free mem has increased on this iteration (i.e.
     * some progress has been made) then reset the retry counter.
     */
    if (freemem_kb > freemem_kb_prev) {
        retries = MAX_RETRIES;
        free_memkb_prev = free_memkb;
    } else {
        retires--;
    }

> 
> > Given that this new loop can take significantly longer to fail I wonder
> > if we should add some progress logging? xl has an xtl logger instance
> > available so using xtl_progress might be an easy option. Maybe a
> > separate patch though.
> 
> Good idea. I'll look into adding that.

Thanks.

Re: [PATCH] libxl: Wait for ballooning if free memory is increasing

[Mike Latimer]

On Thursday, January 29, 2015 10:14:26 AM Ian Campbell wrote:
> I'm thinking it would be clearer if the comment and the condition were
> logically inverted. e.g.:
> 
>     /*
>      * If the amount of free mem has increased on this iteration (i.e.
>      * some progress has been made) then reset the retry counter.
>      */
>     if (freemem_kb > freemem_kb_prev) {
>         retries = MAX_RETRIES;
>         free_memkb_prev = free_memkb;
>     } else {
>         retires--;
>     }

Thanks for the comments, Ian. Inverting the logic makes sense, and I'll send a 
v2 shortly.

> > > Given that this new loop can take significantly longer to fail I wonder
> > > if we should add some progress logging? xl has an xtl logger instance
> > > available so using xtl_progress might be an easy option. Maybe a
> > > separate patch though.

xtl_progress looks interesting. I'll do some additional testing before I 
submit a patch containing this improvement.

-Mike