Re: [PATCH v3 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

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

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

On Tue, Jan 31, 2012 at 10:58 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:

> On Tue, 31 Jan 2012, Shriram Rajagopalan wrote:
> > Or if you have to receive them in the pagebuf, then have two pieces of
> the same state,
> >  consistent_state, curr_state, such that you do
> >
> >  pagebuf_get(curr_state)
> >  tailbuf_get(..)..
> >  if (no error so far), then
> >   copy curr_state to consistent_state.
> >   goto copypages;
> >
> > finish:
> >  ..
> >   finish_hvm:
> >      apply consistent_state.
>
> I think I am starting to understand: see appended patch if it makes
> things better.
>
>
Instead of adding yet another parameter to pagebuf_get_one, you could
simplify the patch by adding a toolstack_data_t field to the pagebuf struct.
See comments below.



> ---
>
> diff --git a/tools/libxc/xc_domain_restore.c
> b/tools/libxc/xc_domain_restore.c
> index 3fda6f8..02b523d 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -684,6 +684,11 @@ typedef struct {
>     uint64_t vm_generationid_addr;
>  } pagebuf_t;
>
> +struct toolstack_data_t {
+    uint8_t *data;
+    uint32_t len;
+};
+

Add an instance of this struct to the pagebuf_t structure
(gets initialized to NULL, by pagebuf_init)

and dont forget to add the appropriate free calls to pagebuf_free.


>   static int pagebuf_init(pagebuf_t* buf)
>  {
>     memset(buf, 0, sizeof(*buf));
> @@ -703,7 +708,8 @@ static void pagebuf_free(pagebuf_t* buf)
>  }
>
>  static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,
> -                           pagebuf_t* buf, int fd, uint32_t dom)
> +                           pagebuf_t* buf, int fd, uint32_t dom,
> +                           struct toolstack_data_t *tdata)
>

Eliminate the extra arg. and all the fixes to pagebuf_get_one() calls.

 +        {
> +    case XC_SAVE_ID_TOOLSTACK:
> +            RDEXACT(fd, &tdata->len, sizeof(tdata->len));
> +            tdata->data = (uint8_t*) realloc(tdata->data, tdata->len);
> +            if ( tdata->data == NULL )
> +            {
> +                PERROR("error memory allocation");
> +                return -1;
> +            }
> +            RDEXACT(fd, tdata->data, tdata->len);
> +            return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
> +        }
>
>
RDEXACT(fd, &buf->tdata->len, sizeof())
...

@@ -1262,7 +1282,8 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>                        unsigned int console_evtchn, unsigned long
> *console_mfn,
>                       unsigned int hvm, unsigned int pae, int superpages,
>                       int no_incr_generationid,
> -                      unsigned long *vm_generationid_addr)
> +                      unsigned long *vm_generationid_addr,
> +                      struct restore_callbacks *callbacks)
>  {
>     DECLARE_DOMCTL;
>     int rc = 1, frc, i, j, n, m, pae_extended_cr3 = 0, ext_vcpucontext = 0;
> @@ -1310,6 +1331,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>
>     pagebuf_t pagebuf;
>     tailbuf_t tailbuf, tmptail;
> +    struct toolstack_data_t tdata, tdata2, tdatatmp;
>

struct toolstack_data_t tdata, tdatatmp;

and the memsets ofcourse.


> @@ -1441,7 +1465,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>          if ( !ctx->completed ) {
>             pagebuf.nr_physpages = pagebuf.nr_pages = 0;
>             pagebuf.compbuf_pos = pagebuf.compbuf_size = 0;
> -            if ( pagebuf_get_one(xch, ctx, &pagebuf, io_fd, dom) < 0 ) {
> +            if ( pagebuf_get_one(xch, ctx, &pagebuf, io_fd, dom, &tdata)
> < 0 ) {
>                  PERROR("Error when reading batch");
>                 goto out;
>             }
>

Both live migration and Remus reach this piece of code, while applying
the 1st or Nth checkpoint (respectively)

if (ctx->last_checkpoint)
{
  //  DPRINTF("Last checkpoint, finishing\n");
  goto finish;
}

// DPRINTF("Buffered checkpoint\n");

if ( pagebuf_get(xch, ctx, &pagebuf, io_fd, dom) ) {

So, your code could be plugged right on top of the first "if".

+    tdatatmp = tdata;
+    tdata = pagebuf.tdata;
+    pagebuf.tdata = tdatatmp;

if (ctx->last_checkpoint)
...

@@ -2023,6 +2050,20 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>     goto out;
>
>   finish_hvm:
> +    if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
>

how about another (tdata.data != NULL) ? If older versions of xen (with
lets say older libxl toolstack)
were to migrate to xen 4.2, they wouldnt be sending the TOOLSTACK_ID would
they?


> +    {
> +        if ( callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> +                    callbacks->data) < 0 )
> +        {
> +            PERROR("error calling toolstack_restore");
> +            free(tdata.data);
> +            free(tdata2.data);
> +            goto out;
> +        }
> +    }
> +    free(tdata.data);
> +    free(tdata2.data);
> +
>

Add free(buf->tdata.data) to pagebuf_free and
just do free(tdata.data) here.

cheers
shriram

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

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

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