Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[Dmitry Torokhov]

Hi Philip,

On Tue, Apr 14, 2015 at 10:29:33AM -0700, Philip P. Moltmann wrote:
> Before this patch the slow memory transfer would cause the destination VM to
> have internal swapping until all memory is transferred. Now the memory is
> transferred fast enough so that the destination VM does not swap. The balloon
> loop already yields to the rest of the system, hence the balloon thread
> should not monopolize a cpu.
> 
> Testing Done: quickly ballooned a lot of pages while wathing if there are any
> perceived hickups (periods of non-responsiveness) in the execution of the
> linux VM. No such hickups were seen.

What happens if you run this new driver on an older hypervisor that does
not support batched operations?

> 
> Signed-off-by: Xavier Deguillard <xdeguillard@xxxxxxxxxx>
> ---
>  drivers/misc/vmw_balloon.c | 66 +++++++++++-----------------------------------
>  1 file changed, 15 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
> index 6eaf7f7..a5e1980 100644
> --- a/drivers/misc/vmw_balloon.c
> +++ b/drivers/misc/vmw_balloon.c
> @@ -46,7 +46,7 @@
>  
>  MODULE_AUTHOR("VMware, Inc.");
>  MODULE_DESCRIPTION("VMware Memory Control (Balloon) Driver");
> -MODULE_VERSION("1.3.3.0-k");
> +MODULE_VERSION("1.3.4.0-k");
>  MODULE_ALIAS("dmi:*:svnVMware*:*");
>  MODULE_ALIAS("vmware_vmmemctl");
>  MODULE_LICENSE("GPL");
> @@ -57,12 +57,6 @@ MODULE_LICENSE("GPL");
>   */
>  
>  /*
> - * Rate of allocating memory when there is no memory pressure
> - * (driver performs non-sleeping allocations).
> - */
> -#define VMW_BALLOON_NOSLEEP_ALLOC_MAX	16384U
> -
> -/*
>   * Rates of memory allocaton when guest experiences memory pressure
>   * (driver performs sleeping allocations).
>   */
> @@ -71,13 +65,6 @@ MODULE_LICENSE("GPL");
>  #define VMW_BALLOON_RATE_ALLOC_INC	16U
>  
>  /*
> - * Rates for releasing pages while deflating balloon.
> - */
> -#define VMW_BALLOON_RATE_FREE_MIN	512U
> -#define VMW_BALLOON_RATE_FREE_MAX	16384U
> -#define VMW_BALLOON_RATE_FREE_INC	16U
> -
> -/*
>   * When guest is under memory pressure, use a reduced page allocation
>   * rate for next several cycles.
>   */
> @@ -99,9 +86,6 @@ MODULE_LICENSE("GPL");
>   */
>  #define VMW_PAGE_ALLOC_CANSLEEP		(GFP_HIGHUSER)
>  
> -/* Maximum number of page allocations without yielding processor */
> -#define VMW_BALLOON_YIELD_THRESHOLD	1024
> -
>  /* Maximum number of refused pages we accumulate during inflation cycle */
>  #define VMW_BALLOON_MAX_REFUSED		16
>  
> @@ -278,7 +262,6 @@ struct vmballoon {
>  
>  	/* adjustment rates (pages per second) */
>  	unsigned int rate_alloc;
> -	unsigned int rate_free;
>  
>  	/* slowdown page allocations for next few cycles */
>  	unsigned int slow_allocation_cycles;
> @@ -502,18 +485,13 @@ static bool vmballoon_send_batched_unlock(struct vmballoon *b,
>  static void vmballoon_pop(struct vmballoon *b)
>  {
>  	struct page *page, *next;
> -	unsigned int count = 0;
>  
>  	list_for_each_entry_safe(page, next, &b->pages, lru) {
>  		list_del(&page->lru);
>  		__free_page(page);
>  		STATS_INC(b->stats.free);
>  		b->size--;
> -
> -		if (++count >= b->rate_free) {
> -			count = 0;
> -			cond_resched();
> -		}
> +		cond_resched();
>  	}
>  
>  	if ((b->capabilities & VMW_BALLOON_BATCHED_CMDS) != 0) {
> @@ -742,13 +720,13 @@ static void vmballoon_inflate(struct vmballoon *b)
>  	 * Start with no sleep allocation rate which may be higher
>  	 * than sleeping allocation rate.
>  	 */
> -	rate = b->slow_allocation_cycles ?
> -			b->rate_alloc : VMW_BALLOON_NOSLEEP_ALLOC_MAX;
> +	rate = b->slow_allocation_cycles ? b->rate_alloc : -1;
>  
>  	pr_debug("%s - goal: %d, no-sleep rate: %d, sleep rate: %d\n",
>  		 __func__, b->target - b->size, rate, b->rate_alloc);
>  
> -	while (b->size < b->target && num_pages < b->target - b->size) {
> +	while (!b->reset_required &&
> +		b->size < b->target && num_pages < b->target - b->size) {
>  		struct page *page;
>  
>  		if (flags == VMW_PAGE_ALLOC_NOSLEEP)
> @@ -798,12 +776,9 @@ static void vmballoon_inflate(struct vmballoon *b)
>  				break;
>  		}
>  
> -		if (++allocations > VMW_BALLOON_YIELD_THRESHOLD) {
> -			cond_resched();
> -			allocations = 0;
> -		}
> +		cond_resched();
>  
> -		if (allocations >= rate) {
> +		if (rate != -1 && allocations >= rate) {
>  			/* We allocated enough pages, let's take a break. */

Why don't you make the rate UINT_MAX when doing fast allocations, then
you would not need to treat -1 as a special case.

>  			break;
>  		}
> @@ -837,36 +812,29 @@ static void vmballoon_deflate(struct vmballoon *b)
>  	unsigned int num_pages = 0;
>  	int error;
>  
> -	pr_debug("%s - size: %d, target %d, rate: %d\n", __func__, b->size,
> -						b->target, b->rate_free);
> +	pr_debug("%s - size: %d, target %d\n", __func__, b->size, b->target);
>  
>  	/* free pages to reach target */
>  	list_for_each_entry_safe(page, next, &b->pages, lru) {
>  		list_del(&page->lru);
>  		b->ops->add_page(b, num_pages++, page);
>  
> +

Seems line unintended whitespace addition.

>  		if (num_pages == b->batch_max_pages) {
>  			error = b->ops->unlock(b, num_pages, &b->target);
>  			num_pages = 0;
> -			if (error) {
> -				/* quickly decrease rate in case of error */
> -				b->rate_free = max(b->rate_free / 2,
> -						VMW_BALLOON_RATE_FREE_MIN);
> +			if (error)
>  				return;
> -			}
>  		}
>  
> -		if (++i >= b->size - b->target)
> +		if (b->reset_required || ++i >= b->size - b->target)
>  			break;
> +
> +		cond_resched();
>  	}
>  
>  	if (num_pages > 0)
>  		b->ops->unlock(b, num_pages, &b->target);
> -
> -	/* slowly increase rate if there were no errors */
> -	if (error == 0)
> -		b->rate_free = min(b->rate_free + VMW_BALLOON_RATE_FREE_INC,
> -				   VMW_BALLOON_RATE_FREE_MAX);
>  }
>  
>  static const struct vmballoon_ops vmballoon_basic_ops = {
> @@ -992,11 +960,8 @@ static int vmballoon_debug_show(struct seq_file *f, void *offset)
>  
>  	/* format rate info */
>  	seq_printf(f,
> -		   "rateNoSleepAlloc:   %8d pages/sec\n"
> -		   "rateSleepAlloc:     %8d pages/sec\n"
> -		   "rateFree:           %8d pages/sec\n",
> -		   VMW_BALLOON_NOSLEEP_ALLOC_MAX,
> -		   b->rate_alloc, b->rate_free);
> +		   "rateSleepAlloc:     %8d pages/sec\n",
> +		   b->rate_alloc);
>  
>  	seq_printf(f,
>  		   "\n"
> @@ -1087,7 +1052,6 @@ static int __init vmballoon_init(void)
>  
>  	/* initialize rates */
>  	balloon.rate_alloc = VMW_BALLOON_RATE_ALLOC_MAX;
> -	balloon.rate_free = VMW_BALLOON_RATE_FREE_MAX;
>  
>  	INIT_DELAYED_WORK(&balloon.dwork, vmballoon_work);

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/