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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Jul 22, 2013 at 05:51:35PM +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.

scratch page?

> 
> > --- 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);
> 
> >  #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.
> 
> > +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