RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block

[KY Srinivasan]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Monday, November 24, 2014 12:55 AM
> To: Jason Wang; 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
> 
> > -----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.

The patch looks good. Yes, the host balloons pages in 2M granularity. This is the behavior of the
current Windows hosts supporting this feature.

Regards,

K. Y
> 
> I'll send the v2 thereafter.
> 
> -- Dexuan
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/