Re: [PATCH 2/3] xen-balloon: Add interface to retrieve ballooned pages

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 03/08/2011 05:13 AM, Ian Campbell wrote:
> On Mon, 2011-03-07 at 18:06 +0000, Daniel De Graaf wrote:
>> Pages that have been ballooned are useful for other Xen drivers doing
>> grant table actions, because these pages have valid struct page/PFNs but
>> have no valid MFN so are available for remapping.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  drivers/xen/balloon.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/xen/balloon.h |    3 ++
>>  2 files changed, 57 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index b0a7a92..be53596 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -328,6 +328,60 @@ void balloon_set_new_target(unsigned long target)
>>  }
>>  EXPORT_SYMBOL_GPL(balloon_set_new_target);
>>  
>> +/**
>> + * get_ballooned_pages - get pages that have been ballooned out
> 
> Since this is exported it should probably have "xen" somewhere in the
> name.
> 
> A "get"/"put" naming scheme usually implies some sort of reference count
> manipulation when used in the kernel. "alloc"/"free" might be better
> here? Or maybe "take"/"return"? (I don't really like that one)

I think "alloc_xenballooned_pages" and "free_xenballooned_pages" would be
good names; they do avoid confusion with the get/put convention which I
hadn't considered, and also imply the potential alloc/free of memory if
the balloon is not inflated.

>> + * @nr_pages: Number of pages to get
>> + * @pages: pages returned
>> + * @force: Try to balloon out more pages if needed
> 
> Is there any case where this isn't passed in as true?

No; it's probably best to remove it. The name change to "alloc" implies that
it will touch allocation which is the use I had thought of for force==0.

>> + * @return number of pages retrieved
>> + */
>> +int get_ballooned_pages(int nr_pages, struct page** pages, int force)
>> +{
>> +	int rv = 0;
>> +	struct page* page;
>> +	mutex_lock(&balloon_mutex);
>> +	/* Pages are pulled off the back of the queue to prefer highmem */
>> +	while (rv < nr_pages) {
>> +		if (list_empty(&ballooned_pages)) {
>> +			if (!force)
>> +				break;
>> +			if (decrease_reservation(nr_pages - rv))
>> +				force = 0;
>> +		} else {
>> +			page = list_entry(ballooned_pages.prev,
>> +				struct page, lru);
>> +			list_del(&page->lru);
>> +			pages[rv++] = page;
>> +		}
>> +	}
>> +	mutex_unlock(&balloon_mutex);
>> +	return rv;
>> +}
>> +EXPORT_SYMBOL(get_ballooned_pages);
>> +
>> +/**
>> + * put_ballooned_pages - return pages retrieved with get_ballooned_pages
>> + * @nr_pages: Number of pages
>> + * @pages: pages to return
>> + */
>> +void put_ballooned_pages(int nr_pages, struct page** pages)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&balloon_mutex);
>> +
>> +	for (i = 0; i < nr_pages; i++) {
>> +		if (PageHighMem(pages[i])) {
>> +			list_add_tail(&pages[i]->lru, &ballooned_pages);
>> +		} else {
>> +			list_add(&pages[i]->lru, &ballooned_pages);
>> +		}
>> +	}
> 
> Maybe we should kick the balloon worker thread here if current < target
> or some such? e.g. to reverse the effect of a force==1 in the getter.

Kicking the worker thread is a good idea: if the balloon module is not loaded,
the pages will never be returned to the kernel allocator (although they will
be reused for later balloon requests).

>> +
>> +	mutex_unlock(&balloon_mutex);
>> +}
>> +EXPORT_SYMBOL(put_ballooned_pages);
>> +
>>  static int __init balloon_init(void)
>>  {
>>   	unsigned long pfn, nr_pages, extra_pfn_end;
>> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
>> index b2b7c21..5fc25fa 100644
>> --- a/include/xen/balloon.h
>> +++ b/include/xen/balloon.h
>> @@ -19,3 +19,6 @@ struct balloon_stats {
>>  extern struct balloon_stats balloon_stats;
>>  
>>  void balloon_set_new_target(unsigned long target);
>> +
>> +int get_ballooned_pages(int nr_pages, struct page** pages, int force);
>> +void put_ballooned_pages(int nr_pages, struct page** pages);
> 
> 


-- 
Daniel De Graaf
National Security Agency