From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] libxl: Wait for ballooning if free memory is increasing Date: Thu, 29 Jan 2015 10:14:26 +0000 Message-ID: <1422526466.30641.16.camel@citrix.com> References: <1421904173-2941-1-git-send-email-mlatimer@suse.com> <1422450325.14124.24.camel@citrix.com> <20039882.y0cehhMcW2@mlatimer1.dnsdhcp.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20039882.y0cehhMcW2@mlatimer1.dnsdhcp.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mike Latimer Cc: George Dunlap , Wei Liu , Stefano Stabellini , Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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.