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: [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(-)
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 6620b73..30ef7b3 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -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);
--
1.7.9.5
On Thu, 2014-01-23 at 11:28 -0800, Matt Wilson wrote:
> 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.
> 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(-)
>
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 6620b73..30ef7b3 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -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);
>
On 23/01/14 20:28, Matt Wilson wrote:
> 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.
> 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(-)
>
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 6620b73..30ef7b3 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -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;
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.
> + /* Since we are shutting down remove all pages from the buffer */
> + shrink_free_pagepool(blkif, 0 /* All */);
> +
> if (log_stats)
> print_stats(blkif);
>
>
On 23.01.14 20:28, Matt Wilson wrote:
> 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
> 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(-)
>
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 6620b73..30ef7b3 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -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);
>
>
On Fri, 2014-01-24 at 09:21 +0000, Ian Campbell wrote:
> On Thu, 2014-01-23 at 11:28 -0800, Matt Wilson wrote:
> > 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.
>
> > 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(-)
> >
> > diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> > index 6620b73..30ef7b3 100644
> > --- a/drivers/block/xen-blkback/blkback.c
> > +++ b/drivers/block/xen-blkback/blkback.c
> > @@ -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);
> >
>
>
>
> _______________________________________________
> Xen-devel mailing list
> [email protected]
> http://lists.xen.org/xen-devel
On Fri, Jan 24, 2014 at 03:36:22PM +0000, Ian Campbell wrote:
> On Fri, 2014-01-24 at 09:21 +0000, Ian Campbell wrote:
> > On Thu, 2014-01-23 at 11:28 -0800, Matt Wilson wrote:
> > > 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.
Testing here looks good. I don't know if perhaps something else is
going on with ARM...
> I don't think that quite qualifies for a Tested-by though, sorry.
How about an Acked-by? ;-)
--msw
> Ian.
>
> >
> > > 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(-)
> > >
> > > diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> > > index 6620b73..30ef7b3 100644
> > > --- a/drivers/block/xen-blkback/blkback.c
> > > +++ b/drivers/block/xen-blkback/blkback.c
> > > @@ -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);
> > >
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > [email protected]
> > http://lists.xen.org/xen-devel
>
>
Matt Wilson <[email protected]> wrote:
>On Fri, Jan 24, 2014 at 03:36:22PM +0000, Ian Campbell wrote:
>> On Fri, 2014-01-24 at 09:21 +0000, Ian Campbell wrote:
>> > On Thu, 2014-01-23 at 11:28 -0800, Matt Wilson wrote:
>> > > 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.
>
>Testing here looks good. I don't know if perhaps something else is
>going on with ARM...
>
>> 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!
On Sat, 2014-01-25 at 13:07 -0800, Matt Wilson wrote:
> On Fri, Jan 24, 2014 at 03:36:22PM +0000, Ian Campbell wrote:
> > On Fri, 2014-01-24 at 09:21 +0000, Ian Campbell wrote:
> > > On Thu, 2014-01-23 at 11:28 -0800, Matt Wilson wrote:
> > > > 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.
>
> Testing here looks good. I don't know if perhaps something else is
> going on with ARM...
>
> > 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.
>
> --msw
>
> > Ian.
> >
> > >
> > > > 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(-)
> > > >
> > > > diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> > > > index 6620b73..30ef7b3 100644
> > > > --- a/drivers/block/xen-blkback/blkback.c
> > > > +++ b/drivers/block/xen-blkback/blkback.c
> > > > @@ -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);
> > > >
> > >
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > [email protected]
> > > http://lists.xen.org/xen-devel
> >
> >