diff mbox

[v11,03/27] tools/libxl: Add back channel to allow migration target send data back

Message ID 1457080891-26054-4-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Changlong Xie March 4, 2016, 8:41 a.m. UTC 
From: Wen Congyang <wency@cn.fujitsu.com>

In COLO mode, secondary needs to send the following data to primary:
1. In libxl
   Secondary sends the following CHECKPOINT_CONTEXT to primary:
   CHECKPOINT_SVM_SUSPENDED, CHECKPOINT_SVM_READY and CHECKPOINT_SVM_RESUMED
2. In libxc
   Secondary sends the dirty pfn list to primary

But the io_fd only can be written in primary, and only can be read in
secondary. Save recv_fd in domain_suspend_state, and send_fd in
domain_create_state. Extend libxl_domain_create_restore API, add a
send_fd param to it. Add LIBXL_HAVE_CREATE_RESTORE_SEND_FD to indicate
the API change.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c                  |  3 +--
 tools/libxl/libxl.h                  | 29 ++++++++++++++++++++++++++++-
 tools/libxl/libxl_create.c           | 10 ++++++----
 tools/libxl/libxl_internal.h         |  2 ++
 tools/libxl/xl_cmdimpl.c             |  8 +++++++-
 tools/ocaml/libs/xl/xenlight_stubs.c |  2 +-
 6 files changed, 45 insertions(+), 9 deletions(-)

Comments

Ian Jackson March 4, 2016, 4:38 p.m. UTC | #1 
Changlong Xie writes ("[PATCH v11 03/27] tools/libxl: Add back channel to allow migration target send data back"):
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> In COLO mode, secondary needs to send the following data to primary:
> 1. In libxl
>    Secondary sends the following CHECKPOINT_CONTEXT to primary:
>    CHECKPOINT_SVM_SUSPENDED, CHECKPOINT_SVM_READY and CHECKPOINT_SVM_RESUMED
> 2. In libxc
>    Secondary sends the dirty pfn list to primary

The overall API approach here seems good to me.

But, my reading of the code is that this new fd is currently ignored.
This is, AFAICT, intentional in the non-colo case, and we have no colo
case yet.

So I think that this new parameter needs to be slightly better
documented.  I suggest:

 * In this patch, add a comment next to it saying "always pass -1".
 * In the patch were the fd actually starts to do something, change
    this comment to something more meaningful.

>  /*
> + * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
> + *
> + * If this is defined, libxl_domain_create_restore()'s API has changed to
> + * include a send_back_fd param which used for libxl migration back channel
> + * during COLO.
> + */
> +#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1

I have a minor grammar quibble with this.  I would write:

  "If this is defined, libxl_domain_create_restore()'s API
   includes the send_back_fd param.  This is used only with
   COLO, for the libxl migration back channel; other callers
   should pass -1."

And, with this definition of the API, I think that the code should
actually check that -1 is passed.  Personally I would be happy with
the error case either failing assert() or returning ERROR_INVAL, but
maybe other maintainers have a specific view.

