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

[Ian Campbell <Ian.Campbell@citrix.com>]

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.