[PATCH] Drivers: hv: hv_balloon: survive ballooning request with num_pages=0

[PATCH] Drivers: hv: hv_balloon: survive ballooning request with num_pages=0

[Vitaly Kuznetsov]

... and simplify alloc_balloon_pages() interface by removing redundant
alloc_error from it.

If we happen to enter balloon_up() with balloon_wrk.num_pages = 0 we will enter
infinite 'while (!done)' loop as alloc_balloon_pages() will be always returning
0 and not setting alloc_error. We will also be sending a meaningless message to
the host on every iteration.

The 'alloc_unit == 1 && alloc_error -> num_ballooned == 0' change and
alloc_error elimination requires a special comment. We do alloc_balloon_pages()
with 2 different alloc_unit values and there are 4 different
alloc_balloon_pages() results, let's check them all.

alloc_unit = 512:
1) num_ballooned = 0, alloc_error = 0: we do 'alloc_unit=1' and retry pre- and
  post-patch.
2) num_ballooned > 0, alloc_error = 0: we check 'num_ballooned == num_pages'
  and act accordingly,  pre- and post-patch.
3) num_ballooned > 0, alloc_error > 0: we report this chunk and remain within
  the loop, no changes here.
4) num_ballooned = 0, alloc_error > 0: we do 'alloc_unit=1' and retry pre- and
  post-patch.

alloc_unit = 1:
1) num_ballooned = 0, alloc_error = 0: this can happen in two cases: when we
  passed 'num_pages=0' to alloc_balloon_pages() or when there was no space in
  bl_resp to place a single response. The second option is not possible as
  bl_resp is of PAGE_SIZE size and single response 'union dm_mem_page_range' is
  8 bytes, but the first one is (in theory, I think that Hyper-V host never
  places such requests). Pre-patch code loops forever, post-patch code sends
  a reply with more_pages = 0 and finishes.
2) num_ballooned > 0, alloc_error = 0: we ran out of space in bl_resp, we
  report partial success and remain within the loop, no changes pre- and
  post-patch.
3) num_ballooned > 0, alloc_error > 0: pre-patch code finishes, post-patch code
  does one more try and if there is no progress (we finish with
  'num_ballooned = 0') we finish. So we try a bit harder with this patch.
4) num_ballooned = 0, alloc_error > 0: both pre- and post-patch code enter
 'more_pages = 0' branch and finish.

So this patch has two real effects:
1) We reply with an empty response to 'num_pages=0' request.
2) We try a bit harder on alloc_unit=1 allocations (and reply with an empty
   tail reply in case we fail).

An empty reply should be supported by host as we were able to send it even with
pre-patch code when we were not able to allocate a single page.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/hv_balloon.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 16d52da..d8c5802 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1071,9 +1071,9 @@ static void free_balloon_pages(struct hv_dynmem_device *dm,
 
 
 
-static int  alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
-			 struct dm_balloon_response *bl_resp, int alloc_unit,
-			 bool *alloc_error)
+static int alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
+			       struct dm_balloon_response *bl_resp,
+			       int alloc_unit)
 {
 	int i = 0;
 	struct page *pg;
@@ -1094,11 +1094,8 @@ static int  alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
 				__GFP_NOMEMALLOC | __GFP_NOWARN,
 				get_order(alloc_unit << PAGE_SHIFT));
 
-		if (!pg) {
-			*alloc_error = true;
+		if (!pg)
 			return i * alloc_unit;
-		}
-
 
 		dm->num_pages_ballooned += alloc_unit;
 
@@ -1130,7 +1127,6 @@ static void balloon_up(struct work_struct *dummy)
 	struct dm_balloon_response *bl_resp;
 	int alloc_unit;
 	int ret;
-	bool alloc_error;
 	bool done = false;
 	int i;
 	struct sysinfo val;
@@ -1163,18 +1159,15 @@ static void balloon_up(struct work_struct *dummy)
 
 
 		num_pages -= num_ballooned;
-		alloc_error = false;
 		num_ballooned = alloc_balloon_pages(&dm_device, num_pages,
-						bl_resp, alloc_unit,
-						 &alloc_error);
+						    bl_resp, alloc_unit);
 
 		if (alloc_unit != 1 && num_ballooned == 0) {
 			alloc_unit = 1;
 			continue;
 		}
 
