xen-blkback: fix memory leak when persistent grants are used

PATCH In review Matt Wilson 5 participants 15 messages Jan 23, 2014 – Jan 27, 2014

Subsystems (2)

Block layer Xen block

lore

Share & tools

lore

Matt Wilson

author

2014-01-23 19:28 UTC #0 lore raw

PATCH 1/1 xen-blkback: fix memory leak when persistent grants are used

From: Matt Rushton <[email protected]>

Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc "Roger Pau Monné" <[email protected]>

Cc Ian Campbell <[email protected]>

Cc David Vrabel <[email protected]>

Cc [email protected]

Cc Anthony Liguori <[email protected]>

Signed-off-by Matt Rushton <[email protected]>

Signed-off-by Matt Wilson <[email protected]>

---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/block/xen-blkback/blkback.c +3 -3

index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);

Signature · 1 line

1.7.9.5

Ian Campbell

2014-01-24 09:21 UTC #1 lore raw

Quoted 6 lines · Matt Wilson

From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.

Quoted 28 lines

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641

Quoted 9 lines

drivers/block/xen-blkback/blkback.c +3 -0

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2014-01-24 09:21 UTC #2 lore raw

In reply to Matt Wilson

Quoted 6 lines · Matt Wilson

From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.

Quoted 37 lines

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc "Roger Pau Monné" <[email protected]>

Cc Ian Campbell <[email protected]>

Cc David Vrabel <[email protected]>

Cc [email protected]

Cc Anthony Liguori <[email protected]>

Signed-off-by Matt Rushton <[email protected]>

Signed-off-by Matt Wilson <[email protected]>

---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/block/xen-blkback/blkback.c +3 -3

index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);

Ian Campbell

2014-01-24 15:36 UTC #3 lore raw

Quoted 10 lines · Ian Campbell

Quoted 6 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.
This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.

Results are a bit inconclusive unfortunately, it seems like I am seeing some other leak too (or instead).

Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

I don't think that quite qualifies for a Tested-by though, sorry.

Ian.

Quoted 32 lines

Quoted 32 lines
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 

Quoted 13 lines

drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641
Quoted 6 lines
drivers/block/xen-blkback/blkback.c +3 -0
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2014-01-24 15:36 UTC #4 lore raw

In reply to Ian Campbell

Quoted 10 lines · Ian Campbell

Quoted 6 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.
This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.

Results are a bit inconclusive unfortunately, it seems like I am seeing some other leak too (or instead).

Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

I don't think that quite qualifies for a Tested-by though, sorry.

Ian.

Quoted 45 lines

Quoted 29 lines
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641
Quoted 9 lines
drivers/block/xen-blkback/blkback.c +3 -0
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Matt Wilson

author

2014-01-25 21:07 UTC #5 lore raw

Quoted 12 lines · Ian Campbell

Quoted 10 lines · Ian Campbell
Quoted 6 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.
This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.

Quoted 6 lines

Results are a bit inconclusive unfortunately, it seems like I am seeing some other leak too (or instead).
Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

Testing here looks good. I don't know if perhaps something else is going on with ARM...

Quoted 1 line

I don't think that quite qualifies for a Tested-by though, sorry.

How about an Acked-by? ;-)

--msw

Quoted 49 lines

Ian.
Quoted 45 lines
Quoted 29 lines
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641
Quoted 9 lines
drivers/block/xen-blkback/blkback.c +3 -0
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Matt Wilson

author

2014-01-25 21:07 UTC #6 lore raw

In reply to Ian Campbell

Quoted 12 lines · Ian Campbell

Quoted 10 lines · Ian Campbell
Quoted 6 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.
This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.

Quoted 6 lines

Results are a bit inconclusive unfortunately, it seems like I am seeing some other leak too (or instead).
Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

Testing here looks good. I don't know if perhaps something else is going on with ARM...

Quoted 1 line

I don't think that quite qualifies for a Tested-by though, sorry.

How about an Acked-by? ;-)

--msw

Quoted 49 lines

Ian.
Quoted 45 lines
Quoted 29 lines
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641
Quoted 9 lines
drivers/block/xen-blkback/blkback.c +3 -0
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Konrad Rzeszutek Wilk

2014-01-26 18:01 UTC #7 lore raw

Quoted 24 lines · Matt Wilson

Quoted 5 lines · Ian Campbell
Quoted 4 lines · Ian Campbell
Quoted 3 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used
for
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants
is
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
torn down.
This may well be the explanation for the memory leak I was
observing on
Quoted 3 lines
Quoted 1 line
ARM last night. I'll give it a go and let you know.
Results are a bit inconclusive unfortunately, it seems like I am
seeing
Quoted 5 lines
some other leak too (or instead).
Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

Quoted 8 lines

Testing here looks good. I don't know if perhaps something else is going on with ARM...
Quoted 1 line
I don't think that quite qualifies for a Tested-by though, sorry.
How about an Acked-by? ;-)
--msw

How big and often does this leak occur - that this patch fixes?

I think there is one comment from Roger that needs to be addressed? Maybe I missed the resolution?

Thanks!

Konrad Rzeszutek Wilk

2014-01-26 18:01 UTC #8 lore raw

In reply to Matt Wilson

Quoted 24 lines · Matt Wilson

Quoted 5 lines · Ian Campbell
Quoted 4 lines · Ian Campbell
Quoted 3 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used
for
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants
is
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
torn down.
This may well be the explanation for the memory leak I was
observing on
Quoted 3 lines
Quoted 1 line
ARM last night. I'll give it a go and let you know.
Results are a bit inconclusive unfortunately, it seems like I am
seeing
Quoted 5 lines
some other leak too (or instead).
Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