Ian.
Wei Liu March 8, 2016, 4:38 p.m. UTC | #2 
On Fri, Mar 04, 2016 at 04:38:23PM +0000, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 03/27] tools/libxl: Add back channel to allow migration target send data back"):
> > From: Wen Congyang <wency@cn.fujitsu.com>
> > 
> > In COLO mode, secondary needs to send the following data to primary:
> > 1. In libxl
> >    Secondary sends the following CHECKPOINT_CONTEXT to primary:
> >    CHECKPOINT_SVM_SUSPENDED, CHECKPOINT_SVM_READY and CHECKPOINT_SVM_RESUMED
> > 2. In libxc
> >    Secondary sends the dirty pfn list to primary
> 
> The overall API approach here seems good to me.
> 
> But, my reading of the code is that this new fd is currently ignored.
> This is, AFAICT, intentional in the non-colo case, and we have no colo
> case yet.
> 
> So I think that this new parameter needs to be slightly better
> documented.  I suggest:
> 
>  * In this patch, add a comment next to it saying "always pass -1".
>  * In the patch were the fd actually starts to do something, change
>     this comment to something more meaningful.
> 
> >  /*
> > + * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
> > + *
> > + * If this is defined, libxl_domain_create_restore()'s API has changed to
> > + * include a send_back_fd param which used for libxl migration back channel
> > + * during COLO.
> > + */
> > +#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
> 
> I have a minor grammar quibble with this.  I would write:
> 
>   "If this is defined, libxl_domain_create_restore()'s API
>    includes the send_back_fd param.  This is used only with
>    COLO, for the libxl migration back channel; other callers
>    should pass -1."
> 
> And, with this definition of the API, I think that the code should
> actually check that -1 is passed.  Personally I would be happy with
> the error case either failing assert() or returning ERROR_INVAL, but
> maybe other maintainers have a specific view.
> 

I have no preference on this issue. Either calling assert or
returning ERROR_INVAL is fine by me.

Wei.

> Ian.
Changlong Xie March 17, 2016, 8:07 a.m. UTC | #3 
On 03/05/2016 12:38 AM, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 03/27] tools/libxl: Add back channel to allow migration target send data back"):
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> In COLO mode, secondary needs to send the following data to primary:
>> 1. In libxl
>>     Secondary sends the following CHECKPOINT_CONTEXT to primary:
>>     CHECKPOINT_SVM_SUSPENDED, CHECKPOINT_SVM_READY and CHECKPOINT_SVM_RESUMED
>> 2. In libxc
>>     Secondary sends the dirty pfn list to primary
>
> The overall API approach here seems good to me.
>
> But, my reading of the code is that this new fd is currently ignored.
> This is, AFAICT, intentional in the non-colo case, and we have no colo
> case yet.
>
> So I think that this new parameter needs to be slightly better
> documented.  I suggest:
>
>   * In this patch, add a comment next to it saying "always pass -1".
>   * In the patch were the fd actually starts to do something, change
>      this comment to something more meaningful.
>

Ok

>>   /*
>> + * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
>> + *
>> + * If this is defined, libxl_domain_create_restore()'s API has changed to
>> + * include a send_back_fd param which used for libxl migration back channel
>> + * during COLO.
>> + */
>> +#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
>
> I have a minor grammar quibble with this.  I would write:
>
>    "If this is defined, libxl_domain_create_restore()'s API
>     includes the send_back_fd param.  This is used only with
>     COLO, for the libxl migration back channel; other callers
>     should pass -1."
>
> And, with this definition of the API, I think that the code should
> actually check that -1 is passed.  Personally I would be happy with
> the error case either failing assert() or returning ERROR_INVAL, but
>maybe other maintainers have a specific view.
>

I prefer assert(), will fix in next version

Thanks
	-Xie

> Ian.
>
>
> .
>
diff mbox

Patch

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4cdc169..e9ab78c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -834,7 +834,6 @@  out:
 static void remus_failover_cb(libxl__egc *egc,
                               libxl__domain_save_state *dss, int rc);
 
-/* TODO: Explicit Checkpoint acknowledgements via recv_fd. */
 int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
                              uint32_t domid, int send_fd, int recv_fd,
                              const libxl_asyncop_how *ao_how)
@@ -871,7 +870,7 @@  int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
     dss->callback = remus_failover_cb;
     dss->domid = domid;
     dss->fd = send_fd;
-    /* TODO do something with recv_fd */
+    dss->recv_fd = recv_fd;
     dss->type = type;
     dss->live = 1;
     dss->debug = 0;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index f9e3ef5..5a286ec 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -639,6 +639,15 @@  typedef struct libxl__ctx libxl_ctx;
 #define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS 1
 
 /*
+ * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
+ *
+ * If this is defined, libxl_domain_create_restore()'s API has changed to
+ * include a send_back_fd param which used for libxl migration back channel
+ * during COLO.
+ */
+#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
+
+/*
  * LIBXL_HAVE_CREATEINFO_PVH
  * If this is defined, then libxl supports creation of a PVH guest.
  */