-		if ((alloc_unit == 1 && alloc_error) ||
-			(num_ballooned == num_pages)) {
+		if (num_ballooned == 0 || num_ballooned == num_pages) {
 			bl_resp->more_pages = 0;
 			done = true;
 			dm_device.state = DM_INITIALIZED;
-- 
1.9.3

Re: [PATCH] Drivers: hv: hv_balloon: survive ballooning request with num_pages=0

[Laszlo Ersek]

On 03/26/15 17:07, Vitaly Kuznetsov wrote:
> ... and simplify alloc_balloon_pages() interface by removing redundant
> alloc_error from it.
> 
> If we happen to enter balloon_up() with balloon_wrk.num_pages = 0 we will enter
> infinite 'while (!done)' loop as alloc_balloon_pages() will be always returning
> 0 and not setting alloc_error. We will also be sending a meaningless message to
> the host on every iteration.
> 
> The 'alloc_unit == 1 && alloc_error -> num_ballooned == 0' change and
> alloc_error elimination requires a special comment. We do alloc_balloon_pages()
> with 2 different alloc_unit values and there are 4 different
> alloc_balloon_pages() results, let's check them all.
> 
> alloc_unit = 512:
> 1) num_ballooned = 0, alloc_error = 0: we do 'alloc_unit=1' and retry pre- and
>   post-patch.
> 2) num_ballooned > 0, alloc_error = 0: we check 'num_ballooned == num_pages'
>   and act accordingly,  pre- and post-patch.
> 3) num_ballooned > 0, alloc_error > 0: we report this chunk and remain within
>   the loop, no changes here.
> 4) num_ballooned = 0, alloc_error > 0: we do 'alloc_unit=1' and retry pre- and
>   post-patch.
> 
> alloc_unit = 1:
> 1) num_ballooned = 0, alloc_error = 0: this can happen in two cases: when we
>   passed 'num_pages=0' to alloc_balloon_pages() or when there was no space in
>   bl_resp to place a single response. The second option is not possible as
>   bl_resp is of PAGE_SIZE size and single response 'union dm_mem_page_range' is
>   8 bytes, but the first one is (in theory, I think that Hyper-V host never
>   places such requests). Pre-patch code loops forever, post-patch code sends
>   a reply with more_pages = 0 and finishes.
> 2) num_ballooned > 0, alloc_error = 0: we ran out of space in bl_resp, we
>   report partial success and remain within the loop, no changes pre- and
>   post-patch.
> 3) num_ballooned > 0, alloc_error > 0: pre-patch code finishes, post-patch code
>   does one more try and if there is no progress (we finish with
>   'num_ballooned = 0') we finish. So we try a bit harder with this patch.
> 4) num_ballooned = 0, alloc_error > 0: both pre- and post-patch code enter
>  'more_pages = 0' branch and finish.
> 
> So this patch has two real effects:
> 1) We reply with an empty response to 'num_pages=0' request.
> 2) We try a bit harder on alloc_unit=1 allocations (and reply with an empty
>    tail reply in case we fail).
> 
> An empty reply should be supported by host as we were able to send it even with
> pre-patch code when we were not able to allocate a single page.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/hv_balloon.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 16d52da..d8c5802 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1071,9 +1071,9 @@ static void free_balloon_pages(struct hv_dynmem_device *dm,
>  
>  
>  
> -static int  alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
> -			 struct dm_balloon_response *bl_resp, int alloc_unit,
> -			 bool *alloc_error)
> +static int alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
> +			       struct dm_balloon_response *bl_resp,
> +			       int alloc_unit)
>  {
>  	int i = 0;
>  	struct page *pg;
> @@ -1094,11 +1094,8 @@ static int  alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
>  				__GFP_NOMEMALLOC | __GFP_NOWARN,
>  				get_order(alloc_unit << PAGE_SHIFT));
>  
> -		if (!pg) {
> -			*alloc_error = true;
> +		if (!pg)
>  			return i * alloc_unit;
> -		}
> -
>  
>  		dm->num_pages_ballooned += alloc_unit;
>  
> @@ -1130,7 +1127,6 @@ static void balloon_up(struct work_struct *dummy)
>  	struct dm_balloon_response *bl_resp;
>  	int alloc_unit;
>  	int ret;
> -	bool alloc_error;
>  	bool done = false;
>  	int i;
>  	struct sysinfo val;
> @@ -1163,18 +1159,15 @@ static void balloon_up(struct work_struct *dummy)
>  
>  
>  		num_pages -= num_ballooned;
> -		alloc_error = false;
>  		num_ballooned = alloc_balloon_pages(&dm_device, num_pages,
> -						bl_resp, alloc_unit,
> -						 &alloc_error);
> +						    bl_resp, alloc_unit);
>  
>  		if (alloc_unit != 1 && num_ballooned == 0) {
>  			alloc_unit = 1;
>  			continue;
>  		}
>  
> -		if ((alloc_unit == 1 && alloc_error) ||
> -			(num_ballooned == num_pages)) {
> +		if (num_ballooned == 0 || num_ballooned == num_pages) {
>  			bl_resp->more_pages = 0;
>  			done = true;
>  			dm_device.state = DM_INITIALIZED;
> 

Perfect, that's what I had in mind.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo