[Xen-devel] Re: [PATCH v2] Use ballooned pages for gntdev

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 03/10/2011 10:57 AM, Ian Campbell wrote:
> On Thu, 2011-03-10 at 14:56 +0000, Daniel De Graaf wrote:
>> On 03/10/2011 04:18 AM, Ian Campbell wrote:
>>> On Wed, 2011-03-09 at 23:07 +0000, Daniel De Graaf wrote:
>>>> Split up the balloon module, similar to how grants and events are
>>>> handled. This allow gntdev to use ballooned pages without adding a
>>>> dependency on the balloon module.
>>>>
>>>> The interface is slightly different from that exposed in 2.6.32; the
>>>> page vector is allocated by the caller, not by the balloon driver. This
>>>> allows more freedom in how the returned pages are used, since they do
>>>> not all have to be returned to the balloon driver at the same time. It
>>>> also tries to reuse already ballooned pages rather than always
>>>> ballooning new pages to ensure they are in low memory as the 2.6.32
>>>> version did.
>>>>
>>>> Changes since v1:
>>>>    * Rebased on konrad/devel/{balloon-hotplug,next-2.6.38} merge
>>>>    * Better function names
>>>>    * Adjust balloon stats for requested pages
>>>>
>>>> [PATCH 1/3] xen-balloon: Move core balloon functionality out of module
>>>> [PATCH 2/3] xen-balloon: Add interface to retrieve ballooned pages
>>>> [PATCH 3/3] xen-gntdev: Use ballooned pages for grant mappings
>>>
>>> Does this introduces a new potential failure case? When a normal balloon
>>> operation takes place is it is now possible for the ballooned_pages list
>>> to be empty (because gntdev has stolen stuff from it) where before we
>>> knew the list was non-empty at that point?
>>>
>>> e.g. increase_reservation includes:
>>>             page = balloon_retrieve();
>>>             BUG_ON(page == NULL);
>>> as well as:
>>>     page = balloon_first_page();
>>>     for (i = 0; i < nr_pages; i++) {
>>>             BUG_ON(page == NULL);
>>>             frame_list[i] = page_to_pfn(page);
>>>             page = balloon_next_page(page);
>>>     }
>>>
>>> Are we sure we aren't about to exercise those BUG_ONs?
>>
>> This is safe because increase_reservation is protected by the balloon
>> mutex. There is a loop just before the hypercall that verifies that
>> there are enough pages in the list.
> 
> Which loop? I don't see anything like that in either
> increase_reservation or the caller (balloon process).
> 

In increase_reservation:
        page = balloon_first_page();
        for (i = 0; i < nr_pages; i++) {
                if (!page) {
                        nr_pages = i;
                        break;
                }
                frame_list[i] = page_to_pfn(page);
                page = balloon_next_page(page);
        }

This ensures that the balloon size is at least nr_pages.

>>> As a secondary concern, assuming we don't crash, does the balloon
>>> process correctly sleep until explicitly kicked when ballooned_pages
>>> becomes empty? Or does it keep needlessly waking up on the jiffy timer?
>>> IOW does credit in balloon_process become 0 when this situation occurs?
>>>
>>> Or does adjusting the balloon stats ensure this all works out alright?
>>
>> Adjusting the statistics should make this work out properly. Since only
>> pages actually in the list will be counted in balloon_low+balloon_high,
>> which is the lower bound for credit, we will always be able to satisfy
>> the reservation changes requested by balloon_process. The jiffy timer is
>> used only when Linux or Xen cannot satisfy the memory allocations required
>> to adjust the balloon size.
> 
> OK.
> 
>>
>>> An interesting experiment would probably be to allocate a bunch of
>>> address space via gntdev and then manually balloon the guest above it's
>>> current allocation, back below the lower limit due to the gntdev
>>> allocation, back up to starting point etc etc.
>>>
>>> Ian.
>>>
>>
>> That does sound like a useful test case.
>>
> 
> 

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