From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v6 COLO 03/15] primary vm suspend/get_dirty_pfn/resume/checkpoint code Date: Tue, 16 Jun 2015 12:05:47 +0100 Message-ID: <1434452747.13744.110.camel@citrix.com> References: <1433735159-26739-1-git-send-email-yanghy@cn.fujitsu.com> <1433735159-26739-4-git-send-email-yanghy@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1433735159-26739-4-git-send-email-yanghy@cn.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Yang Hongyang Cc: wei.liu2@citrix.com, wency@cn.fujitsu.com, andrew.cooper3@citrix.com, yunhong.jiang@intel.com, eddie.dong@intel.com, xen-devel@lists.xen.org, guijianfeng@cn.fujitsu.com, rshriram@cs.ubc.ca, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Mon, 2015-06-08 at 11:45 +0800, Yang Hongyang wrote: > diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h > index 86bcf9c..d5902a6 100644 > --- a/tools/libxc/include/xenguest.h > +++ b/tools/libxc/include/xenguest.h > @@ -75,6 +75,18 @@ struct save_callbacks { > */ > int (*toolstack_save)(uint32_t domid, uint8_t **buf, uint32_t *len, void *data); > > + /* Called after the guest is suspended. > + * > + * returns the list of dirty pfn: > + * struct { > + * uint64_t count; > + * uint64_t pfn[]; > + * }; Seeing this comment and then a callback which returns a uint8_t* makes me suspicious. Can we not do something a bit more typesafe here, like returning a pointer to a suitable struct? > + * > + * Note: the caller must free the return value. > + */ > + uint8_t *(*get_dirty_pfn)(void *data); > + > /* to be provided as the last argument to each callback function */ > void* data; > }; > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 10d3d82..1145ae4 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -17,6 +17,7 @@ > #include "libxl_osdeps.h" > > #include "libxl_internal.h" > +#include "libxl_colo.h" > > #define PAGE_TO_MEMKB(pages) ((pages) * 4) > #define BACKEND_STRING_SIZE 5 > @@ -841,7 +842,10 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info, > assert(info); > > /* Point of no return */ > - libxl__remus_setup(egc, &dss->rs); > + if (libxl_defbool_val(info->colo)) libxl code must arrange to have called libxl_defbool_setdefault before using libxl_defbool_val, which I don't see here. There is a big block of such settings near the top of this function which you should add to. On the other hand -- is it possible for a caller to say they don't care what kind of check pointing they want and have libxl decide? If not then it doesn't make sense to use a defbool, a regular bool would be appropriate. I'm also wondering to what extent COLO could be considered an extension to Remus, as opposed to an alternative -- iow I'm unsure if reusing libxl_domain_remus_start as the API makes sense (the implementation could still be shared where appropriate). > diff --git a/tools/libxl/libxl_colo.h b/tools/libxl/libxl_colo.h > index 91df275..26a2563 100644 > --- a/tools/libxl/libxl_colo.h > +++ b/tools/libxl/libxl_colo.h > @@ -35,4 +35,14 @@ extern void libxl__colo_restore_teardown(libxl__egc *egc, > libxl__colo_restore_state *crs, > int rc); > > +extern void libxl__colo_save_domain_suspend_callback(void *data); > +extern void libxl__colo_save_domain_resume_callback(void *data); > +extern void libxl__colo_save_domain_checkpoint_callback(void *data); > +extern void libxl__colo_save_get_dirty_pfn_callback(void *data); > +extern void libxl__colo_save_setup(libxl__egc *egc, > + libxl__colo_save_state *css); > +extern void libxl__colo_save_teardown(libxl__egc *egc, > + libxl__colo_save_state *css, > + int rc); Should all be marked _hidden I think? [...]