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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home