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 <ler...@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuzn...@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 <ler...@redhat.com>
Thanks!
Laszlo
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
