[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 COLOPre 06/26] libxl/save: Refactor libxl__domain_suspend_state



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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.