Michael Kelley <mikelley@xxxxxxxxxxxxx> writes:
> From: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx> Sent: Thursday, January 16, 2020 6:16 AM
>>
>> Current code has assumption that balloon request memory size aligns
>> with 2MB. But actually Hyper-V doesn't guarantee such alignment. When
>> balloon driver receives non-aligned balloon request, it produces warning
>> and balloon up more memory than requested in order to keep 2MB alignment.
>> Remove the warning and balloon up memory according to actual requested
>> memory size.
>>
>> Fixes: f6712238471a ("hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory
>> block")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>
>> ---
>> Change since v2:
>> - Change logic of switching alloc_unit from 2MB to 4KB
>> in the balloon_up() to avoid redundant iteration when
>> handle non-aligned page request.
>> - Remove 2MB alignment operation and comment in balloon_up()
>> ---
>> drivers/hv/hv_balloon.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>> index 7f3e7ab22d5d..536807efbc35 100644
>> --- a/drivers/hv/hv_balloon.c
>> +++ b/drivers/hv/hv_balloon.c
>> @@ -1684,7 +1684,7 @@ static unsigned int alloc_balloon_pages(struct
>> hv_dynmem_device *dm,
>> if (num_pages < alloc_unit)
>> return 0;
>
> The above test is no longer necessary. The num_pages < alloc_unit
> case is handled implicitly by your new 'for' loop condition.
>
>>
>> - for (i = 0; (i * alloc_unit) < num_pages; i++) {
>> + for (i = 0; i < num_pages / alloc_unit; i++) {
>> if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) >
>> HV_HYP_PAGE_SIZE)
>> return i * alloc_unit;
>> @@ -1722,7 +1722,7 @@ static unsigned int alloc_balloon_pages(struct
>> hv_dynmem_device *dm,
>>
>> }
>>
>> - return num_pages;
>> + return i * alloc_unit;
>> }
>>
>> static void balloon_up(union dm_msg_info *msg_info)
>> @@ -1737,9 +1737,6 @@ static void balloon_up(union dm_msg_info *msg_info)
>> long avail_pages;
>> unsigned long floor;
>>
>> - /* The host balloons pages in 2M granularity. */
>> - WARN_ON_ONCE(num_pages % PAGES_IN_2M != 0);
>> -
>> /*
>> * We will attempt 2M allocations. However, if we fail to
>> * allocate 2M chunks, we will go back to PAGE_SIZE allocations.
>> @@ -1749,14 +1746,13 @@ static void balloon_up(union dm_msg_info *msg_info)
>> avail_pages = si_mem_available();
>> floor = compute_balloon_floor();
>>
>> - /* Refuse to balloon below the floor, keep the 2M granularity. */
>> + /* Refuse to balloon below the floor. */
>> if (avail_pages < num_pages || avail_pages - num_pages < floor) {
>> pr_warn("Balloon request will be partially fulfilled. %s\n",
>> avail_pages < num_pages ? "Not enough memory." :
>> "Balloon floor reached.");
>>
>> num_pages = avail_pages > floor ? (avail_pages - floor) : 0;
>> - num_pages -= num_pages % PAGES_IN_2M;
>> }
>>
>> while (!done) {
>> @@ -1770,7 +1766,7 @@ static void balloon_up(union dm_msg_info *msg_info)
>> num_ballooned = alloc_balloon_pages(&dm_device, num_pages,
>> bl_resp, alloc_unit);
>>
>> - if (alloc_unit != 1 && num_ballooned == 0) {
>> + if (alloc_unit != 1 && num_ballooned != num_pages) {
>
> Maybe I'm missing something, but I don't think Vitaly's optimization works. If
> alloc_unit specifies 2 Mbytes, and num_pages specifies 3 Mbytes, then num_ballooned
> will come back as 2 Mbytes, which is correct. But if we revert alloc_unit to 1 page and
> "continue" in that case, we will lose the 2 Mbytes of memory (it's not freed), and the
> next time through the loop will try to allocate only 1 Mbyte (because num_pages
> will be decremented by num_ballooned). I think the original code does the right thing.
>
True, nice catch! What I meant to say is that we can avoid sending two
replies to the host and get away with just one. Unfortunately,
"num_ballooned != num_pages" modification is not sufficient as
alloc_balloon_pages() will overwrite bl_resp->range_array[] (as it
always starts from 0). I think we can still achieve the goal by
allocating bl_resp outside of the loop and filling it from
range_array[range_count] instead of range_array[i] but it seems to big
of a change for now particular gain. Let's just drop the idea.
> Michael
>
>> alloc_unit = 1;
>> continue;
>> }
>> --
>> 2.14.5
>
--
Vitaly
