From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v3 COLOPre 06/26] libxl/save: Refactor libxl__domain_suspend_state Date: Tue, 30 Jun 2015 17:43:46 +0800 Message-ID: <559264D2.7060406@cn.fujitsu.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1435593706.32500.369.camel@citrix.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: Ian Campbell 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 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. > > 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. > > Ian. > > . > -- Thanks, Yang.