[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/3] Handle GNTST_eagain in kernel drivers



On Fri, Dec 16, 2011 at 10:22:20PM -0500, Adin Scannell wrote:
> Handle GNTST_eagain status from GNTTABOP_map_grant_ref and
> GNTTABOP_copy operations properly to allow usage of xenpaging without
> causing crashes or data corruption.
> 
> Replace all relevant HYPERVISOR_grant_table_op() calls with a retry
> loop. This loop is implemented as a macro to allow different
> GNTTABOP_* args. It will sleep up to 33 seconds and wait for the
> page to be paged in again.
> 
> All ->status checks were updated to use the GNTST_* namespace. All
> return values are converted from GNTST_* namespace to 0/-EINVAL, since
> all callers did not use the actual return value.
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Acked-by: Patrick Colp <pjcolp@xxxxxxxxx>
> 
> This is a port to the mainline Linux tree.  This required dropping many 
> backend
> drivers which have not yet made it in.  Additionally, several of the 
> referenced
> functions have moved and/or been refactored.  I attempted to minimize changes
> while keeping the same semantics.
> 
> Signed-off-by: Adin Scannell <adin@xxxxxxxxxxx>
> 
> Index: linux/drivers/block/xen-blkback/blkback.c
> ===================================================================
> ---
>  drivers/block/xen-blkback/blkback.c |    4 ++-
>  drivers/xen/grant-table.c           |    7 ++++-
>  drivers/xen/xenbus/xenbus_client.c  |    4 +++
>  include/xen/grant_table.h           |   39 
> +++++++++++++++++++++++++++++++++++
>  include/xen/interface/grant_table.h |    6 ++++-

