Roger Pau Monne
2013-Feb-14 10:12 UTC
[PATCH] xen-blkback: use balloon pages for persistent grants
With current persistent grants implementation we are not freeing the
persistent grants after we disconnect the device. Since grant map
operations change the mfn of the allocated page, and we can no longer
pass it to __free_page without setting the mfn to a sane value, use
balloon grant pages instead, as the gntdev device does.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: xen-devel@lists.xen.org
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
drivers/block/xen-blkback/blkback.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
index c46824f..e6c2f6a 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -46,6 +46,7 @@
#include <xen/xen.h>
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
+#include <xen/balloon.h>
#include "common.h"
/*
@@ -239,6 +240,7 @@ static void free_persistent_gnts(struct rb_root *root,
unsigned int num)
ret = gnttab_unmap_refs(unmap, NULL, pages,
segs_to_unmap);
BUG_ON(ret);
+ free_xenballooned_pages(segs_to_unmap, pages);
segs_to_unmap = 0;
}
@@ -527,8 +529,8 @@ static int xen_blkbk_map(struct blkif_request *req,
GFP_KERNEL);
if (!persistent_gnt)
return -ENOMEM;
- persistent_gnt->page = alloc_page(GFP_KERNEL);
- if (!persistent_gnt->page) {
+ if (alloc_xenballooned_pages(1, &persistent_gnt->page,
+ false)) {
kfree(persistent_gnt);
return -ENOMEM;
}
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monné
2013-Feb-14 10:56 UTC
Re: [PATCH] xen-blkback: use balloon pages for persistent grants
On 14/02/13 11:12, Roger Pau Monne wrote:> With current persistent grants implementation we are not freeing the > persistent grants after we disconnect the device. Since grant map > operations change the mfn of the allocated page, and we can no longer > pass it to __free_page without setting the mfn to a sane value, use > balloon grant pages instead, as the gntdev device does. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Cc: xen-devel@lists.xen.org > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>I think this patch is missing the following change in Kconfig, but gntdev doesn't depend on the balloon driver, which it also uses, so I'm not sure. --- diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig index f529407..74e2415 100644 --- a/drivers/block/Kconfig +++ b/drivers/block/Kconfig @@ -488,7 +488,7 @@ config XEN_BLKDEV_FRONTEND config XEN_BLKDEV_BACKEND tristate "Xen block-device backend driver" - depends on XEN_BACKEND + depends on XEN_BACKEND && XEN_BALLOON help The block-device backend driver allows the kernel to export its block devices to other guests via a high-performance shared-memory _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2013-Feb-14 13:23 UTC
Re: [PATCH] xen-blkback: use balloon pages for persistent grants
On Thu, Feb 14, 2013 at 11:12:09AM +0100, Roger Pau Monne wrote:> With current persistent grants implementation we are not freeing the > persistent grants after we disconnect the device. Since grant mapCan you explain this in more details please? Isn''t gnttab_set_unmap_op in free_persistent_gnts doing the right job of putting in the right mfn in? And then we could free the page?> operations change the mfn of the allocated page, and we can no longer > pass it to __free_page without setting the mfn to a sane value, use > balloon grant pages instead, as the gntdev device does.Wow. I did not realize that we leaving such a huge memory leak behind! But I guess that was never an issue as we would recycle those persistent grants to other domains.> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Cc: xen-devel@lists.xen.org > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/block/xen-blkback/blkback.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c > index c46824f..e6c2f6a 100644 > --- a/drivers/block/xen-blkback/blkback.c > +++ b/drivers/block/xen-blkback/blkback.c > @@ -46,6 +46,7 @@ > #include <xen/xen.h> > #include <asm/xen/hypervisor.h> > #include <asm/xen/hypercall.h> > +#include <xen/balloon.h> > #include "common.h" > > /* > @@ -239,6 +240,7 @@ static void free_persistent_gnts(struct rb_root *root, unsigned int num) > ret = gnttab_unmap_refs(unmap, NULL, pages, > segs_to_unmap); > BUG_ON(ret); > + free_xenballooned_pages(segs_to_unmap, pages); > segs_to_unmap = 0; > } > > @@ -527,8 +529,8 @@ static int xen_blkbk_map(struct blkif_request *req, > GFP_KERNEL); > if (!persistent_gnt) > return -ENOMEM; > - persistent_gnt->page = alloc_page(GFP_KERNEL); > - if (!persistent_gnt->page) { > + if (alloc_xenballooned_pages(1, &persistent_gnt->page, > + false)) { > kfree(persistent_gnt); > return -ENOMEM; > } > -- > 1.7.7.5 (Apple Git-26) >
Roger Pau Monné
2013-Feb-14 15:02 UTC
Re: [PATCH] xen-blkback: use balloon pages for persistent grants
On 14/02/13 14:23, Konrad Rzeszutek Wilk wrote:> On Thu, Feb 14, 2013 at 11:12:09AM +0100, Roger Pau Monne wrote: >> With current persistent grants implementation we are not freeing the >> persistent grants after we disconnect the device. Since grant map > > Can you explain this in more details please? Isn''t gnttab_set_unmap_op > in free_persistent_gnts doing the right job of putting in the right > mfn in? And then we could free the page?No, after gnttab_unmap_refs the page still points to the grant frame mfn. All the users of grant frames either have an internal buffer of pages that are reused (like blkback), or use balloon pages (like gntdev). I could probably modify gnttab_map to store the mfn and set it back at gnttab_unmap, but that will surely require more work than this simple fix.> > >> operations change the mfn of the allocated page, and we can no longer >> pass it to __free_page without setting the mfn to a sane value, use >> balloon grant pages instead, as the gntdev device does. > > > Wow. I did not realize that we leaving such a huge memory leak behind! > But I guess that was never an issue as we would recycle those persistent > grants to other domains.No, we didn''t recycle them AFAIK, we allocated them using alloc_page, passed them to gnttab_map, used them, and when closing the backend we only unmapped them, but they where never freed.>> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> Cc: xen-devel@lists.xen.org >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> --- >> drivers/block/xen-blkback/blkback.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c >> index c46824f..e6c2f6a 100644 >> --- a/drivers/block/xen-blkback/blkback.c >> +++ b/drivers/block/xen-blkback/blkback.c >> @@ -46,6 +46,7 @@ >> #include <xen/xen.h> >> #include <asm/xen/hypervisor.h> >> #include <asm/xen/hypercall.h> >> +#include <xen/balloon.h> >> #include "common.h" >> >> /* >> @@ -239,6 +240,7 @@ static void free_persistent_gnts(struct rb_root *root, unsigned int num) >> ret = gnttab_unmap_refs(unmap, NULL, pages, >> segs_to_unmap); >> BUG_ON(ret); >> + free_xenballooned_pages(segs_to_unmap, pages); >> segs_to_unmap = 0; >> } >> >> @@ -527,8 +529,8 @@ static int xen_blkbk_map(struct blkif_request *req, >> GFP_KERNEL); >> if (!persistent_gnt) >> return -ENOMEM; >> - persistent_gnt->page = alloc_page(GFP_KERNEL); >> - if (!persistent_gnt->page) { >> + if (alloc_xenballooned_pages(1, &persistent_gnt->page, >> + false)) { >> kfree(persistent_gnt); >> return -ENOMEM; >> } >> -- >> 1.7.7.5 (Apple Git-26) >>
Konrad Rzeszutek Wilk
2013-Feb-15 02:52 UTC
Re: [PATCH] xen-blkback: use balloon pages for persistent grants
On Thu, Feb 14, 2013 at 04:02:43PM +0100, Roger Pau Monné wrote:> On 14/02/13 14:23, Konrad Rzeszutek Wilk wrote: > > On Thu, Feb 14, 2013 at 11:12:09AM +0100, Roger Pau Monne wrote: > >> With current persistent grants implementation we are not freeing the > >> persistent grants after we disconnect the device. Since grant map > > > > Can you explain this in more details please? Isn''t gnttab_set_unmap_op > > in free_persistent_gnts doing the right job of putting in the right > > mfn in? And then we could free the page? > > No, after gnttab_unmap_refs the page still points to the grant frame > mfn. All the users of grant frames either have an internal buffer of > pages that are reused (like blkback), or use balloon pages (like gntdev). > > I could probably modify gnttab_map to store the mfn and set it back at > gnttab_unmap, but that will surely require more work than this simple fix.Simple fix is what we want right now.> > > > > > >> operations change the mfn of the allocated page, and we can no longer > >> pass it to __free_page without setting the mfn to a sane value, use > >> balloon grant pages instead, as the gntdev device does. > > > > > > Wow. I did not realize that we leaving such a huge memory leak behind! > > But I guess that was never an issue as we would recycle those persistent > > grants to other domains. > > No, we didn''t recycle them AFAIK, we allocated them using alloc_page, > passed them to gnttab_map, used them, and when closing the backend we > only unmapped them, but they where never freed.Ghastly!