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

PATCH Superseded Dexuan Cui 3 participants 8 messages Nov 24, 2014 Hyper-V/Azure core and drivers

lore

Share & tools

lore

Dexuan Cui

author maintainer

2014-11-24 04:47 UTC #0 lore raw

PATCH 1/1 hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block

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

Cc K. Y. Srinivasan <[email protected]>

Cc <[email protected]>

Signed-off-by Dexuan Cui <[email protected]>

---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/hv/hv_balloon.c +3 -1

index 5e90c5d..cba2d3b 100644
@@ -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;
 		}

Signature · 1 line

1.9.1

Jason Wang

2014-11-24 05:40 UTC #1 lore raw

Quoted 24 lines · Dexuan Cui

If num_ballooned is not 0, we shouldn't neglect the already-allocated 2MB memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/hv/hv_balloon.c +2 -0
index 5e90c5d..cba2d3b 100644
@@ -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

drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy) · line 1119

Quoted 8 lines

drivers/hv/hv_balloon.c +1 -1

@@ -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

Dexuan Cui

author maintainer

2014-11-24 06:24 UTC #2 lore raw

Quoted 44 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 1 line · Dexuan Cui
If num_ballooned is not 0, we shouldn't neglect the already-allocated
2MB
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct
Quoted 14 lines
memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/hv/hv_balloon.c +0 -0
index 5e90c5d..cba2d3b 100644
@@ -1091,6 +1091,8 @@ static void balloon_up(struct work_struct
*dummy)
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct
Quoted 9 lines
drivers/hv/hv_balloon.c +2 -0
 	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)
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c +1 -1
 						bl_resp, alloc_unit,
 						 &alloc_error);
 
-		if ((alloc_error) && (alloc_unit != 1)) {
+		if (alloc_error && (alloc_unit != 1) && num_ballooned == 0)
{

Quoted 6 lines

drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy) · line 1119
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -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).

Quoted 2 lines

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.

Quoted 1 line

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. :-)

Quoted 2 lines

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

-- Dexuan

Jason Wang

2014-11-24 07:28 UTC #3 lore raw

Quoted 54 lines · Dexuan Cui

Quoted 50 lines
-----Original Message-----
Quoted 49 lines
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 1 line · Dexuan Cui
Quoted 1 line
If num_ballooned is not 0, we shouldn't neglect the already-allocated
2MB
Quoted 9 lines
Quoted 9 lines
memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct
Quoted 5 lines
drivers/hv/hv_balloon.c +0 -0
index 5e90c5d..cba2d3b 100644
@@ -1091,6 +1091,8 @@ static void balloon_up(struct work_struct
*dummy)
Quoted 9 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct
Quoted 9 lines
drivers/hv/hv_balloon.c +2 -0
 	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)
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c +1 -1
 						bl_resp, alloc_unit,
 						 &alloc_error);
 
-		if ((alloc_error) && (alloc_unit != 1)) {
+		if (alloc_error && (alloc_unit != 1) && num_ballooned == 0)
{
Quoted 3 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy) · line 1119
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -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).
Quoted 2 lines
Quoted 2 lines
after the change, if part of 2M allocation were failed, we won't retry 4K allocation. Is this expected?

Quoted 7 lines

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)) {
    ...
}

Dexuan Cui

author maintainer

2014-11-24 07:55 UTC #4 lore raw

Quoted 84 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 4 lines · Dexuan Cui
Quoted 4 lines
-----Original Message-----
Quoted 3 lines
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; [email protected]; linux-
[email protected];
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
[email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on
alloc_error of
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
2MB memory block
Quoted 1 line · Dexuan Cui
Quoted 1 line
If num_ballooned is not 0, we shouldn't neglect the already-
allocated
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
2MB
Quoted 9 lines
Quoted 9 lines
memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 5 lines
drivers/hv/hv_balloon.c +0 -0
index 5e90c5d..cba2d3b 100644
@@ -1091,6 +1091,8 @@ static void balloon_up(struct
work_struct
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
*dummy)
Quoted 9 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 9 lines
drivers/hv/hv_balloon.c +2 -0
 	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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
*dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c +1 -1
 						bl_resp, alloc_unit,
 						 &alloc_error);
 
