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

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

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@aepfle.de>
> Acked-by: Patrick Colp <pjcolp@cs.ubc.ca>
> 
> 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@scannell.ca>
> 
> 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
>