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 18:05:50 +0800 Message-ID: <559269FE.5000904@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> <559264D2.7060406@cn.fujitsu.com> <1435657802.21469.55.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: <1435657802.21469.55.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 05:50 PM, Ian Campbell wrote: > 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 ;-) Ok, will do :) > >>> 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. > > > . > -- Thanks, Yang.