[PATCH] xen-blkback: use balloon pages for persistent grants

[PATCH] xen-blkback: use balloon pages for persistent grants

[Roger Pau Monne]

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)

Re: [PATCH] xen-blkback: use balloon pages for persistent grants

[Roger Pau Monné]

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

Re: [PATCH] xen-blkback: use balloon pages for persistent grants

[Konrad Rzeszutek Wilk]

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?


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

Re: [PATCH] xen-blkback: use balloon pages for persistent grants

[Roger Pau Monné]

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

Re: [PATCH] xen-blkback: use balloon pages for persistent grants

[Konrad Rzeszutek Wilk]

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!