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@xxxxxxxxxx> > Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> Thanks! Laszlo _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel