RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
> -Original Message-
> From: Dexuan Cui
> Sent: Monday, November 24, 2014 12:55 AM
> To: Jason Wang; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> a...@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:jasow...@redhat.com]
> > Sent: Monday, November 24, 2014 16:48 PM
> > To: Dexuan Cui; gre...@linuxfoundation.org;
> > linux-kernel@vger.kernel.org; driverdev-de...@linuxdriverproject.org;
> > o...@aepfle.de; a...@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:jasow...@redhat.com]
> > >> Sent: Monday, November 24, 2014 15:28 PM
> > >> To: Dexuan Cui; gre...@linuxfoundation.org; linux-
> > ker...@vger.kernel.org;
> > >> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> > >> a...@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:jasow...@redhat.com]
> > >>>>> Sent: Monday, November 24, 2014 13:18 PM
> > >>>>> To: Dexuan Cui; gre...@linuxfoundation.org; linux-
> > >> ker...@vger.kernel.org;
> > >>>>> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> > >>>>> a...@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
> > >>>>>>> Cc:
> > >>>>>>> Signed-off-by: Dexuan Cui
> > >>>>>>> ---
> > >>>>>>> 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 @@
> > >>>>>>> -,7 +1113,7 @@ static void balloon_up(struct
> > >> work_struct
> > >>>>> *dummy)
> > >>>>>>> bl_resp, alloc_unit,
> > >>>>>>> _error);
> > >>>>>>>
> > >>>>>>> - if ((alloc_error) && (alloc_unit != 1)) {
> > >>>>>>> + if (alloc_error && (alloc_unit != 1) &&
> > >> num_ballooned == 0)
> > >>>>> {
> > >>>>>>> alloc_unit = 1;
> > >>>>>>> continue;
> > >>>>>>> }
> > >>>>> Before the change
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Monday, November 24, 2014 16:48 PM
> To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> a...@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:jasow...@redhat.com]
> >> Sent: Monday, November 24, 2014 15:28 PM
> >> To: Dexuan Cui; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org;
> >> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> >> a...@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:jasow...@redhat.com]
> >>>>> Sent: Monday, November 24, 2014 13:18 PM
> >>>>> To: Dexuan Cui; gre...@linuxfoundation.org; linux-
> >> ker...@vger.kernel.org;
> >>>>> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> >>>>> a...@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
> >>>>>>> Cc:
> >>>>>>> Signed-off-by: Dexuan Cui
> >>>>>>> ---
> >>>>>>> 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
> >>>>>>> @@ -,7 +1113,7 @@ static void balloon_up(struct
> >> work_struct
> >>>>> *dummy)
> >>>>>>> bl_resp, alloc_unit,
> >>>>>>>_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_e
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:jasow...@redhat.com]
>> Sent: Monday, November 24, 2014 15:28 PM
>> To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
>> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
>> a...@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:jasow...@redhat.com]
>>>>> Sent: Monday, November 24, 2014 13:18 PM
>>>>> To: Dexuan Cui; gre...@linuxfoundation.org; linux-
>> ker...@vger.kernel.org;
>>>>> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
>>>>> a...@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
>>>>>>> Cc:
>>>>>>> Signed-off-by: Dexuan Cui
>>>>>>> ---
>>>>>>> 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
>>>>>>> @@ -,7 +1113,7 @@ static void balloon_up(struct
>> work_struct
>>>>> *dummy)
>>>>>>> bl_resp, alloc_unit,
>>>>>>> _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;
>> con
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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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
@@ -,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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
-Original Message-
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Monday, November 24, 2014 16:48 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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
@@ -,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
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
-Original Message-
From: Dexuan Cui
Sent: Monday, November 24, 2014 12:55 AM
To: Jason Wang; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 16:48 PM
To: Dexuan Cui; gre...@linuxfoundation.org;
linux-kernel@vger.kernel.org; driverdev-de...@linuxdriverproject.org;
o...@aepfle.de; a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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 @@
-,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
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Monday, November 24, 2014 15:28 PM
> To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> a...@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:jasow...@redhat.com]
> >> > Sent: Monday, November 24, 2014 13:18 PM
> >> > To: Dexuan Cui; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org;
> >> > driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> >> > a...@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
> >>> > > Cc:
> >>> > > Signed-off-by: Dexuan Cui
> >>> > > ---
> >>> > > 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
> >>> > > @@ -,7 +1113,7 @@ static void balloon_up(struct
> work_struct
> >> > *dummy)
> >>> > > bl_resp, alloc_unit,
> >>> > >_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)
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:jasow...@redhat.com]
>> > Sent: Monday, November 24, 2014 13:18 PM
>> > To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
>> > driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
>> > a...@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
>>> > > Cc:
>>> > > Signed-off-by: Dexuan Cui
>>> > > ---
>>> > > 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
>>> > > @@ -,7 +1113,7 @@ static void balloon_up(struct work_struct
>> > *dummy)
>>> > > bl_resp, alloc_unit,
>>> > > _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)) {
...
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Monday, November 24, 2014 13:18 PM
> To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
> a...@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
> > Cc:
> > Signed-off-by: Dexuan Cui
> > ---
> > 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
> > @@ -,7 +1113,7 @@ static void balloon_up(struct work_struct
> *dummy)
> > bl_resp, alloc_unit,
> > _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
> Cc:
> Signed-off-by: Dexuan Cui
> ---
> 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
> @@ -,7 +1113,7 @@ static void balloon_up(struct work_struct *dummy)
> bl_resp, alloc_unit,
>_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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
@@ -,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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
-Original Message-
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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
@@ -,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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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
@@ -,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)) {
...
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
-Original Message-
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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:jasow...@redhat.com]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org;
driverdev-de...@linuxdriverproject.org; o...@aepfle.de;
a...@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 k...@microsoft.com
Cc: sta...@vger.kernel.org
Signed-off-by: Dexuan Cui de...@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
@@ -,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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please
