Re: [PATCH 09/14] xen/grant-table: add a mechanism to safely unmap pages that are in use

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 01/12/2015 10:43 AM, David Vrabel wrote:
> From: Jenny Herbert <jennifer.herbert@citrix.com>
>
> Introduce gnttab_unmap_refs_async() that can be used to safely unmap
> pages that may be in use (ref count > 1).  If the pages are in use the
> unmap is deferred and retried later.  This polling is not very clever
> but it should be good enough if the cases where the delay is necessary
> are rare.
>
> The initial delay is 5 ms and is increased linearly on each subsequent
> retry (to reduce load if the page is in use for a long time).
>
> This is needed to allow block backends using grant mapping to safely
> use network storage (block or filesystem based such as iSCSI or NFS).
>
> The network storage driver may complete a block request whilst there
> is a queued network packet retry (because the ack from the remote end
> races with deciding to queue the retry).  The pages for the retried
> packet would be grant unmapped and the network driver (or hardware)
> would access the unmapped page.
>
> Signed-off-by: Jenny Herbert <jennifer.herbert@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   drivers/xen/grant-table.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>   include/xen/grant_table.h |   18 ++++++++++++++++++
>   2 files changed, 62 insertions(+)
>
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 9c7dc75..d9beffc 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -42,6 +42,7 @@
>   #include <linux/io.h>
>   #include <linux/delay.h>
>   #include <linux/hardirq.h>
> +#include <linux/workqueue.h>
>   
>   #include <xen/xen.h>
>   #include <xen/interface/xen.h>
> @@ -817,6 +818,49 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>   }
>   EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
>   
> +#define GNTTAB_UNMAP_REFS_DELAY 5
> +
> +static void __gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item);
> +
> +static void gnttab_unmap_work(struct work_struct *work)
> +{
> +	struct gntab_unmap_queue_data
> +		*unmap_data = container_of(work,
> +					   struct gntab_unmap_queue_data,
> +					   gnttab_work.work);
> +	if (unmap_data->age != UINT_MAX)
> +		unmap_data->age++;
> +	__gnttab_unmap_refs_async(unmap_data);

Should there be a termination condition if pages are never (for some 
definition of "never") released? Return -ETIMEDOUT in done()? Or at 
least print a warning once?

-boris

> +}
> +
> +static void __gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
> +{
> +	int ret;
> +	int pc;
> +
> +	for (pc = 0; pc < item->count; pc++) {
> +		if (page_count(item->pages[pc]) > 1) {
> +			unsigned long delay = GNTTAB_UNMAP_REFS_DELAY * (item->age + 1);
> +			schedule_delayed_work(&item->gnttab_work,
> +					      msecs_to_jiffies(delay));
> +			return;
> +		}
> +	}
> +
> +	ret = gnttab_unmap_refs(item->unmap_ops, item->kunmap_ops,
> +				item->pages, item->count);
> +	item->done(ret, item);
> +}
> +
> +void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
> +{
> +	INIT_DELAYED_WORK(&item->gnttab_work, gnttab_unmap_work);
> +	item->age = 0;
> +
> +	__gnttab_unmap_refs_async(item);
> +}
> +EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async);
> +
>   static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
>   {
>   	int rc;
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index d3bef56..143ca5f 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -60,6 +60,22 @@ struct gnttab_free_callback {
>   	u16 count;
>   };
>   
> +struct gntab_unmap_queue_data;
> +
> +typedef void (*gnttab_unmap_refs_done)(int result, struct gntab_unmap_queue_data *data);
> +
> +struct gntab_unmap_queue_data
> +{
> +	struct delayed_work	gnttab_work;
> +	void *data;
> +	gnttab_unmap_refs_done	done;
> +	struct gnttab_unmap_grant_ref *unmap_ops;
> +	struct gnttab_unmap_grant_ref *kunmap_ops;
> +	struct page **pages;
> +	unsigned int count;
> +	unsigned int age;
> +};
> +
>   int gnttab_init(void);
>   int gnttab_suspend(void);
>   int gnttab_resume(void);
> @@ -174,6 +190,8 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>   int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>   		      struct gnttab_unmap_grant_ref *kunmap_ops,
>   		      struct page **pages, unsigned int count);
> +void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item);
> +
>   
>   /* Perform a batch of grant map/copy operations. Retry every batch slot
>    * for which the hypervisor returns GNTST_eagain. This is typically due