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

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

[Dexuan Cui]

If num_ballooned is not 0, we shouldn't neglect the already-allocated 2MB
memory block(s).

Cc: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 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;
 		}
-- 
1.9.1

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

[Jason Wang]

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@microsoft.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  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
after the change, if part of 2M allocation were failed, we won't retry
4K allocation. Is this expected?

Btw, can host just require 1M? If yes, should alloc_balloon_pages() set
alloc_error if num_pages < alloc_unit for caller to catch this and retry
4K allocation?

Thanks

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

[Dexuan Cui]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Monday, November 24, 2014 13:18 PM
> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; 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@microsoft.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >  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.


> Btw, can host just require 1M? If yes, should alloc_balloon_pages() set
Hi KY,
Can you please clarify this?
You know the host much more than me. :-)

> alloc_error if num_pages < alloc_unit for caller to catch this and retry
> 4K allocation?
 
-- Dexuan

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

[Jason Wang]

On 11/24/2014 02:08 PM, Dexuan Cui wrote:
>> -----Original Message-----
>> > From: Jason Wang [mailto:jasowang@redhat.com]
>> > Sent: Monday, November 24, 2014 13:18 PM
>> > To: Dexuan Cui; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
>> > driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
>> > apw@canonical.com; 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@microsoft.com>
>>> > > Cc: <stable@vger.kernel.org>
>>> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
>>> > > ---
>>> > >  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?

Probably you need something like:

if ((alloc_unit != 1) && (num_ballooned == 0)) {
    alloc_unit = 1;
    continue;
}

if ((alloc_unit == 1) || (num_ballooned = num_pages)) {
    ...
}

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

[Dexuan Cui]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Monday, November 24, 2014 15:28 PM
> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; 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@redhat.com]
> >> > Sent: Monday, November 24, 2014 13:18 PM
> >> > To: Dexuan Cui; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org;
> >> > driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> >> > apw@canonical.com; 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@microsoft.com>
> >>> > > Cc: <stable@vger.kernel.org>
> >>> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> >>> > > ---
> >>> > >  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.

Thanks,
-- Dexuan

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

[Jason Wang]

On 11/24/2014 03:54 PM, Dexuan Cui wrote:
>> -----Original Message-----
>> From: Jason Wang [mailto:jasowang@redhat.com]
>> Sent: Monday, November 24, 2014 15:28 PM
>> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
>> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
>> apw@canonical.com; 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@redhat.com]
>>>>> Sent: Monday, November 24, 2014 13:18 PM
>>>>> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-
>> kernel@vger.kernel.org;
>>>>> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
>>>>> apw@canonical.com; 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@microsoft.com>
>>>>>>> Cc: <stable@vger.kernel.org>
>>>>>>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
>>>>>>> ---
>>>>>>>  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.
>
> Thanks,
> -- Dexuan

Looks good.

Thanks

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

[Dexuan Cui]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Monday, November 24, 2014 16:48 PM
> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; 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@redhat.com]
> >> Sent: Monday, November 24, 2014 15:28 PM
> >> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org;
> >> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> >> apw@canonical.com; 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@redhat.com]
> >>>>> Sent: Monday, November 24, 2014 13:18 PM
> >>>>> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-
> >> kernel@vger.kernel.org;
> >>>>> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> >>>>> apw@canonical.com; 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@microsoft.com>
> >>>>>>> Cc: <stable@vger.kernel.org>
> >>>>>>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> >>>>>>> ---
> >>>>>>>  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

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@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; 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@redhat.com]
> > Sent: Monday, November 24, 2014 16:48 PM
> > To: Dexuan Cui; gregkh@linuxfoundation.org;
> > linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > olaf@aepfle.de; apw@canonical.com; 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@redhat.com]
> > >> Sent: Monday, November 24, 2014 15:28 PM
> > >> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-
> > kernel@vger.kernel.org;
> > >> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> > >> apw@canonical.com; 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@redhat.com]
> > >>>>> Sent: Monday, November 24, 2014 13:18 PM
> > >>>>> To: Dexuan Cui; gregkh@linuxfoundation.org; linux-
> > >> kernel@vger.kernel.org;
> > >>>>> driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
> > >>>>> apw@canonical.com; 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@microsoft.com>
> > >>>>>>> Cc: <stable@vger.kernel.org>
> > >>>>>>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > >>>>>>> ---
> > >>>>>>>  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
>