-		if ((alloc_error) && (alloc_unit != 1)) {
+		if (alloc_error && (alloc_unit != 1) &&
num_ballooned == 0)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
{
Quoted 3 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy) · line 1119
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -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
Quoted 11 lines
Yes, but all the partially-allocated 2MB memory blocks are lost(mem leak).
Quoted 2 lines
Quoted 2 lines
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:

Quoted 5 lines

                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!

Quoted 10 lines

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

Jason Wang

2014-11-24 08:47 UTC #5 lore raw

Quoted 134 lines · Dexuan Cui

Quoted 82 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 4 lines · Dexuan Cui
Quoted 4 lines
-----Original Message-----
Quoted 3 lines
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; [email protected]; linux-
[email protected];
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
[email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on
alloc_error of
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
2MB memory block
Quoted 1 line · Dexuan Cui
Quoted 1 line
If num_ballooned is not 0, we shouldn't neglect the already-
allocated
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
2MB
Quoted 9 lines
Quoted 9 lines
memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 5 lines
drivers/hv/hv_balloon.c +0 -0
index 5e90c5d..cba2d3b 100644
@@ -1091,6 +1091,8 @@ static void balloon_up(struct
work_struct
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
*dummy)
Quoted 9 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 9 lines
drivers/hv/hv_balloon.c +2 -0
 	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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
*dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c +1 -1
 						bl_resp, alloc_unit,
 						 &alloc_error);
 
-		if ((alloc_error) && (alloc_unit != 1)) {
+		if (alloc_error && (alloc_unit != 1) &&
num_ballooned == 0)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
{
Quoted 3 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy) · line 1119
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -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
Quoted 11 lines
Yes, but all the partially-allocated 2MB memory blocks are lost(mem leak).
Quoted 2 lines
Quoted 2 lines
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:
Quoted 5 lines
                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!
Quoted 10 lines
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.
(quoted diff) +4 -2
@@ -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.

Quoted 2 lines

Thanks,
-- Dexuan

Looks good.

Thanks

Dexuan Cui

author maintainer

2014-11-24 09:11 UTC #6 lore raw

Quoted 150 lines

-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 16:48 PM
To: Dexuan Cui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 4 lines · Dexuan Cui
Quoted 4 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; [email protected]; linux-
[email protected];
Quoted 4 lines
Quoted 4 lines
[email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error
of
Quoted 49 lines
Quoted 49 lines
2MB memory block
Quoted 4 lines · Dexuan Cui
Quoted 4 lines
-----Original Message-----
Quoted 3 lines
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; [email protected]; linux-
[email protected];
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
[email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on
alloc_error of
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
2MB memory block
Quoted 1 line · Dexuan Cui
Quoted 1 line
If num_ballooned is not 0, we shouldn't neglect the already-
allocated
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
2MB
Quoted 9 lines
Quoted 9 lines
memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 5 lines
drivers/hv/hv_balloon.c +0 -0
index 5e90c5d..cba2d3b 100644
@@ -1091,6 +1091,8 @@ static void balloon_up(struct
work_struct
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
*dummy)
Quoted 9 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 9 lines
drivers/hv/hv_balloon.c +2 -0
 	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
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
*dummy)
Quoted 3 lines
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -0
 						bl_resp, alloc_unit,
 						 &alloc_error);
 
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/hv/hv_balloon.c static void balloon_up(struct · line 1116
Quoted 2 lines
drivers/hv/hv_balloon.c +1 -1
-		if ((alloc_error) && (alloc_unit != 1)) {
+		if (alloc_error && (alloc_unit != 1) &&
num_ballooned == 0)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
{
Quoted 3 lines
drivers/hv/hv_balloon.c static void balloon_up(struct work_struct *dummy) · line 1119
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -0
 			alloc_unit = 1;
 			continue;
 		}
Before the change, we may retry the 4K allocation when part or all
2M
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
allocations were failed. This makes sense when memory is
fragmented.
Quoted 2 lines
Quoted 2 lines
But
Quoted 1 line
Yes, but all the partially-allocated 2MB memory blocks are lost(mem
leak).
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
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
Review static void balloon_up(struct work_struct
Quoted 37 lines
Quoted 7 lines
Quoted 3 lines
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:
Quoted 5 lines
                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!
Quoted 10 lines
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.
(quoted diff) +0 -0
@@ -1086,16 +1088,18 @@ static void balloon_up(struct work_struct
*dummy)
Quoted 20 lines
(quoted diff) +4 -2
                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;
 
 

Quoted 4 lines

Quoted 2 lines
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

KY Srinivasan

2014-11-24 21:55 UTC #7 lore raw

Quoted 168 lines

-----Original Message-----
From: Dexuan Cui
Sent: Monday, November 24, 2014 12:55 AM
To: Jason Wang; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: RE: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 129 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 16:48 PM
To: Dexuan Cui; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block
Quoted 4 lines · Dexuan Cui
Quoted 4 lines
-----Original Message-----
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 15:28 PM
To: Dexuan Cui; [email protected]; linux-
[email protected];
Quoted 5 lines
Quoted 5 lines
[email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on alloc_error
of
Quoted 52 lines
Quoted 52 lines
2MB memory block
Quoted 4 lines · Dexuan Cui
Quoted 4 lines
-----Original Message-----
Quoted 3 lines
From: Jason Wang [mailto:[email protected]]
Sent: Monday, November 24, 2014 13:18 PM
To: Dexuan Cui; [email protected]; linux-
[email protected];
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
[email protected]; [email protected]; [email protected]; KY Srinivasan
Cc: Haiyang Zhang
Subject: Re: [PATCH] hv: hv_balloon: avoid memory leak on
alloc_error of
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
2MB memory block
Quoted 1 line · Dexuan Cui
Quoted 1 line
If num_ballooned is not 0, we shouldn't neglect the already-
allocated
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
2MB
Quoted 9 lines
Quoted 9 lines
memory block(s).
Cc K. Y. Srinivasan <[email protected]>
Cc <[email protected]>
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/hv_balloon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 5 lines
drivers/hv/hv_balloon.c +0 -0
index 5e90c5d..cba2d3b 100644
@@ -1091,6 +1091,8 @@ static void balloon_up(struct
work_struct
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
*dummy)
Quoted 9 lines
drivers/hv/hv_balloon.c static void balloon_up(struct
Quoted 9 lines
drivers/hv/hv_balloon.c +2 -1
 	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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
*dummy)
Quoted 5 lines
drivers/hv/hv_balloon.c
Quoted 5 lines
drivers/hv/hv_balloon.c +1 -1
 						bl_resp, alloc_unit,
 						 &alloc_error);
 
-		if ((alloc_error) && (alloc_unit != 1)) {
+		if (alloc_error && (alloc_unit != 1) &&
num_ballooned == 0)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
{
Quoted 3 lines
drivers/hv/hv_balloon.c
Quoted 3 lines
drivers/hv/hv_balloon.c +0 -0
 			alloc_unit = 1;
 			continue;
 		}
Before the change, we may retry the 4K allocation when part or all
2M
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
allocations were failed. This makes sense when memory is
fragmented.
Quoted 3 lines
Quoted 3 lines
But
Quoted 2 lines
Yes, but all the partially-allocated 2MB memory blocks are lost(mem
leak).
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
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
Quoted 33 lines
Quoted 7 lines
Quoted 3 lines
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:
Quoted 6 lines
                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!
Quoted 10 lines
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;
Quoted 9 lines
Review static void balloon_up(struct work_struct
Quoted 5 lines
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.
(quoted diff) +0 -0
@@ -1086,16 +1088,18 @@ static void balloon_up(struct work_struct
*dummy)
Quoted 3 lines
(quoted diff) +1 -0
                num_pages -= num_ballooned;
+               alloc_error = false;
                num_ballooned = alloc_balloon_pages(&dm_device,
num_pages,
Quoted 17 lines
Quoted 17 lines
(quoted diff) +3 -2
                                                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;
 
 

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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

Quoted 5 lines

I'll send the v2 thereafter.
-- Dexuan