Re: [Xen-devel] [PATCH net-next] xen-netfront: clean up code in xennet_release_rx_bufs

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

On 15/01/14 14:17, annie li wrote:
> 
> I am thinking of two ways, and they can be implemented in new patches.
> 1. If gnttab_end_foreign_access_ref succeeds, then kfree_skb is called
> to free skb. Otherwise, using gnttab_end_foreign_access to release ref
> and pages.
> 2. Add a similar deferred way of gnttab_end_foreign_access in
> gnttab_end_foreign_access_ref.

Something like the following (untested!) patch is what I'm thinking of.

Fixups to existing drivers (except netfront) are included but may not be
quite correct.

8<----------
>From 76c254c8e020f4427e8f37c867622f0bfd5ac85f Mon Sep 17 00:00:00 2001
From: David Vrabel <david.vrabel@citrix.com>
Date: Wed, 15 Jan 2014 15:04:52 +0000
Subject: [PATCH] HACK! xen/gnttab: make gnttab_end_foreign_access() more
useful

This is UNTESTED and is an example of the sort of change I'm looking
for.

Freeing the page in gnttab_end_foreign_access() means it cannot be
used where the pages are freed in some other way (e.g., as part of a
kfree_skb()).

Leave the freeing of the page to the caller.  If the page still has
foreign users, take an additional reference before adding it to the
deferred list.

Hack all the users of the call to do something resembling the right
thing.  Not quite sure on the blkfront changes.
---
 drivers/block/xen-blkfront.c    |   22 +++++++++++++---------
 drivers/char/tpm/xen-tpmfront.c |    3 +--
 drivers/pci/xen-pcifront.c      |    3 +--
 drivers/xen/grant-table.c       |   19 +++++++++++--------
 include/xen/grant_table.h       |   11 ++++++-----
 5 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index aa846a4..a586496 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -504,7 +504,7 @@ static void gnttab_handle_deferred(unsigned long unused)
 			if (entry->page) {
 				pr_debug("freeing g.e. %#x (pfn %#lx)\n",
 					 entry->ref, page_to_pfn(entry->page));
-				__free_page(entry->page);
+				put_page(entry->page);
 			} else
 				pr_info("freeing g.e. %#x\n", entry->ref);
 			kfree(entry);
@@ -555,15 +555,18 @@ static void gnttab_add_deferred(grant_ref_t ref,
bool readonly,
 }

 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
-			       unsigned long page)
+			       struct page *page)
 {
-	if (gnttab_end_foreign_access_ref(ref, readonly)) {
+	if (gnttab_end_foreign_access_ref(ref, readonly))
 		put_free_entry(ref);
-		if (page != 0)
-			free_page(page);
-	} else
-		gnttab_add_deferred(ref, readonly,
-				    page ? virt_to_page(page) : NULL);
+	else {
+		/*
+		 * Take a reference to the page so it's not freed
+		 * while the foreign domain still has access to it.
+		 */
+		get_page(page);
+		gnttab_add_deferred(ref, readonly, page);
+	}
 }
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access);

diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 694dcaf..ffa3ce6 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -91,13 +91,14 @@ bool gnttab_trans_grants_available(void);
 int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly);

 /*
- * Eventually end access through the given grant reference, and once that
- * access has been ended, free the given page too.  Access will be ended
- * immediately iff the grant entry is not in use, otherwise it will happen
- * some time later.  page may be 0, in which case no freeing will occur.
+ * Eventually end access through the given grant reference, if the
+ * page is still in use an additional reference is taken and released
+ * when access has ended.  Access will be ended immediately iff the
+ * grant entry is not in use, otherwise it will happen some time
+ * later.
  */
 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
-			       unsigned long page);
+			       struct page *page);

 int gnttab_grant_foreign_transfer(domid_t domid, unsigned long pfn);

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index c4a4c90..45a2a01 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -931,14 +931,16 @@ static void blkif_free(struct blkfront_info *info,
int suspend)
 	if (!list_empty(&info->grants)) {
 		list_for_each_entry_safe(persistent_gnt, n,
 		                         &info->grants, node) {
+			struct page *page = pfn_to_page(persistent_gnt->pfn);
+
 			list_del(&persistent_gnt->node);
 			if (persistent_gnt->gref != GRANT_INVALID_REF) {
 				gnttab_end_foreign_access(persistent_gnt->gref,
-				                          0, 0UL);
+				                          0, page);
 				info->persistent_gnts_c--;
 			}
 			if (info->feature_persistent)
-				__free_page(pfn_to_page(persistent_gnt->pfn));
+				__free_page(page);
 			kfree(persistent_gnt);
 		}
 	}
