From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v3 COLOPre 06/26] libxl/save: Refactor libxl__domain_suspend_state Date: Tue, 30 Jun 2015 10:50:02 +0100 Message-ID: <1435657802.21469.55.camel@citrix.com> References: <1435213552-10556-1-git-send-email-yanghy@cn.fujitsu.com> <1435213552-10556-7-git-send-email-yanghy@cn.fujitsu.com> <1435593706.32500.369.camel@citrix.com> <559264D2.7060406@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559264D2.7060406@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 List-Id: xen-devel@lists.xenproject.org On Tue, 2015-06-30 at 17:43 +0800, Yang Hongyang wrote: > > On 06/30/2015 12:01 AM, Ian Campbell wrote: > > On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote: > > [...] > >> switch (type) { > >> case LIBXL_DOMAIN_TYPE_HVM: { > >> dss->hvm = 1; > >> + dsps->hvm = 1; > >> break; > >> } > >> case LIBXL_DOMAIN_TYPE_PV: > >> dss->hvm = 0; > >> + dsps->hvm = 0; > >> break; > > [...] > >> @@ -2913,9 +2914,27 @@ typedef struct libxl__logdirty_switch { > >> } libxl__logdirty_switch; > >> > >> struct libxl__domain_suspend_state { > >> + /* set by caller of domain_suspend_callback_common */ > >> + libxl__ao *ao; > >> + > >> + uint32_t domid; > >> + int hvm; > >> + /* private */ > >> + libxl__ev_evtchn guest_evtchn; > >> + int guest_evtchn_lockfd; > >> + int guest_responded; > >> + libxl__xswait_state pvcontrol; > >> + libxl__ev_xswatch guest_watch; > >> + libxl__ev_time guest_timeout; > >> + const char *dm_savefile; > >> + void (*callback_common_done)(libxl__egc*, > >> + struct libxl__domain_suspend_state*, int ok); > >> +}; > >> + > >> +struct libxl__domain_save_state { > >> /* set by caller of libxl__domain_suspend */ > >> libxl__ao *ao; > >> - libxl__domain_suspend_cb *callback; > >> + libxl__domain_save_cb *callback; > >> > >> uint32_t domid; > >> int fd; > >> @@ -2924,22 +2943,14 @@ struct libxl__domain_suspend_state { > >> int debug; > >> const libxl_domain_remus_info *remus; > >> /* private */ > >> - libxl__ev_evtchn guest_evtchn; > >> - int guest_evtchn_lockfd; > >> + libxl__domain_suspend_state dsps; > >> int hvm; > > > > I wonder if, given that any domain suspend must necessarily be contained > > within a domain save it would be preferable to have "suspend" code > > upcast the suspend_state to the containing save_state in order to look > > at ->hvm, rather than duplicating it. Likewise domid. > > The reason we include hvm in suspend state is that we need a more common > suspend function we will use on restore side (with COLO). It should not touch > the upper struct, because it will be used on both save/restore side. OK makes sense, thanks. You may as well mention this in the commit message so I don't forget next time ;-) > > For ao I can imagine that the suspend and save might actually have > > separate ao lifetimes, in which case these do of course need to remain > > different. > > > > Would that make any sense at all? > > > > FYI it was the dual initialisation of hvm quoted above which lead me to > > think along these lines. > > > > Alternatively perhaps suspend_state should a separate init function to > > logically separate it from the save_state initialiser. Perhaps taking > > the latter as an argument? Or possibly just the relevant fields. > > Yes, the latter should be better. I will separate the init of the > suspend state. Thanks.