Re: [Xen-devel] [PATCH v2 1/2] xen/balloon: set a mapping for ballooned out pages

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2013-07-23 at 13:49 +0100, Stefano Stabellini wrote:
> On Mon, 22 Jul 2013, Ian Campbell wrote:
> > On Mon, 2013-07-22 at 18:26 +0100, David Vrabel wrote:
> > > On 22/07/13 18:22, Ian Campbell wrote:
> > > > On Mon, 2013-07-22 at 17:51 +0100, David Vrabel wrote:
> > > >> On 22/07/13 17:28, Stefano Stabellini wrote:
> > > >>>
> > > >>>  #ifdef CONFIG_HIGHMEM
> > > >>>  #define inc_totalhigh_pages() (totalhigh_pages++)
> > > >>> @@ -423,7 +426,8 @@ static enum bp_state 
> > > >>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> > > >>>               if (xen_pv_domain() && !PageHighMem(page)) {
> > > >>>                       ret = HYPERVISOR_update_va_mapping(
> > > >>>                               (unsigned long)__va(pfn << PAGE_SHIFT),
> > > >>> -                             __pte_ma(0), 0);
> > > >>> +                             
> > > >>> pfn_pte(page_to_pfn(get_balloon_trade_page()),
> > > >>> +                                     PAGE_KERNEL_RO), 0);
> > > >>
> > > >> Preemption needs to be disabled while using the trade page, see
> > > >> suggestion below.
> > > > 
> > > > Hopefully you mean just when setting up/manipulating it?
> > > 
> > > Yes, sorry.
> > > 
> > > get_...()
> > > update_va_mapping()
> > > put_...()
> > 
> > I can see why it would matter in the unmap_and_replace+fixup case (since
> > you need them to happen "atomically") but why in this case? We don't
> > actually care which of the trade pages gets used for this purpose, so
> > even if we happen to get preempted and change CPU it doesn't really
> > matter. Hrm, unless the CPU goes offline I suppose, ah but we don't free
> > the page when the cpu goes down (this is good).
> 
> I agree with Ian, I think it only matters in the m2p code.
> 
> 
> > Oh, that made me notice:
> > 
> > +       case CPU_UP_PREPARE:
> > +               balloon_trade_pages[cpu] = alloc_page(GFP_KERNEL);
> > 
> > will leak the previously allocated page if you take the CPU down then up
> > again.
>  
> That means I'll have to free the page on CPU_DOWN_PREPARE

No, don't do that. The page may still be in use in the page tables,
taking the CPU down doesn't make all uses of its trade page go away.
You'll need to add a check in CPU_UP_PREPARE to only allocate a page if
one doesn't already exist, I think.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel