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

Dmitry Torokhov
2015-04-16T20:56:07Z

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@vmware.com>
> ---
> 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