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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/12/2015 10:43 AM, David Vrabel wrote:
> From: Jenny Herbert <jennifer.herbert@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
>   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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel