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

[Mike Latimer <mlatimer@xxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel