Re: [PATCH 1 of 3] tools/libxl: QEMU device model suspend/resume

[Shriram Rajagopalan <rshriram@cs.ubc.ca>]

[-- Attachment #1.1: Type: text/plain, Size: 6678 bytes --]

On Wed, Jan 25, 2012 at 3:08 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:

> On Mon, 2012-01-23 at 22:46 +0000, rshriram@cs.ubc.ca wrote:
> > # HG changeset patch
> > # User Shriram Rajagopalan <rshriram@cs.ubc.ca>
> > # Date 1327358638 28800
> > # Node ID 11fb1dfda7de4d6759dec87d80cd16cf137f7369
> > # Parent  80fdf2182bc62ca358ba2f1a3513b47a4f8d9dfd
> > tools/libxl: QEMU device model suspend/resume
> >
> >  * Refactor the libxl__domain_save_device_model.
> >  * Add support for suspend/resume QEMU device model
> >    (both traditional xen and QMP).
> >
> >
> > Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> >
> > diff -r 80fdf2182bc6 -r 11fb1dfda7de tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c     Sat Jan 21 17:15:40 2012 +0000
> > +++ b/tools/libxl/libxl.c     Mon Jan 23 14:43:58 2012 -0800
> > @@ -477,7 +477,7 @@
> >
> >      rc = libxl__domain_suspend_common(gc, domid, fd, type, live, debug);
> >      if (!rc && type == LIBXL_DOMAIN_TYPE_HVM)
> > -        rc = libxl__domain_save_device_model(gc, domid, fd);
> > +        rc = libxl__domain_save_device_model(gc, domid, fd, /* No Remus
> */ 0);
> >      GC_FREE;
> >      return rc;
> >  }
> > diff -r 80fdf2182bc6 -r 11fb1dfda7de tools/libxl/libxl_dom.c
> > --- a/tools/libxl/libxl_dom.c Sat Jan 21 17:15:40 2012 +0000
> > +++ b/tools/libxl/libxl_dom.c Mon Jan 23 14:43:58 2012 -0800
> > @@ -534,6 +534,53 @@
> >      return 0;
> >  }
> >
> > +static int libxl__remus_domain_suspend_qemu(libxl__gc *gc, uint32_t
> domid)
> > +{
> > +
> > +    switch (libxl__device_model_version_running(gc, domid)) {
> > +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> > +        char *path = NULL;
> > +        path = libxl__sprintf(gc,
> "/local/domain/0/device-model/%d/command",
> > +                              domid);
> > +        libxl__xs_write(gc, XBT_NULL, path, "save");
> > +        libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL,
> NULL);
>
> This and libxl__remus_domain_resume_qemu, qemu_pci_add_xenstore,
> qemu_pci_remove_xenstore, libxl__domain_save_device_model and
> libxl_domain_unpause would probably benefit from a helper function to
> send a command to a traditional qemu.
>
> > +        break;
> > +    }
> > +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> > +        /* Stop QEMU */
> > +        if (libxl__qmp_stop(gc, domid))
> > +            return ERROR_FAIL;
> > +        break;
> > +    default:
> > +        return ERROR_INVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int libxl__remus_domain_resume_qemu(libxl__gc *gc, uint32_t
> domid)
> > +{
> > +
> > +    switch (libxl__device_model_version_running(gc, domid)) {
> > +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> > +        char *path = NULL;
> > +        path = libxl__sprintf(gc,
> "/local/domain/0/device-model/%d/command",
> > +                              domid);
> > +        libxl__xs_write(gc, XBT_NULL, path, "continue");
> > +        break;
> > +    }
> > +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> > +        /* Stop QEMU */
> > +        if (libxl__qmp_resume(gc, domid))
> > +            return ERROR_FAIL;
> > +        break;
> > +    default:
> > +        return ERROR_INVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
> >                                   libxl_domain_type type,
> >                                   int live, int debug)
> > @@ -620,7 +667,7 @@
> >      return rc;
> >  }
> >
> > -int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int
> fd)
> > +int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int
> fd, int remus)
> >  {
> >      libxl_ctx *ctx = libxl__gc_owner(gc);
> >      int ret, fd2 = -1, c;
> > @@ -631,13 +678,12 @@
> >
> >      switch (libxl__device_model_version_running(gc, domid)) {
> >      case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> > -        char *path = NULL;
> > -        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
> > -                   "Saving device model state to %s", filename);
> > -        path = libxl__sprintf(gc,
> "/local/domain/0/device-model/%d/command",
> > -                              domid);
> > -        libxl__xs_write(gc, XBT_NULL, path, "save");
> > -        libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL,
> NULL);
> > +        /* For Remus,we issue suspend_qemu call separately */
>
> Why?
>
>
In remus, save and transmit device model phases are decoupled. This code
(sending the saved device model to the target) is executed after the domain
is
resumed.

The control flow in current Remus is like this:
1 suspend vm
  1.1 suspend qemu (save command, that saves the device model to a file)
2 copy out dirty memory into a buffer
3 resume vm
  3.1 resume qemu
4 send the data in the buffer
5  send the saved device model
    (xc_domain_restore extracts this from the tailbuf)



> How does this interact with Stefano's XC_SAVEID_TOOLSTACK patches?
>
>
 I couldnt find anything online by this name. Can you point me to the patch
series?


I think some sort of high level description of the control flow used by
> Remus and how/why it differs from normal save/restore would be useful
> for review.
>
> > +        if (!remus) {
> > +            c = libxl__remus_domain_suspend_qemu(gc, domid);
>
> It seems odd to call this function libxl__remus_FOO and the use it
> when !remus. The function doesn't look to be inherently specific to
> either remus or !remus anyhow.
>
> > +            if (c)
> > +                return c;
> > +        }
> >          break;
> >      }
> >      case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> > @@ -668,8 +714,9 @@
> >      qemu_state_len = st.st_size;
> >      LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Qemu state is %d bytes\n",
> qemu_state_len);
> >
> > -    ret = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE,
> strlen(QEMU_SIGNATURE),
> > -                              "saved-state file", "qemu signature");
> > +    ret = libxl_write_exactly(ctx, fd, remus ? REMUS_QEMU_SIGNATURE :
> QEMU_SIGNATURE,
> > +                            remus ? strlen(REMUS_QEMU_SIGNATURE):
> strlen(QEMU_SIGNATURE),
> > +                            "saved-state file", "qemu signature");
>
> QEMU_SIGNATURE is "DeviceModelRecord0002" which I believe libxc treats
> identically to the Remus signature?
>
>
Thanks. Yep, the remus_signature is redundant.


> Again consideration of how this interacts with Stefano's patch would be
> useful. If possible we'd quite like to pull the qemu-restore our of
> xc_domain_restore for consistency with how xc_domain_save saves it, the
> current layering is quite mad.
>
> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 8908 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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