> -----Original Message----- > From: Jason Wang [mailto:jasowang@xxxxxxxxxx] > Sent: Monday, November 24, 2014 16:48 PM > To: Dexuan Cui; gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; KY Srinivasan > Cc: Haiyang Zhang > Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of > 2MB memory block > > On 11/24/2014 03:54 PM, Dexuan Cui wrote: > >> -----Original Message----- > >> From: Jason Wang [mailto:jasowang@xxxxxxxxxx] > >> Sent: Monday, November 24, 2014 15:28 PM > >> To: Dexuan Cui; gregkh@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; > >> driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > >> apw@xxxxxxxxxxxxx; KY Srinivasan > >> Cc: Haiyang Zhang > >> Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error > of > >> 2MB memory block > >> > >> On 11/24/2014 02:08 PM, Dexuan Cui wrote: > >>>> -----Original Message----- > >>>>> From: Jason Wang [mailto:jasowang@xxxxxxxxxx] > >>>>> Sent: Monday, November 24, 2014 13:18 PM > >>>>> To: Dexuan Cui; gregkh@xxxxxxxxxxxxxxxxxxx; linux- > >> kernel@xxxxxxxxxxxxxxx; > >>>>> driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > >>>>> apw@xxxxxxxxxxxxx; KY Srinivasan > >>>>> Cc: Haiyang Zhang > >>>>> Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on > >> alloc_error of > >>>>> 2MB memory block > >>>>> > >>>>> On 11/24/2014 01:56 PM, Dexuan Cui wrote: > >>>>>>> If num_ballooned is not 0, we shouldn't neglect the already- > >> allocated > >>>>> 2MB > >>>>>>> memory block(s). > >>>>>>> > >>>>>>> Cc: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> > >>>>>>> Cc: <stable@xxxxxxxxxxxxxxx> > >>>>>>> Signed-off-by: Dexuan Cui <decui@xxxxxxxxxxxxx> > >>>>>>> --- > >>>>>>> drivers/hv/hv_balloon.c | 4 +++- > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c > >>>>>>> index 5e90c5d..cba2d3b 100644 > >>>>>>> --- a/drivers/hv/hv_balloon.c > >>>>>>> +++ b/drivers/hv/hv_balloon.c > >>>>>>> @@ -1091,6 +1091,8 @@ static void balloon_up(struct > >> work_struct > >>>>> *dummy) > >>>>>>> bool done = false; > >>>>>>> int i; > >>>>>>> > >>>>>>> + /* The host does balloon_up in 2MB. */ > >>>>>>> + WARN_ON(num_pages % PAGES_IN_2M != 0); > >>>>>>> > >>>>>>> /* > >>>>>>> * We will attempt 2M allocations. However, if we fail to > >>>>>>> @@ -1111,7 +1113,7 @@ static void balloon_up(struct > >> work_struct > >>>>> *dummy) > >>>>>>> bl_resp, alloc_unit, > >>>>>>> &alloc_error); > >>>>>>> > >>>>>>> - if ((alloc_error) && (alloc_unit != 1)) { > >>>>>>> + if (alloc_error && (alloc_unit != 1) && > >> num_ballooned == 0) > >>>>> { > >>>>>>> alloc_unit = 1; > >>>>>>> continue; > >>>>>>> } > >>>>> Before the change, we may retry the 4K allocation when part or all > 2M > >>>>> allocations were failed. This makes sense when memory is > fragmented. > >> But > >>> Yes, but all the partially-allocated 2MB memory blocks are lost(mem > leak). > >>> > >>>>> after the change, if part of 2M allocation were failed, we won't retry > >>>>> 4K allocation. Is this expected? > >>> Hi Jason, > >>> The patch doesn't break the "try 2MB first; then try 4K" logic: > >>> > >>> With the change, we'll retry the 2MB allocation in the next iteration of > the > >>> same while (!done) loop -- we expect this retry will cause > >>> "alloc_error && (alloc_unit != 1) && num_ballooned == 0" to be true, > >>> so we'll later try 4K allocation, as we did before. > >> If I read the code correctly, if part of 2M allocation fails. > >> alloc_balloon_pages() will have a non zero return value with alloc_error > >> set. Then it will match the following check: > >> > >> if ((alloc_error) || (num_ballooned == num_pages)) > >> { > >> > >> which will set done to true. So there's no second iteration of while > >> (!done) loop? > > Oh... I see the issue in my patch. > > Thanks for pointing this out, Jason! > > > >> Probably you need something like: > >> > >> if ((alloc_unit != 1) && (num_ballooned == 0)) { > >> alloc_unit = 1; > >> continue; > >> } > >> > >> if ((alloc_unit == 1) || (num_ballooned == num_pages)) { > >> ... > >> } > > Your code is good, except for one thing: > > alloc_balloon_pages() can return due to: > > > > if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) > > > PAGE_SIZE) > > return i * alloc_unit; > > > > In this case, "alloc_unit == 1" is true, but we shouldn't run "done = true". > > > > How do you like this? I made a few changes based on your code. > > > > @@ -1086,16 +1088,18 @@ 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); > > > > - if ((alloc_error) && (alloc_unit != 1)) { > > + if (alloc_unit != 1 && num_ballooned == 0) { > > alloc_unit = 1; > > continue; > > } > > > > - if ((alloc_error) || (num_ballooned == num_pages)) { > > + if ((alloc_unit == 1 && alloc_error) || > > + (num_ballooned == num_pages)) { > > bl_resp->more_pages = 0; > > done = true; > > dm_device.state = DM_INITIALIZED; > > > > > > If you're Ok with this, I'll send out a v2 patch. > > -- Dexuan > > Looks good. Thanks, Jason! Let's wait for KY's clarification about if the host can balloon up 1MB. I'll send the v2 thereafter. -- Dexuan _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
