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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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


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