Re: [PATCH v2] Use ballooned pages for gntdev

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

On 03/10/2011 11:22 AM, Ian Campbell wrote:
> On Thu, 2011-03-10 at 16:10 +0000, Daniel De Graaf wrote:
>> 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.
> 
> I must be missing a patch, on my version this is exactly the loop with
> the BUG_ON in it that I was referring to:
> 	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);
> 	}
> 
> Ian
> 

Either way, increase_reservation is only called from balloon_process, with
nr_pages = current_target() - balloon_stats.current_pages, which is at
most balloon_stats.balloon_low + balloon_stats.balloon_high due to how
current_target() is calculated. Since all of these calculations are done
under the balloon mutex, it's impossible to trigger the BUG_ON in your
version or the break in konrad's #devel/balloon-hotplug branch.

-- 
Daniel De Graaf
National Security Agency