Quoted 8 lines

Testing here looks good. I don't know if perhaps something else is going on with ARM...
Quoted 1 line
I don't think that quite qualifies for a Tested-by though, sorry.
How about an Acked-by? ;-)
--msw

How big and often does this leak occur - that this patch fixes?

I think there is one comment from Roger that needs to be addressed? Maybe I missed the resolution?

Thanks!

Ian Campbell

2014-01-27 10:08 UTC #9 lore raw

In reply to Matt Wilson

Quoted 20 lines · Matt Wilson

Quoted 12 lines · Ian Campbell
Quoted 10 lines · Ian Campbell
Quoted 6 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.
This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.
Quoted 6 lines
Results are a bit inconclusive unfortunately, it seems like I am seeing some other leak too (or instead).
Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

Quoted 6 lines

Testing here looks good. I don't know if perhaps something else is going on with ARM...
Quoted 1 line
I don't think that quite qualifies for a Tested-by though, sorry.
How about an Acked-by? ;-)

I'm not at all familiar with the modern blkback code base so I'm afraid it would be a pretty hollow Ack.

Quoted 52 lines

--msw
Quoted 49 lines
Ian.
Quoted 45 lines
Quoted 29 lines
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641
Quoted 9 lines
drivers/block/xen-blkback/blkback.c +3 -0
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2014-01-27 10:08 UTC #10 lore raw

In reply to Matt Wilson

Quoted 20 lines · Matt Wilson

Quoted 12 lines · Ian Campbell
Quoted 10 lines · Ian Campbell
Quoted 6 lines · Matt Wilson
From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.
This may well be the explanation for the memory leak I was observing on ARM last night. I'll give it a go and let you know.
Quoted 6 lines
Results are a bit inconclusive unfortunately, it seems like I am seeing some other leak too (or instead).
Totally unscientifically it does seem to be leaking more slowly than before, so perhaps this patch has helped, but nothing conclusive I'm afraid.

Quoted 6 lines

Testing here looks good. I don't know if perhaps something else is going on with ARM...
Quoted 1 line
I don't think that quite qualifies for a Tested-by though, sorry.
How about an Acked-by? ;-)

I'm not at all familiar with the modern blkback code base so I'm afraid it would be a pretty hollow Ack.

Quoted 52 lines

--msw
Quoted 49 lines
Ian.
Quoted 45 lines
Quoted 29 lines
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641
Quoted 9 lines
drivers/block/xen-blkback/blkback.c +3 -0
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Roger Pau Monné

2014-01-24 10:39 UTC #11 lore raw

In reply to Matt Wilson

Quoted 6 lines · Matt Wilson

From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

Good catch.

Quoted 28 lines

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 634

Quoted 2 lines

drivers/block/xen-blkback/blkback.c +0 -0

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;

I think we should also add:

flush_work(&blkif->persistent_purge_work);

Here in case there's some pending purge work going on, which could also add pages to the free list after we have cleaned it.

drivers/block/xen-blkback/blkback.c purge_gnt_list:

Quoted 7 lines

drivers/block/xen-blkback/blkback.c +3 -0

+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Roger Pau Monné

2014-01-24 10:39 UTC #12 lore raw

In reply to Matt Wilson

Quoted 6 lines · Matt Wilson

From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

Good catch.

Quoted 28 lines

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 634

Quoted 2 lines

drivers/block/xen-blkback/blkback.c +0 -0

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;

I think we should also add:

flush_work(&blkif->persistent_purge_work);

Here in case there's some pending purge work going on, which could also add pages to the free list after we have cleaned it.

Quoted 7 lines

drivers/block/xen-blkback/blkback.c +3 -0

+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);

Egger, Christoph

2014-01-24 13:12 UTC #13 lore raw

In reply to Matt Wilson

Quoted 6 lines · Matt Wilson

From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

This memory leak was introduced with commit c6cc142dac52e62e1e8a2aff5de1300202b96c66 xen-blkback: use balloon pages for all mappings

Christoph

Quoted 29 lines

Cc Konrad Rzeszutek Wilk <[email protected]>
Cc "Roger Pau Monné" <[email protected]>
Cc Ian Campbell <[email protected]>
Cc David Vrabel <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Anthony Liguori <[email protected]>
Signed-off-by Matt Rushton <[email protected]>
Signed-off-by Matt Wilson <[email protected]>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/xen-blkback/blkback.c +0 -3
index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

drivers/block/xen-blkback/blkback.c purge_gnt_list: · line 641

Quoted 9 lines

drivers/block/xen-blkback/blkback.c +3 -0

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Egger, Christoph

2014-01-24 13:13 UTC #14 lore raw

In reply to Matt Wilson

Quoted 6 lines · Matt Wilson

From: Matt Rushton <[email protected]>
Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down.

This memory leak was introduced with commit c6cc142dac52e62e1e8a2aff5de1300202b96c66 xen-blkback: use balloon pages for all mappings

Christoph

Quoted 38 lines

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc "Roger Pau Monné" <[email protected]>

Cc Ian Campbell <[email protected]>

Cc David Vrabel <[email protected]>

Cc [email protected]

Cc Anthony Liguori <[email protected]>

Signed-off-by Matt Rushton <[email protected]>

Signed-off-by Matt Wilson <[email protected]>

---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/block/xen-blkback/blkback.c +3 -3

index 6620b73..30ef7b3 100644
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,

@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);