From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 09/14] xen/grant-table: add a mechanism to safely unmap pages that are in use Date: Tue, 13 Jan 2015 18:31:44 -0500 Message-ID: <54B5AAE0.1060503@oracle.com> References: <1421077417-7162-1-git-send-email-david.vrabel@citrix.com> <1421077417-7162-10-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YBAwA-0001SG-P2 for xen-devel@lists.xenproject.org; Tue, 13 Jan 2015 23:31:50 +0000 In-Reply-To: <1421077417-7162-10-git-send-email-david.vrabel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , xen-devel@lists.xenproject.org Cc: Jenny Herbert List-Id: xen-devel@lists.xenproject.org On 01/12/2015 10:43 AM, David Vrabel wrote: > From: Jenny Herbert > > 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 > Signed-off-by: David Vrabel > --- > 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 > #include > #include > +#include > > #include > #include > @@ -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