>  5 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c 
> b/drivers/block/xen-blkback/blkback.c
> index 15ec4db..d3fb290 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -390,7 +390,9 @@ static int xen_blkbk_map(struct blkif_request *req,
>        * the page from the other domain.
>        */
>       for (i = 0; i < nseg; i++) {
> -             if (unlikely(map[i].status != 0)) {
> +             if (unlikely(map[i].status == GNTST_eagain))
> +                     
> gnttab_check_GNTST_eagain_do_while(GNTTABOP_map_grant_ref, &map[i]);
> +             if (unlikely(map[i].status != GNTST_okay)) {
>                       pr_debug(DRV_PFX "invalid buffer -- could not remap 
> it\n");
>                       map[i].handle = BLKBACK_INVALID_HANDLE;
>                       ret |= 1;

> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index bf1c094..48826c3 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -464,9 +464,12 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>  
>       for (i = 0; i < count; i++) {
>               /* Do not add to override if the map failed. */
> -             if (map_ops[i].status)
> +             if (map_ops[i].status != GNTST_okay && map_ops[i].status != 
> GNTST_eagain)
>                       continue;
>  
> +             if (map_ops[i].status == GNTST_eagain)
> +                     return -EAGAIN;
> +
>               if (map_ops[i].flags & GNTMAP_contains_pte) {
>                       pte = (pte_t *) 
> (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
>                               (map_ops[i].host_addr & ~PAGE_MASK));
> @@ -565,7 +568,7 @@ static int gnttab_map(unsigned int start_idx, unsigned 
> int end_idx)
>               return -ENOSYS;
>       }
>  
> -     BUG_ON(rc || setup.status);
> +     BUG_ON(rc || (setup.status != GNTST_okay));
>  
>       rc = arch_gnttab_map_shared(frames, nr_gframes, 
> gnttab_max_grant_frames(),
>                                   &shared);
> diff --git a/drivers/xen/xenbus/xenbus_client.c 
> b/drivers/xen/xenbus/xenbus_client.c
> index 1906125..d123c78 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -455,6 +455,8 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev, int 
> gnt_ref, void **vaddr)
>       if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
>               BUG();
>  
> +     if (op.status == GNTST_eagain)
> +             gnttab_check_GNTST_eagain_do_while(GNTTABOP_map_grant_ref, 
> (&op));
>       if (op.status != GNTST_okay) {
>               free_vm_area(area);
>               xenbus_dev_fatal(dev, op.status,
> @@ -499,6 +501,8 @@ int xenbus_map_ring(struct xenbus_device *dev, int 
> gnt_ref,
>       if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
>               BUG();
>  
> +     if (op.status == GNTST_eagain)
> +             gnttab_check_GNTST_eagain_do_while(GNTTABOP_map_grant_ref, 
> (&op));
>       if (op.status != GNTST_okay) {
>               xenbus_dev_fatal(dev, op.status,
>                                "mapping in shared page %d from domain %d",
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 11e2dfc..46fac85 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -37,6 +37,8 @@
>  #ifndef __ASM_GNTTAB_H__
>  #define __ASM_GNTTAB_H__
>  
> +#include <linux/delay.h>
> +
>  #include <asm/page.h>
>  
>  #include <xen/interface/xen.h>
> @@ -160,4 +162,41 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>  int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>                     struct page **pages, unsigned int count);
>  
> +#define gnttab_check_GNTST_eagain_while(__HCop, __HCarg_p)                   
>                 \

So why does this have to be a macro? What is the advantage of that
versus making this a function?

> +do {                                                                         
>                                                                 \
> +     u8 __hc_delay = 1;                                                      
>                                                         \
> +     int __ret;                                                              
>                                                                 \
> +     while (unlikely((__HCarg_p)->status == GNTST_eagain && __hc_delay)) {   
> \
> +             msleep(__hc_delay++);                                           
>                                                 \

Ugh. Not sure what happend to this, but there are tons of '\' at the
end.

So why msleep? Why not go for a proper yield? Call the safe_halt()
function?

> +             __ret = HYPERVISOR_grant_table_op(__HCop, (__HCarg_p), 1);      
>                 \
> +             BUG_ON(__ret);                                                  
>                                                         \
> +     }                                                                       
>                                                                         \
> +     if (__hc_delay == 0) {                                                  
>                                                 \

So this would happen if we rolled over __hc_delay, so did this more than
255 times? Presumarily this can happen if the swapper in dom0 crashes..

I would recommend you use 'WARN' here and include tons of details.
This is a pretty serious issues, is it not?

> +             printk(KERN_ERR "%s: gnt busy\n", __func__,);                   
>                         \
> +             (__HCarg_p)->status = GNTST_bad_page;                           
>                                 \
> +     }                                                                       
>                                                                         \
> +     if ((__HCarg_p)->status != GNTST_okay)                                  
>                                 \
> +             printk(KERN_ERR "%s: gnt status %x\n",                          
>                                 \
> +                     __func__, (__HCarg_p)->status);                         
>                                         \

Use GNTTABOP_error_msgs. Also should we continue? What is the
impact if we do continue? The times this is hit is if the status
is not GNTS_okay and if it is not GNTS_eagain - so what are the
situations in which this happens and what can the domain do
to recover? Should there be some helpfull message instead of
just "gnt status: X" ?

> +} while(0)
> +
> +#define gnttab_check_GNTST_eagain_do_while(__HCop, __HCarg_p)                
>         \
> +do {                                                                         
>                                                         \
> +     u8 __hc_delay = 1;                                                      
>                                                 \
> +     int __ret;                                                              
>                                                         \
> +     do {                                                                    
>                                                         \
> +             __ret = HYPERVISOR_grant_table_op(__HCop, (__HCarg_p), 1);      
>         \
> +             BUG_ON(__ret);                                                  
>                                                 \
> +             if ((__HCarg_p)->status == GNTST_eagain)                        
>                         \
> +                     msleep(__hc_delay++);                                   
>                                         \
> +     } while ((__HCarg_p)->status == GNTST_eagain && __hc_delay);            
> \
> +     if (__hc_delay == 0) {                                                  
>                                         \
> +             printk(KERN_ERR "%s: gnt busy\n", __func__);                    
>                 \
> +             (__HCarg_p)->status = GNTST_bad_page;                           
>                         \
> +     }                                                                       
>                                                                 \
> +     if ((__HCarg_p)->status != GNTST_okay)                                  
>                         \
> +             printk(KERN_ERR "%s: gnt status %x\n",                          
>                         \
> +                     __func__, (__HCarg_p)->status);                         
>                                 \
> +} while(0)
> +
>  #endif /* __ASM_GNTTAB_H__ */
> diff --git a/include/xen/interface/grant_table.h 
> b/include/xen/interface/grant_table.h
> index 39e5717..ba04080 100644
> --- a/include/xen/interface/grant_table.h
> +++ b/include/xen/interface/grant_table.h
> @@ -363,6 +363,8 @@ DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size);
>  #define GNTST_permission_denied (-8) /* Not enough privilege for operation.  
> */
>  #define GNTST_bad_page         (-9) /* Specified page was invalid for op.    
> */
>  #define GNTST_bad_copy_arg    (-10) /* copy arguments cross page boundary */
> +#define GNTST_address_too_big (-11) /* transfer page address too large.      
> */
> +#define GNTST_eagain          (-12) /* Could not map at the moment. Retry.   
> */
>  
>  #define GNTTABOP_error_msgs {                   \
>      "okay",                                     \
> @@ -375,7 +377,9 @@ DEFINE_GUEST_HANDLE_STRUCT(gnttab_query_size);
>      "no spare translation slot in the I/O MMU", \
>      "permission denied",                        \
>      "bad page",                                 \
> -    "copy arguments cross page boundary"        \
> +    "copy arguments cross page boundary",       \
> +    "page address size too large",              \
> +    "could not map at the moment, retry"        \
>  }
>  
>  #endif /* __XEN_PUBLIC_GRANT_TABLE_H__ */
> -- 
> 1.6.2.5
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.