|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |