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: Fri, 30 Aug 2013 09:42:51 +0100 Message-ID: <52205B0B.1050309@citrix.com> References: <521BD6A0.6030207@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1582799200995794327==" 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 , Ian Jackson , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============1582799200995794327== Content-Type: multipart/alternative; boundary="------------070503030002080508080707" --------------070503030002080508080707 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 29/08/13 22:49, Shriram Rajagopalan wrote: > On Mon, Aug 26, 2013 at 6:28 PM, Andrew Cooper > > wrote: > > 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. > > > Did you mean bool (not bool_t) ? Because I didnt see a typedef in > libxl folder. > Indeed. Sorry. (I had Xen on the brain when reviewing, which does use bool_t rather than bool). ~Andrew --------------070503030002080508080707 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 29/08/13 22:49, Shriram Rajagopalan wrote:
On Mon, Aug 26, 2013 at 6:28 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
On 26/08/2013 00:45, rshriram@cs.ubc.ca wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@cs.ubc.ca>
> # 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 <rshriram@cs.ubc.ca>
>
> 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.


Did you mean bool (not bool_t) ? Because I didnt see a typedef in libxl folder.
 

Indeed.  Sorry.  (I had Xen on the brain when reviewing, which does use bool_t rather than bool).

~Andrew
--------------070503030002080508080707-- --===============1582799200995794327== 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.xen.org http://lists.xen.org/xen-devel --===============1582799200995794327==--