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

[David Vrabel <david.vrabel@citrix.com>]

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);

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