From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Vrabel <david.vrabel@citrix.com>
Subject: Re: [PATCH v2 1/2] xen/balloon: set a mapping for
	ballooned out pages
Date: Mon, 22 Jul 2013 17:51:35 +0100
Message-ID: <51ED6317.2060707@citrix.com>
References: <alpine.DEB.2.02.1307221717160.4893@kaball.uk.xensource.com>
	<1374510529-10395-1-git-send-email-stefano.stabellini@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1374510529-10395-1-git-send-email-stefano.stabellini@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: dcrisan@flexiant.com, xen-devel@lists.xensource.com, Ian.Campbell@citrix.com, alex@alex.org.uk
List-Id: xen-devel@lists.xenproject.org

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