From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 4 of 5] tools/libxl: Control network buffering in remus callbacks Date: Mon, 26 Aug 2013 23:28:48 +0100 Message-ID: <521BD6A0.6030207@citrix.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: rshriram@cs.ubc.ca Cc: stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 26/08/2013 00:45, rshriram@cs.ubc.ca wrote: > # HG changeset patch > # User Shriram Rajagopalan > # Date 1377473611 25200 > # Node ID c6804ccfe660cb9c373c5f53a8996d0443316684 > # Parent 4b23104828c09218aa7f9fbde1578bb6706e13d6 > tools/libxl: Control network buffering in remus callbacks > > This patch constitutes the core network buffering logic. > and does the following: > a) create a new network buffer when the domain is suspended > (remus_domain_suspend_callback) > b) release the previous network buffer pertaining to the > committed checkpoint (remus_domain_checkpoint_dm_saved) > > Signed-off-by: Shriram Rajagopalan > > diff -r 4b23104828c0 -r c6804ccfe660 tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Sun Aug 25 16:33:29 2013 -0700 > +++ b/tools/libxl/libxl_dom.c Sun Aug 25 16:33:31 2013 -0700 > @@ -1213,12 +1213,28 @@ int libxl__toolstack_save(uint32_t domid > > /*----- remus callbacks -----*/ > > +/* REMUS TODO: Issue disk checkpoint reqs. */ Why does this comment need to move? > static int libxl__remus_domain_suspend_callback(void *data) > { > - /* REMUS TODO: Issue disk and network checkpoint reqs. */ > - return libxl__domain_suspend_common_callback(data); > + libxl__save_helper_state *shs = data; > + libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs); > + libxl__remus_ctx *remus_ctx = dss->remus_ctx; > + int is_suspended = 0; > + STATE_AO_GC(dss->ao); > + > + is_suspended = libxl__domain_suspend_common_callback(data); > + if (!remus_ctx->netbuf_ctx) split to new line please. > return is_suspended; > + > + if (is_suspended) { > + if (libxl__remus_netbuf_start_new_epoch(gc, dss->domid, > + remus_ctx)) > + return !is_suspended; > + } > + > + return is_suspended; is_suspended is logically a boolean, so should be bool_t. Unhelpfully, libxl__domain_suspend_common_callback() returns an int, although its implementation strictly returns 0 on failure and 1 on success. IMO, It would probably be best to have "bool_t is_suspended" set to "!!libxl__domain_suspend_common_callback()", at which the subsequent return !is_suspended doesn't look as suspect, although that is just a matter of style. ~Andrew > } > > +/* REMUS TODO: Deal with disk. */ > static int libxl__remus_domain_resume_callback(void *data) > { > libxl__save_helper_state *shs = data; > @@ -1229,7 +1245,6 @@ static int libxl__remus_domain_resume_ca > if (libxl__domain_resume(gc, dss->domid, /* Fast Suspend */1)) > return 0; > > - /* REMUS TODO: Deal with disk. Start a new network output buffer */ > return 1; > } > > @@ -1256,10 +1271,34 @@ static void libxl__remus_domain_checkpoi > static void remus_checkpoint_dm_saved(libxl__egc *egc, > libxl__domain_suspend_state *dss, int rc) > { > - /* REMUS TODO: Wait for disk and memory ack, release network buffer */ > - /* REMUS TODO: make this asynchronous */ > - assert(!rc); /* REMUS TODO handle this error properly */ > - usleep(dss->remus_ctx->interval * 1000); > + /* REMUS TODO: Wait for disk and explicit memory ack (through restore > + callback from remote) before releasing network buffer. */ > + libxl__remus_ctx *remus_ctx = dss->remus_ctx; > + struct timespec epoch; > + int ret; > + STATE_AO_GC(dss->ao); > + > + if (rc) { > + LOG(ERROR, "Failed to save device model. Terminating Remus.."); > + libxl__xc_domain_saverestore_async_callback_done(egc, > + &dss->shs, rc); > + return; > + } > + > + if (remus_ctx->netbuf_ctx) { > + ret = libxl__remus_netbuf_release_prev_epoch(gc, dss->domid, > + remus_ctx); > + if (ret) { > + libxl__xc_domain_saverestore_async_callback_done(egc, > + &dss->shs, > + ret); > + return; > + } > + } > + > + epoch.tv_sec = remus_ctx->interval / 1000; /* interval is in ms */ > + epoch.tv_nsec = remus_ctx->interval * 1000L * 1000L; > + nanosleep(&epoch, 0); > libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, 1); > } > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel