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

["Roger Pau Monné" <roger.pau@citrix.com>]

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