Xen devel - Sep 2013 - [PATCH] xen/balloon: don't alloc page while non-preemptible

From: David Vrabel <david.vrabel@citrix.com>

get_balloon_scratch_page() disables preemption so we cannot call
alloc_page() in between get/put_balloon_scratch_page().  Shuffle bits
around in decrease_reservation() to avoid this.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/balloon.c |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index a50c6e3..b232908 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -398,8 +398,6 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
 	if (nr_pages > ARRAY_SIZE(frame_list))
 		nr_pages = ARRAY_SIZE(frame_list);
 
-	scratch_page = get_balloon_scratch_page();
-
 	for (i = 0; i < nr_pages; i++) {
 		page = alloc_page(gfp);
 		if (page == NULL) {
@@ -413,6 +411,12 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
 
 		scrub_page(page);
 
+		/*
+		 * Ballooned out frames are effectively replaced with
+		 * a scratch frame.  Ensure direct mappings and the
+		 * p2m are consistent.
+		 */
+		scratch_page = get_balloon_scratch_page();
 #ifdef CONFIG_XEN_HAVE_PVMMU
 		if (xen_pv_domain() && !PageHighMem(page)) {
 			ret = HYPERVISOR_update_va_mapping(
@@ -422,24 +426,19 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
 			BUG_ON(ret);
 		}
 #endif
-	}
-
-	/* Ensure that ballooned highmem pages don''t have kmaps. */
-	kmap_flush_unused();
-	flush_tlb_all();
-
-	/* No more mappings: invalidate P2M and add to balloon. */
-	for (i = 0; i < nr_pages; i++) {
-		pfn = mfn_to_pfn(frame_list[i]);
 		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
 			unsigned long p;
 			p = page_to_pfn(scratch_page);
 			__set_phys_to_machine(pfn, pfn_to_mfn(p));
 		}
+		put_balloon_scratch_page();
+
 		balloon_append(pfn_to_page(pfn));
 	}
 
-	put_balloon_scratch_page();
+	/* Ensure that ballooned highmem pages don''t have kmaps. */
+	kmap_flush_unused();
+	flush_tlb_all();
 
 	set_xen_guest_handle(reservation.extent_start, frame_list);
 	reservation.nr_extents   = nr_pages;
-- 
1.7.2.5

On Thu, 19 Sep 2013, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> get_balloon_scratch_page() disables preemption so we cannot call
> alloc_page() in between get/put_balloon_scratch_page().  Shuffle bits
> around in decrease_reservation() to avoid this.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/xen/balloon.c |   23 +++++++++++------------
>  1 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index a50c6e3..b232908 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -398,8 +398,6 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
>  	if (nr_pages > ARRAY_SIZE(frame_list))
>  		nr_pages = ARRAY_SIZE(frame_list);
>  
> -	scratch_page = get_balloon_scratch_page();
> -
>  	for (i = 0; i < nr_pages; i++) {
>  		page = alloc_page(gfp);
>  		if (page == NULL) {
> @@ -413,6 +411,12 @@ static enum bp_state decrease_reservation(unsigned
long nr_pages, gfp_t gfp)
>  
>  		scrub_page(page);
>  
> +		/*
> +		 * Ballooned out frames are effectively replaced with
> +		 * a scratch frame.  Ensure direct mappings and the
> +		 * p2m are consistent.
> +		 */
> +		scratch_page = get_balloon_scratch_page();
>  #ifdef CONFIG_XEN_HAVE_PVMMU
>  		if (xen_pv_domain() && !PageHighMem(page)) {
>  			ret = HYPERVISOR_update_va_mapping(
> @@ -422,24 +426,19 @@ static enum bp_state decrease_reservation(unsigned
long nr_pages, gfp_t gfp)
>  			BUG_ON(ret);
>  		}
>  #endif
> -	}
> -
> -	/* Ensure that ballooned highmem pages don''t have kmaps. */
> -	kmap_flush_unused();
> -	flush_tlb_all();
> -
> -	/* No more mappings: invalidate P2M and add to balloon. */
> -	for (i = 0; i < nr_pages; i++) {
> -		pfn = mfn_to_pfn(frame_list[i]);
>  		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>  			unsigned long p;
>  			p = page_to_pfn(scratch_page);
>  			__set_phys_to_machine(pfn, pfn_to_mfn(p));
>  		}
> +		put_balloon_scratch_page();
> +
>  		balloon_append(pfn_to_page(pfn));
>  	}
>  
> -	put_balloon_scratch_page();
> +	/* Ensure that ballooned highmem pages don''t have kmaps. */
> +	kmap_flush_unused();
> +	flush_tlb_all();
The change with possible side effects in this patch is that
kmap_flush_unused and flush_tlb_all are now called after setting the p2m
and after calling balloon_append instead of before.

However they are still called before the XENMEM_decrease_reservation
hypercall, so I think it''s OK.

On 25/09/13 14:26, Stefano Stabellini wrote:
> On Thu, 19 Sep 2013, David Vrabel wrote:
>> 
>> -	put_balloon_scratch_page();
>> +	/* Ensure that ballooned highmem pages don''t have kmaps. */
>> +	kmap_flush_unused();
>> +	flush_tlb_all();
> 
> The change with possible side effects in this patch is that
> kmap_flush_unused and flush_tlb_all are now called after setting the p2m
> and after calling balloon_append instead of before.
> 
> However they are still called before the XENMEM_decrease_reservation
> hypercall, so I think it''s OK.
Yes, the requirement is that the kmap cache is flushed before releasing
the frame to Xen (in case it contains a kmap for a frame we''re about to
release).

I also wonder if the flush_tlb_all() is even necessary.  Surely Xen has
all the appropriate TLB flushes in the decrease_reservation hypercall.
It can''t possibly rely on the guest doing the right thing.

David

On Wed, 25 Sep 2013, David Vrabel wrote:
> On 25/09/13 14:26, Stefano Stabellini wrote:
> > On Thu, 19 Sep 2013, David Vrabel wrote:
> >> 
> >> -	put_balloon_scratch_page();
> >> +	/* Ensure that ballooned highmem pages don''t have
kmaps. */
> >> +	kmap_flush_unused();
> >> +	flush_tlb_all();
> > 
> > The change with possible side effects in this patch is that
> > kmap_flush_unused and flush_tlb_all are now called after setting the
p2m
> > and after calling balloon_append instead of before.
> > 
> > However they are still called before the XENMEM_decrease_reservation
> > hypercall, so I think it''s OK.
> 
> Yes, the requirement is that the kmap cache is flushed before releasing
> the frame to Xen (in case it contains a kmap for a frame we''re
about to
> release).
> 
> I also wonder if the flush_tlb_all() is even necessary.  Surely Xen has
> all the appropriate TLB flushes in the decrease_reservation hypercall.
> It can''t possibly rely on the guest doing the right thing.
 
That would only be the case for PV guests, but yes, Xen has certainly a
flush.