From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shriram Rajagopalan Subject: Re: [PATCH v3 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK Date: Tue, 31 Jan 2012 14:51:25 -0800 Message-ID: References: <1327942455-23587-1-git-send-email-stefano.stabellini@eu.citrix.com> Reply-To: rshriram@cs.ubc.ca Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4325662592453297858==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" , Ian Campbell List-Id: xen-devel@lists.xenproject.org --===============4325662592453297858== Content-Type: multipart/alternative; boundary=0015175cac0ce49bc904b7dacf8f --0015175cac0ce49bc904b7dacf8f Content-Type: text/plain; charset=ISO-8859-1 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 --0015175cac0ce49bc904b7dacf8f Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Tue, Jan 31, 2012 at 10:58 AM, Stefano Stabel= lini <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,
> =A0consistent_state, curr_state, such that you do
>
> =A0pagebuf_get(curr_state)
> =A0tailbuf_get(..)..
> =A0if (no error so far), then
> =A0 copy curr_state to consistent_state.
> =A0 goto copypages;
>
> finish:
> =A0..
> =A0 finish_hvm:
> =A0=A0=A0=A0 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 pagebu= f_get_one, you could
simplify the patch by adding a toolstack_data_t fie= ld to the pagebuf struct.
See comments below.

=A0
---

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restor= e.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 {
=A0 =A0 uint64_t vm_generationid_addr;
=A0} pagebuf_t;

+struct toolstack_data_t {
+ =A0 =A0uint8_t *data;
+ =A0 =A0uint32_t len;
+};
+

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

and dont forget to add the a= ppropriate free calls to pagebuf_free.
=A0
=A0 static int pagebuf_init(pagebuf_t* buf)
=A0{
=A0 =A0 memset(buf, 0, sizeof(*buf));
@@ -703,7 +708,8 @@ static void pagebuf_free(pagebuf_t* buf)
=A0}

=A0static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 pagebuf_t* buf, int fd, uint32_t dom)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pagebuf_t* buf, int f= d, uint32_t dom,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct toolstac= k_data_t *tdata)

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

+ =A0 =A0 =A0 =A0{
+ =A0 =A0case XC_SAVE_ID_TOOLSTACK:
+ =A0 =A0 =A0 =A0 =A0 =A0RDEXA= CT(fd, &tdata->len, sizeof(tdata->len));
+ =A0 =A0 =A0 =A0 =A0 =A0tdata->data =3D (uint8_t*) realloc(tdata->da= ta, tdata->len);
+ =A0 =A0 =A0 =A0 =A0 =A0if ( tdata->data =3D=3D NULL )
+ =A0 =A0 =A0 =A0 =A0 =A0{
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PERROR("error memor= y allocation");
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -1;
+ =A0 =A0 =A0 =A0 =A0 =A0}
+ =A0 =A0 =A0 =A0 =A0 =A0RDEXACT(fd, tdata->data, tdata->len);
+ =A0 =A0 =A0 =A0 =A0 =A0return pagebuf_get_one(xch, ctx, buf, fd, dom, tda= ta);
+ =A0 =A0 =A0 =A0}


RDEXACT(fd, &buf->tdata= ->len, sizeof())
...

@@ -1262,7 +1282,8 = @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned int= console_evtchn, unsigned long *console_mfn,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned int hvm, unsigned int= pae, int superpages,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int no_incr_generationid,
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long *vm_generationid= _addr)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long *vm_generationid= _addr,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct restore_callbacks *call= backs)
=A0{
=A0 =A0 DECLARE_DOMCTL;
=A0 =A0 int rc =3D 1, frc, i, j, n, m, pae_extended_cr3 =3D 0, ext_vcpucon= text =3D 0;
@@ -1310,6 +1331,7 @@ int xc_domain_restore(xc_interface *xch, int io= _fd, uint32_t dom,

=A0 =A0 pagebuf_t pagebuf;
=A0 =A0 tailbuf_t tailbuf, tmptail;
+ =A0 =A0struct toolstack_data_t tdata, tdata2, tdatatmp;
<= div>
struct toolstack_data_t tdata, tdatatmp;

and the memsets ofc= ourse.


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

Both live migrati= on and Remus reach this piece of code, while applying
the 1st or Nth che= ckpoint (respectively)

if (ctx->last_checkpoint)
{
=A0 //= =A0 DPRINTF("Last checkpoint, finishing\n");
=A0 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"= .

+=A0=A0=A0 tdatatmp =3D tdata;
+ =A0 =A0tdata =3D pagebuf.tdata;
+ =A0=A0 pagebuf.tdata =3D tdatatmp;

if (ctx->last_checkpoint)
...

@@ -2023,6 +2050,20 @@ int xc_domain_restore(xc_interface *xch, int io_fd, = uint32_t dom,
=A0 =A0 goto out;

=A0 finish_hvm:
+ =A0 =A0if ( callbacks !=3D NULL && callbacks-&g= t;toolstack_restore !=3D NULL )

how about an= other (tdata.data !=3D NULL) ? If older versions of xen (with lets say olde= r libxl toolstack)
were to migrate to xen 4.2, they wouldnt be sending the TOOLSTACK_ID would = they?
=A0
+ =A0 =A0{
+ =A0 =A0 =A0 =A0if ( callbacks->toolstack_restore(dom, tdata.data, tdat= a.len,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0callbacks->da= ta) < 0 )
+ =A0 =A0 =A0 =A0{
+ =A0 =A0 =A0 =A0 =A0 =A0PERROR("error calling= toolstack_restore");
+ =A0 =A0 =A0 =A0 =A0 =A0free(tdata.data);
+ =A0 =A0 =A0 =A0 =A0 =A0free(tdata2.data);
+ =A0 =A0 =A0 =A0 =A0 =A0goto out;
+ =A0 =A0 =A0 =A0}
+ =A0 =A0}
+ =A0 =A0free(tdata.data);
+ =A0 =A0free(tdata2.data);
+
=
Add free(buf->tdata.data) to pagebuf_free and
just do free(tdata.= data) here.

cheers
shriram
--0015175cac0ce49bc904b7dacf8f-- --===============4325662592453297858== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============4325662592453297858==--