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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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

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