@@ -1165,6 +1174,7 @@  int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
                             LIBXL_EXTERNAL_CALLERS_ONLY;
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
                                 uint32_t *domid, int restore_fd,
+                                int send_back_fd,
                                 const libxl_domain_restore_params *params,
                                 const libxl_asyncop_how *ao_how,
                                 const libxl_asyncprogress_how *aop_console_how)
@@ -1185,7 +1195,7 @@  int static inline libxl_domain_create_restore_0x040200(
     libxl_domain_restore_params_init(&params);
 
     ret = libxl_domain_create_restore(
-        ctx, d_config, domid, restore_fd, &params, ao_how, aop_console_how);
+        ctx, d_config, domid, restore_fd, -1, &params, ao_how, aop_console_how);
 
     libxl_domain_restore_params_dispose(&params);
     return ret;
@@ -1193,6 +1203,23 @@  int static inline libxl_domain_create_restore_0x040200(
 
 #define libxl_domain_create_restore libxl_domain_create_restore_0x040200
 
+#elif defined(LIBXL_API_VERSION) && LIBXL_API_VERSION >= 0x040400 \
+                                 && LIBXL_API_VERSION < 0x040700
+
+int static inline libxl_domain_create_restore_0x040400(
+    libxl_ctx *ctx, libxl_domain_config *d_config,
+    uint32_t *domid, int restore_fd,
+    const libxl_domain_restore_params *params,
+    const libxl_asyncop_how *ao_how,
+    const libxl_asyncprogress_how *aop_console_how)
+    LIBXL_EXTERNAL_CALLERS_ONLY
+{
+    return libxl_domain_create_restore(ctx, d_config, domid, restore_fd,
+                                       -1, params, ao_how, aop_console_how);
+}
+
+#define libxl_domain_create_restore libxl_domain_create_restore_0x040400
+
 #endif
 
 int libxl_domain_soft_reset(libxl_ctx *ctx,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f1028bc..525bf85 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1572,7 +1572,7 @@  static void domain_create_cb(libxl__egc *egc,
                              int rc, uint32_t domid);
 
 static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
-                            uint32_t *domid, int restore_fd,
+                            uint32_t *domid, int restore_fd, int send_back_fd,
                             const libxl_domain_restore_params *params,
                             const libxl_asyncop_how *ao_how,
                             const libxl_asyncprogress_how *aop_console_how)
@@ -1587,6 +1587,7 @@  static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
     libxl_domain_config_init(&cdcs->dcs.guest_config_saved);
     libxl_domain_config_copy(ctx, &cdcs->dcs.guest_config_saved, d_config);
     cdcs->dcs.restore_fd = cdcs->dcs.libxc_fd = restore_fd;
+    cdcs->dcs.send_back_fd = send_back_fd;
     if (restore_fd > -1) {
         cdcs->dcs.restore_params = *params;
         rc = libxl__fd_flags_modify_save(gc, cdcs->dcs.restore_fd,
@@ -1765,18 +1766,19 @@  int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
                             const libxl_asyncop_how *ao_how,
                             const libxl_asyncprogress_how *aop_console_how)
 {
-    return do_domain_create(ctx, d_config, domid, -1, NULL,
+    return do_domain_create(ctx, d_config, domid, -1, -1, NULL,
                             ao_how, aop_console_how);
 }
 
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
                                 uint32_t *domid, int restore_fd,
+                                int send_back_fd,
                                 const libxl_domain_restore_params *params,
                                 const libxl_asyncop_how *ao_how,
                                 const libxl_asyncprogress_how *aop_console_how)
 {
-    return do_domain_create(ctx, d_config, domid, restore_fd, params,
-                            ao_how, aop_console_how);
+    return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd,
+                            params, ao_how, aop_console_how);
 }
 
 int libxl_domain_soft_reset(libxl_ctx *ctx,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 15b9508..5314522 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3132,6 +3132,7 @@  struct libxl__domain_save_state {
     uint32_t domid;
     int fd;
     int fdfl; /* original flags on fd */
+    int recv_fd;
     libxl_domain_type type;
     int live;
     int debug;
@@ -3465,6 +3466,7 @@  struct libxl__domain_create_state {
     libxl_domain_config guest_config_saved; /* vanilla config */
     int restore_fd, libxc_fd;
     int restore_fdfl; /* original flags of restore_fd */
+    int send_back_fd;
     libxl_domain_restore_params restore_params;
     uint32_t domid_soft_reset;
     libxl__domain_create_cb *callback;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 990d3c9..523e0e9 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -159,6 +159,7 @@  struct domain_create {
     char *extra_config; /* extra config string */
     const char *restore_file;
     int migrate_fd; /* -1 means none */
+    int send_back_fd; /* -1 means none */
     char **migration_domname_r; /* from malloc */
 };
 
@@ -2691,6 +2692,7 @@  static uint32_t create_domain(struct domain_create *dom_info)
     int config_len = 0;
     int restore_fd = -1;
     int restore_fd_to_close = -1;
+    int send_back_fd = -1;
     const libxl_asyncprogress_how *autoconnect_console_how;
     struct save_file_header hdr;
     uint32_t domid_soft_reset = INVALID_DOMID;
@@ -2708,6 +2710,7 @@  static uint32_t create_domain(struct domain_create *dom_info)
         if (migrate_fd >= 0) {
             restore_source = "<incoming migration stream>";
             restore_fd = migrate_fd;
+            send_back_fd = dom_info->send_back_fd;
         } else {
             restore_source = restore_file;
             restore_fd = open(restore_file, O_RDONLY);
@@ -2896,7 +2899,7 @@  start:
 
         ret = libxl_domain_create_restore(ctx, &d_config,
                                           &domid, restore_fd,
-                                          &params,
+                                          send_back_fd, &params,
                                           0, autoconnect_console_how);
 
         libxl_domain_restore_params_dispose(&params);
@@ -4459,6 +4462,7 @@  static void migrate_receive(int debug, int daemonize, int monitor,
     dom_info.monitor = monitor;
     dom_info.paused = 1;
     dom_info.migrate_fd = recv_fd;
+    dom_info.send_back_fd = send_fd;
     dom_info.migration_domname_r = &migration_domname;
     dom_info.checkpointed_stream = checkpointed;
 
@@ -4632,6 +4636,7 @@  int main_restore(int argc, char **argv)
     dom_info.config_file = config_file;
     dom_info.restore_file = checkpoint_file;
     dom_info.migrate_fd = -1;
+    dom_info.send_back_fd = -1;
     dom_info.vnc = vnc;
     dom_info.vncautopass = vncautopass;
     dom_info.console_autoconnect = console_autoconnect;
@@ -5099,6 +5104,7 @@  int main_create(int argc, char **argv)
     dom_info.quiet = quiet;
     dom_info.config_file = filename;
     dom_info.migrate_fd = -1;
+    dom_info.send_back_fd = -1;
     dom_info.vnc = vnc;
     dom_info.vncautopass = vncautopass;
     dom_info.console_autoconnect = console_autoconnect;
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 4133527..98b52b9 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -538,7 +538,7 @@  value stub_libxl_domain_create_restore(value ctx, value domain_config, value par
 
 	caml_enter_blocking_section();
 	ret = libxl_domain_create_restore(CTX, &c_dconfig, &c_domid, restore_fd,
-		&c_params, ao_how, NULL);
+		-1, &c_params, ao_how, NULL);
 	caml_leave_blocking_section();
 
 	free(ao_how);