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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
>> ---
>>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel