[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Tue, Jan 31, 2012 at 10:58 AM, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 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 = "" 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.