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

[annie li <annie.li@oracle.com>]

On 2014/1/15 23:13, David Vrabel wrote:
> 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.

Part of it implements the 1 above, if gnttab_end_foreign_access_ref 
fails and then use gnttab_end_foreign_access to end the grant. Another 
is splitting __free_page from gnttab_end_foreign_access. This is 
improvement for current grant end access, and all drivers that involve 
gnttab_end_foreign_access needs to do corresponding change.
I think it can be a separate patch from my clean up patch which fixes 
grant leaking in netfront.

Thanks
Annie
>
> 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);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html