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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2013-07-22 at 17:51 +0100, David Vrabel wrote:
> On 22/07/13 17:28, Stefano Stabellini wrote:
> > Currently ballooned out pages are mapped to 0 and have INVALID_P2M_ENTRY
> > in the p2m. These ballooned out pages are used to map foreign grants
> > by gntdev and blkback (see alloc_xenballooned_pages).
> > 
> > Allocate a page per cpu and map all the ballooned out pages to the
> > corresponding mfn. Set the p2m accordingly. This way reading from a
> > ballooned out page won't cause a kernel crash (see
> > http://lists.xen.org/archives/html/xen-devel/2012-12/msg01154.html).
> 
> Generally in favour of this approach.
> 
> I can't help thinking the "trade page" doesn't really mean anything but
> I can't think of a better, more descriptive name.
> 
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -36,6 +36,7 @@
> >   * IN THE SOFTWARE.
> >   */
> >  
> > +#include <linux/cpu.h>
> >  #include <linux/kernel.h>
> >  #include <linux/sched.h>
> >  #include <linux/errno.h>
> > @@ -88,6 +89,8 @@ EXPORT_SYMBOL_GPL(balloon_stats);
> >  
> >  /* We increase/decrease in batches which fit in a page */
> >  static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
> > +static struct page** balloon_trade_pages;
> 
> static DECLARE_PER_CPU(struct page *, balloon_trade_pages);

And use this_cpu(balloon_trade_page) etc.

> >  #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?

Otherwise that would basically mean all the time since just ballooning
out a single page will cause it to be used.

> 
> > +struct page* get_balloon_trade_page(void)
> > +{
> > +   BUG_ON(balloon_trade_pages[smp_processor_id()] == NULL);
> > +   return balloon_trade_pages[smp_processor_id()];
> > +}
> 
> I think you need a get_balloon_trade_page() and put_balloon_trade_page()
> pair that call get_cpu_var() and put_cpu_var() to ensure that preemption
> is disabled while using it.
> 
> David



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