Re: [Xen-devel] [PATCH 9/9] libxl: Convert to asynchronous: device removal

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-01-13 at 19:25 +0000, Ian Jackson wrote:
> Convert libxl_FOO_device_remove, and the function which does the bulk
> of the work, libxl__device_remove, to the new async ops scheme.
> 
> Adjust all callers.
> 
> Also remove libxl__wait_for_device_state which is now obsolete.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl.c          |   60 +++++++++++++--------
>  tools/libxl/libxl.h          |   16 ++++--
>  tools/libxl/libxl_device.c   |  118 
> +++++++++++++-----------------------------
>  tools/libxl/libxl_internal.h |   30 ++---------
>  tools/libxl/xl_cmdimpl.c     |    4 +-
>  5 files changed, 93 insertions(+), 135 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9890d79..d63da97 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1310,19 +1310,23 @@ out:
>  }
> 
>  int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
> -                             libxl_device_disk *disk)
> +                             libxl_device_disk *disk,
> +                             const libxl_asyncop_how *ao_how)
>  {
> -    GC_INIT(ctx);
> +    AO_CREATE(ctx, domid, ao_how);
>      libxl__device device;
>      int rc;
> 
>      rc = libxl__device_from_disk(gc, domid, disk, &device);
>      if (rc != 0) goto out;
> 
> -    rc = libxl__device_remove(gc, &device, 1);
> +    rc = libxl__initiate_device_remove(ao, &device);
> +    if (rc) goto out;
> +
> +    AO_INPROGRESS;
> +
>  out:
> -    GC_FREE;
> -    return rc;
> +    AO_ABORT(rc);
>  }

After all the internal complexity the actual usage is refreshingly
simple. Phew!

> 
>  int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
> @@ -1536,11 +1540,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t 
> domid, libxl_device_disk *disk)
> 
>      ret = 0;
> 
> -    libxl_device_disk_remove(ctx, domid, disks + i);
> +    libxl_device_disk_remove(ctx, domid, disks + i, 0);
>      libxl_device_disk_add(ctx, domid, disk);
>      stubdomid = libxl_get_stubdom_id(ctx, domid);
>      if (stubdomid) {
> -        libxl_device_disk_remove(ctx, stubdomid, disks + i);
> +        libxl_device_disk_remove(ctx, stubdomid, disks + i, 0);
>          libxl_device_disk_add(ctx, stubdomid, disk);
>      }
>  out:

The async capability here ought to be propagated to the
libxl_cdrom_insert interface too. I guess that would mean handling
compound asynchronous operations.

...in the fullness of time, of course.


> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 416d6e8..602bd01 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h

> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 5d05e90..e905133 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -357,85 +357,41 @@ int libxl__device_disk_dev_number(const char *virtpath, 
> int *pdisk,
>      return -1;
>  }
> 
[...]
> +typedef struct {
> +    libxl__ao *ao;
> +    libxl__ev_devstate ds;
> +} libxl__ao_device_remove;
> +
> +static void device_remove_cleanup(libxl__gc *gc,
> +                                  libxl__ao_device_remove *aorm) {
> +    if (!aorm) return;
> +    libxl__ev_devstate_cancel(gc, &aorm->ds);
> +}
> 
[...]
> +static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
> +                                   int rc) {
> +    libxl__ao_device_remove *aorm = CONTAINER_OF(ds, *aorm, ds);
> +    libxl__gc *gc = &aorm->ao->gc;
> +    libxl__ao_complete(egc, aorm->ao, rc);
> +    device_remove_cleanup(gc, aorm);
>  }
> 
[...]
> +int libxl__initiate_device_remove(libxl__ao *ao, libxl__device *dev)
>  {
> +    /* Arranges that dev will be removed from its guest.  When
> +     * this is done, the ao will be completed.  An error
> +     * return from libxl__device_remove means that the ao
> +     * will _not_ be completed and the caller must do so.

Do you mean aborted or cancelled rather than completed here?

> +     */
> +    AO_GC;
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      xs_transaction_t t;
>      char *be_path = libxl__device_backend_path(gc, dev);
>      char *state_path = libxl__sprintf(gc, "%s/state", be_path);
>      char *state = libxl__xs_read(gc, XBT_NULL, state_path);
>      int rc = 0;
> +    libxl__ao_device_remove *aorm = 0;
> 
>      if (!state)
>          goto out;
> @@ -458,23 +414,21 @@ retry_transaction:
>          }
>      }
> 
[...]
>      libxl__device_destroy_tapdisk(gc, be_path);
> 
[...]
> +    aorm = libxl__zalloc(gc, sizeof(*aorm));
> +    aorm->ao = ao;
> +    libxl__ev_devstate_init(&aorm->ds);
> 
[...]
> +    rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
> +                                 state_path, XenbusStateClosed,
> +                                 LIBXL_DESTROY_TIMEOUT * 1000);
> +    if (rc) goto out;
> +
> +    return 0;
> +
> + out:
> +    device_remove_cleanup(gc, aorm);
>      return rc;
>  }
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index b7f0f54..9920fb9 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -653,35 +653,15 @@ _hidden char *libxl__device_backend_path(libxl__gc *gc, 
> libxl__device *device);
[...]
> +/* Arranges that dev will be removed from its guest.  When
> + * this is done, the ao will be completed.  An error
> + * return from libxl__device_remove means that the ao
> + * will _not_ be completed and the caller must do so. */

This is the same comment as at the head of the implementation so the
same comment re aborting applies. Do we need both comments?

> +_hidden int libxl__initiate_device_remove(libxl__ao*, libxl__device *dev);
> 
>  /*
>   * libxl__ev_devstate - waits a given time for a device to
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c2b7a1e..659a9e6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4624,7 +4624,7 @@ int main_networkdetach(int argc, char **argv)
>              return 1;
>          }
>      }
> -    if (libxl_device_nic_remove(ctx, domid, &nic)) {
> +    if (libxl_device_nic_remove(ctx, domid, &nic, 0)) {
>          fprintf(stderr, "libxl_device_nic_del failed.\n");
>          return 1;
>      }

There aren't actually any examples of a caller using the ao-ness in xl
are there?

I know that sync is for the most part ao+wait but I'm a bit concerned
that e.g. several of the paths in libxl__ao_complete probably haven't
been run and one of the flow-charts added to tools/libxl/libxl_event.c
in patch 6/8 has probably never happened either.

IMHO this isn't a blocker for this patch but since xl is, in addition to
being a toolstack, a testbed for libxl perhaps one or more "gratuitously
asynchronous" calls could be made? Perhaps the libxl_cdrom_insert case
would be an interesting one? In particular the case in the
create_domain() event loop (e.g. so that the response to a cdrom eject
does not block shutdown processing).

Ian.



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