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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 26 Jan 2015, David Vrabel wrote:
> From: Jennifer 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: Jennifer 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 89dcca4..17972fb 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>
> @@ -819,6 +820,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);
> +}
> +
> +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);

I think that the +1 here is unnecessary.

Regardess:

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>



> +                     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
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
> 

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