@@ -970,10 +972,13 @@ static void blkif_free(struct blkfront_info *info,
int suspend)
 		       info->shadow[i].req.u.indirect.nr_segments :
 		       info->shadow[i].req.u.rw.nr_segments;
 		for (j = 0; j < segs; j++) {
+			struct page *page;
+
 			persistent_gnt = info->shadow[i].grants_used[j];
-			gnttab_end_foreign_access(persistent_gnt->gref, 0, 0UL);
+			page = pfn_to_page(persistent_gnt->pfn);
+			gnttab_end_foreign_access(persistent_gnt->gref, 0, page);
 			if (info->feature_persistent)
-				__free_page(pfn_to_page(persistent_gnt->pfn));
+				__free_page(page);
 			kfree(persistent_gnt);
 		}

@@ -1010,10 +1015,11 @@ free_shadow:
 	/* Free resources associated with old device channel. */
 	if (info->ring_ref != GRANT_INVALID_REF) {
 		gnttab_end_foreign_access(info->ring_ref, 0,
-					  (unsigned long)info->ring.sring);
+					  virt_to_page(info->ring.sring));
 		info->ring_ref = GRANT_INVALID_REF;
 		info->ring.sring = NULL;
 	}
+	free_page((unsigned long)info->ring.sring);
 	if (info->irq)
 		unbind_from_irqhandler(info->irq, info);
 	info->evtchn = info->irq = 0;
@@ -1053,7 +1059,7 @@ static void blkif_completion(struct blk_shadow *s,
struct blkfront_info *info,
 	}
 	/* Add the persistent grant into the list of free grants */
 	for (i = 0; i < nseg; i++) {
-		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
+		if (gnttab_end_foreign_access_ref(s->grants_used[i]->gref, 0)) {
 			/*
 			 * If the grant is still mapped by the backend (the
 			 * backend has chosen to make this grant persistent)
@@ -1072,14 +1078,13 @@ static void blkif_completion(struct blk_shadow
*s, struct blkfront_info *info,
 			 * so it will not be picked again unless we run out of
 			 * persistent grants.
 			 */
-			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
 			s->grants_used[i]->gref = GRANT_INVALID_REF;
 			list_add_tail(&s->grants_used[i]->node, &info->grants);
 		}
 	}
 	if (s->req.operation == BLKIF_OP_INDIRECT) {
 		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
-			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
+			if (gnttab_end_foreign_access_ref(s->indirect_grants[i]->gref, 0)) {
 				if (!info->feature_persistent)
 					pr_alert_ratelimited("backed has not unmapped grant: %u\n",
 							     s->indirect_grants[i]->gref);
@@ -1088,7 +1093,6 @@ static void blkif_completion(struct blk_shadow *s,
struct blkfront_info *info,
 			} else {
 				struct page *indirect_page;

-				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
 				/*
 				 * Add the used indirect page back to the list of
 				 * available pages for indirect grefs.
diff --git a/drivers/char/tpm/xen-tpmfront.c
b/drivers/char/tpm/xen-tpmfront.c
index c8ff4df..00d1132 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -315,8 +315,7 @@ static void ring_free(struct tpm_private *priv)
 	if (priv->ring_ref)
 		gnttab_end_foreign_access(priv->ring_ref, 0,
 				(unsigned long)priv->shr);
-	else
-		free_page((unsigned long)priv->shr);
+	free_page((unsigned long)priv->shr);

 	if (priv->chip && priv->chip->vendor.irq)
 		unbind_from_irqhandler(priv->chip->vendor.irq, priv);
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index f7197a7..253a129 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -759,8 +759,7 @@ static void free_pdev(struct pcifront_device *pdev)
 	if (pdev->gnt_ref != INVALID_GRANT_REF)
 		gnttab_end_foreign_access(pdev->gnt_ref, 0 /* r/w page */,
 					  (unsigned long)pdev->sh_info);
-	else
-		free_page((unsigned long)pdev->sh_info);
+	free_page((unsigned long)pdev->sh_info);

 	dev_set_drvdata(&pdev->xdev->dev, NULL);