| Message ID | 1485412569-7431-4-git-send-email-zhangchen.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 26, 2017 at 02:36:06PM +0800, Zhang Chen wrote: > In this patch we add a function to close > kernel COLO-Proxy on secondary side. > > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > --- > tools/libxl/libxl_colo_restore.c | 9 +++++++-- > tools/libxl/libxl_create.c | 9 +++++++-- > tools/libxl/libxl_types.idl | 1 + > tools/libxl/xl_cmdimpl.c | 18 +++++++++++++++--- > 4 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/tools/libxl/libxl_colo_restore.c b/tools/libxl/libxl_colo_restore.c > index 6a96328..1d42539 100644 > --- a/tools/libxl/libxl_colo_restore.c > +++ b/tools/libxl/libxl_colo_restore.c > @@ -774,8 +774,13 @@ static void colo_setup_checkpoint_devices(libxl__egc *egc, > > STATE_AO_GC(crs->ao); > > - cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VIF) | > - (1 << LIBXL__DEVICE_KIND_VBD); > + if (crs->cps.is_userspace_proxy) { > + cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VBD); > + } else { > + cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VIF) | > + (1 << LIBXL__DEVICE_KIND_VBD); > + } > + Style issue. > cds->callback = colo_restore_setup_cds_done; > cds->ao = ao; > cds->domid = crs->domid; > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index e3bc257..d230ecd 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -1609,6 +1609,7 @@ static int do_domain_create(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 char *colo_proxy_script, > + const bool userspace_colo_proxy, > const libxl_asyncop_how *ao_how, > const libxl_asyncprogress_how *aop_console_how) > { > @@ -1633,6 +1634,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, > cdcs->dcs.callback = domain_create_cb; > cdcs->dcs.domid_soft_reset = INVALID_DOMID; > cdcs->dcs.colo_proxy_script = colo_proxy_script; > + cdcs->dcs.crs.cps.is_userspace_proxy = userspace_colo_proxy; > libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how); > cdcs->domid_out = domid; > > @@ -1821,7 +1823,7 @@ int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, > { > unset_disk_colo_restore(d_config); > return do_domain_create(ctx, d_config, domid, -1, -1, NULL, NULL, > - ao_how, aop_console_how); > + false, ao_how, aop_console_how); > } > > int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, > @@ -1832,16 +1834,19 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, > const libxl_asyncprogress_how *aop_console_how) > { > char *colo_proxy_script = NULL; > + bool userspace_colo_proxy = false; > > if (params->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) { > colo_proxy_script = params->colo_proxy_script; > + userspace_colo_proxy = libxl_defbool_val(params->userspace_colo_proxy); I think I'm going to ask for a bit of cleanup here. You don't actually need the values of colo_proxy_script and userspace_colo_proxy here. So instead of having both values here. I suggest: 1. provide a patch to refactor existing code so that do_domain_create doesn't take colo_proxy_script anymore. It should be able to do cdcs->dcs.colo_proxy_script = params->colo_proxy_script. 2. rework this patch on top of that patch. Does this make sense? Let me know if this is not feasible due to I miss something obvious. > set_disk_colo_restore(d_config); > } else { > unset_disk_colo_restore(d_config); > } > > return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd, > - params, colo_proxy_script, ao_how, aop_console_how); > + params, colo_proxy_script, userspace_colo_proxy, > + ao_how, aop_console_how); > } > > int libxl_domain_soft_reset(libxl_ctx *ctx, > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 1bd2057..89c2c9d 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -390,6 +390,7 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ > ("checkpointed_stream", integer), > ("stream_version", uint32, {'init_val': '1'}), > ("colo_proxy_script", string), > + ("userspace_colo_proxy", libxl_defbool), I suppose you can use LIBXL_HAVE_COLO_USERSPACE_PROXY for this whole series. Since this series touches a lot of common code, I would like you to confirm you've tested configurations without COLO enabled. Basic VM lifecycle operations like create, save/restore and migration should still work. Wei.
On 01/28/2017 01:05 AM, Wei Liu wrote: > On Thu, Jan 26, 2017 at 02:36:06PM +0800, Zhang Chen wrote: >> In this patch we add a function to close >> kernel COLO-Proxy on secondary side. >> >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> --- >> tools/libxl/libxl_colo_restore.c | 9 +++++++-- >> tools/libxl/libxl_create.c | 9 +++++++-- >> tools/libxl/libxl_types.idl | 1 + >> tools/libxl/xl_cmdimpl.c | 18 +++++++++++++++--- >> 4 files changed, 30 insertions(+), 7 deletions(-) >> >> diff --git a/tools/libxl/libxl_colo_restore.c b/tools/libxl/libxl_colo_restore.c >> index 6a96328..1d42539 100644 >> --- a/tools/libxl/libxl_colo_restore.c >> +++ b/tools/libxl/libxl_colo_restore.c >> @@ -774,8 +774,13 @@ static void colo_setup_checkpoint_devices(libxl__egc *egc, >> >> STATE_AO_GC(crs->ao); >> >> - cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VIF) | >> - (1 << LIBXL__DEVICE_KIND_VBD); >> + if (crs->cps.is_userspace_proxy) { >> + cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VBD); >> + } else { >> + cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VIF) | >> + (1 << LIBXL__DEVICE_KIND_VBD); >> + } >> + > Style issue. I will fix this in next version. > >> cds->callback = colo_restore_setup_cds_done; >> cds->ao = ao; >> cds->domid = crs->domid; >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index e3bc257..d230ecd 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -1609,6 +1609,7 @@ static int do_domain_create(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 char *colo_proxy_script, >> + const bool userspace_colo_proxy, >> const libxl_asyncop_how *ao_how, >> const libxl_asyncprogress_how *aop_console_how) >> { >> @@ -1633,6 +1634,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, >> cdcs->dcs.callback = domain_create_cb; >> cdcs->dcs.domid_soft_reset = INVALID_DOMID; >> cdcs->dcs.colo_proxy_script = colo_proxy_script; >> + cdcs->dcs.crs.cps.is_userspace_proxy = userspace_colo_proxy; >> libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how); >> cdcs->domid_out = domid; >> >> @@ -1821,7 +1823,7 @@ int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, >> { >> unset_disk_colo_restore(d_config); >> return do_domain_create(ctx, d_config, domid, -1, -1, NULL, NULL, >> - ao_how, aop_console_how); >> + false, ao_how, aop_console_how); >> } >> >> int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, >> @@ -1832,16 +1834,19 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, >> const libxl_asyncprogress_how *aop_console_how) >> { >> char *colo_proxy_script = NULL; >> + bool userspace_colo_proxy = false; >> >> if (params->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) { >> colo_proxy_script = params->colo_proxy_script; >> + userspace_colo_proxy = libxl_defbool_val(params->userspace_colo_proxy); > I think I'm going to ask for a bit of cleanup here. > > You don't actually need the values of colo_proxy_script and > userspace_colo_proxy here. > > So instead of having both values here. I suggest: > > 1. provide a patch to refactor existing code so that do_domain_create > doesn't take colo_proxy_script anymore. It should be able to do > cdcs->dcs.colo_proxy_script = params->colo_proxy_script. > 2. rework this patch on top of that patch. > > Does this make sense? Let me know if this is not feasible due to I miss > something obvious. Good idea, I will follow your comments send a new patch in next version. > >> set_disk_colo_restore(d_config); >> } else { >> unset_disk_colo_restore(d_config); >> } >> >> return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd, >> - params, colo_proxy_script, ao_how, aop_console_how); >> + params, colo_proxy_script, userspace_colo_proxy, >> + ao_how, aop_console_how); >> } >> >> int libxl_domain_soft_reset(libxl_ctx *ctx, >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >> index 1bd2057..89c2c9d 100644 >> --- a/tools/libxl/libxl_types.idl >> +++ b/tools/libxl/libxl_types.idl >> @@ -390,6 +390,7 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ >> ("checkpointed_stream", integer), >> ("stream_version", uint32, {'init_val': '1'}), >> ("colo_proxy_script", string), >> + ("userspace_colo_proxy", libxl_defbool), > I suppose you can use LIBXL_HAVE_COLO_USERSPACE_PROXY for this whole > series. If I understand correctly, we use LIBXL_HAVE_COLO_USERSPACE_PROXY for this whole series means we make colo userspace proxy as default option, User can't use colo kernel proxy by a easy way (remus commands). The goal of this series is to provide a another way to run colo proxy(we recommended), We want to keep the colo kernel proxy function currently(maybe someone can maintain it), and when colo userspace proxy performance better than kernel proxy, we will change colo userspace as default option. Thanks Zhang Chen > > Since this series touches a lot of common code, I would like you to > confirm you've tested configurations without COLO enabled. Basic VM > lifecycle operations like create, save/restore and migration should > still work. > > Wei. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel > >
On Mon, Feb 06, 2017 at 05:27:43PM +0800, Zhang Chen wrote: [...] > > > > > > set_disk_colo_restore(d_config); > > > } else { > > > unset_disk_colo_restore(d_config); > > > } > > > return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd, > > > - params, colo_proxy_script, ao_how, aop_console_how); > > > + params, colo_proxy_script, userspace_colo_proxy, > > > + ao_how, aop_console_how); > > > } > > > int libxl_domain_soft_reset(libxl_ctx *ctx, > > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > > index 1bd2057..89c2c9d 100644 > > > --- a/tools/libxl/libxl_types.idl > > > +++ b/tools/libxl/libxl_types.idl > > > @@ -390,6 +390,7 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ > > > ("checkpointed_stream", integer), > > > ("stream_version", uint32, {'init_val': '1'}), > > > ("colo_proxy_script", string), > > > + ("userspace_colo_proxy", libxl_defbool), > > I suppose you can use LIBXL_HAVE_COLO_USERSPACE_PROXY for this whole > > series. > > If I understand correctly, we use LIBXL_HAVE_COLO_USERSPACE_PROXY for > this whole series means we make colo userspace proxy as default option, > User can't use colo kernel proxy by a easy way (remus commands). > The goal of this series is to provide a another way to run colo proxy(we > recommended), > We want to keep the colo kernel proxy function currently(maybe someone can > maintain it), and when colo userspace proxy performance better than kernel > proxy, > we will change colo userspace as default option. > No, that macro is not used to denote which mechanism is the default one. It is used to indicate the availability of a particular thing in libxl. So you can have that macro and choose whichever you want to be the default. Feel free to ask questions if I'm not clear enough. Wei.
On 02/06/2017 07:25 PM, Wei Liu wrote: > On Mon, Feb 06, 2017 at 05:27:43PM +0800, Zhang Chen wrote: > [...] >>>> set_disk_colo_restore(d_config); >>>> } else { >>>> unset_disk_colo_restore(d_config); >>>> } >>>> return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd, >>>> - params, colo_proxy_script, ao_how, aop_console_how); >>>> + params, colo_proxy_script, userspace_colo_proxy, >>>> + ao_how, aop_console_how); >>>> } >>>> int libxl_domain_soft_reset(libxl_ctx *ctx, >>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >>>> index 1bd2057..89c2c9d 100644 >>>> --- a/tools/libxl/libxl_types.idl >>>> +++ b/tools/libxl/libxl_types.idl >>>> @@ -390,6 +390,7 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ >>>> ("checkpointed_stream", integer), >>>> ("stream_version", uint32, {'init_val': '1'}), >>>> ("colo_proxy_script", string), >>>> + ("userspace_colo_proxy", libxl_defbool), >>> I suppose you can use LIBXL_HAVE_COLO_USERSPACE_PROXY for this whole >>> series. >> If I understand correctly, we use LIBXL_HAVE_COLO_USERSPACE_PROXY for >> this whole series means we make colo userspace proxy as default option, >> User can't use colo kernel proxy by a easy way (remus commands). >> The goal of this series is to provide a another way to run colo proxy(we >> recommended), >> We want to keep the colo kernel proxy function currently(maybe someone can >> maintain it), and when colo userspace proxy performance better than kernel >> proxy, >> we will change colo userspace as default option. >> > No, that macro is not used to denote which mechanism is the default one. > It is used to indicate the availability of a particular thing in libxl. > So you can have that macro and choose whichever you want to be the > default. > > Feel free to ask questions if I'm not clear enough. OK. I will add the macro "LIBXL_HAVE_COLO_USERSPACE_PROXY" like the "LIBXL_HAVE_REMUS". Thank you for your nice comments. Zhang Chen > Wei. > > > . >
diff --git a/tools/libxl/libxl_colo_restore.c b/tools/libxl/libxl_colo_restore.c index 6a96328..1d42539 100644 --- a/tools/libxl/libxl_colo_restore.c +++ b/tools/libxl/libxl_colo_restore.c @@ -774,8 +774,13 @@ static void colo_setup_checkpoint_devices(libxl__egc *egc, STATE_AO_GC(crs->ao); - cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VIF) | - (1 << LIBXL__DEVICE_KIND_VBD); + if (crs->cps.is_userspace_proxy) { + cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VBD); + } else { + cds->device_kind_flags = (1 << LIBXL__DEVICE_KIND_VIF) | + (1 << LIBXL__DEVICE_KIND_VBD); + } + cds->callback = colo_restore_setup_cds_done; cds->ao = ao; cds->domid = crs->domid; diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index e3bc257..d230ecd 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1609,6 +1609,7 @@ static int do_domain_create(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 char *colo_proxy_script, + const bool userspace_colo_proxy, const libxl_asyncop_how *ao_how, const libxl_asyncprogress_how *aop_console_how) { @@ -1633,6 +1634,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, cdcs->dcs.callback = domain_create_cb; cdcs->dcs.domid_soft_reset = INVALID_DOMID; cdcs->dcs.colo_proxy_script = colo_proxy_script; + cdcs->dcs.crs.cps.is_userspace_proxy = userspace_colo_proxy; libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how); cdcs->domid_out = domid; @@ -1821,7 +1823,7 @@ int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, { unset_disk_colo_restore(d_config); return do_domain_create(ctx, d_config, domid, -1, -1, NULL, NULL, - ao_how, aop_console_how); + false, ao_how, aop_console_how); } int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, @@ -1832,16 +1834,19 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, const libxl_asyncprogress_how *aop_console_how) { char *colo_proxy_script = NULL; + bool userspace_colo_proxy = false; if (params->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) { colo_proxy_script = params->colo_proxy_script; + userspace_colo_proxy = libxl_defbool_val(params->userspace_colo_proxy); set_disk_colo_restore(d_config); } else { unset_disk_colo_restore(d_config); } return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd, - params, colo_proxy_script, ao_how, aop_console_how); + params, colo_proxy_script, userspace_colo_proxy, + ao_how, aop_console_how); } int libxl_domain_soft_reset(libxl_ctx *ctx, diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 1bd2057..89c2c9d 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -390,6 +390,7 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ ("checkpointed_stream", integer), ("stream_version", uint32, {'init_val': '1'}), ("colo_proxy_script", string), + ("userspace_colo_proxy", libxl_defbool), ]) libxl_sched_params = Struct("sched_params",[ diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 905c5f6..1620bd7 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -162,6 +162,7 @@ struct domain_create { char *extra_config; /* extra config string */ const char *restore_file; char *colo_proxy_script; + bool userspace_colo_proxy; int migrate_fd; /* -1 means none */ int send_back_fd; /* -1 means none */ char **migration_domname_r; /* from malloc */ @@ -3024,6 +3025,8 @@ start: params.stream_version = (hdr.mandatory_flags & XL_MANDATORY_FLAG_STREAMv2) ? 2 : 1; params.colo_proxy_script = dom_info->colo_proxy_script; + libxl_defbool_set(¶ms.userspace_colo_proxy, + dom_info->userspace_colo_proxy); ret = libxl_domain_create_restore(ctx, &d_config, &domid, restore_fd, @@ -4824,7 +4827,8 @@ static void migrate_receive(int debug, int daemonize, int monitor, int pause_after_migration, int send_fd, int recv_fd, libxl_checkpointed_stream checkpointed, - char *colo_proxy_script) + char *colo_proxy_script, + bool userspace_colo_proxy) { uint32_t domid; int rc, rc2; @@ -4852,6 +4856,7 @@ static void migrate_receive(int debug, int daemonize, int monitor, dom_info.migration_domname_r = &migration_domname; dom_info.checkpointed_stream = checkpointed; dom_info.colo_proxy_script = colo_proxy_script; + dom_info.userspace_colo_proxy = userspace_colo_proxy; rc = create_domain(&dom_info); if (rc < 0) { @@ -5051,11 +5056,13 @@ int main_migrate_receive(int argc, char **argv) int debug = 0, daemonize = 1, monitor = 1, pause_after_migration = 0; libxl_checkpointed_stream checkpointed = LIBXL_CHECKPOINTED_STREAM_NONE; int opt; + bool userspace_colo_proxy = false; char *script = NULL; static struct option opts[] = { {"colo", 0, 0, 0x100}, /* It is a shame that the management code for disk is not here. */ {"coloft-script", 1, 0, 0x200}, + {"userspace-colo-proxy", 0, 0, 0x300}, COMMON_LONG_OPTS }; @@ -5079,6 +5086,9 @@ int main_migrate_receive(int argc, char **argv) case 0x200: script = optarg; break; + case 0x300: + userspace_colo_proxy = true; + break; case 'p': pause_after_migration = 1; break; @@ -5090,7 +5100,7 @@ int main_migrate_receive(int argc, char **argv) } migrate_receive(debug, daemonize, monitor, pause_after_migration, STDOUT_FILENO, STDIN_FILENO, - checkpointed, script); + checkpointed, script, userspace_colo_proxy); return EXIT_SUCCESS; } @@ -8984,11 +8994,13 @@ int main_remus(int argc, char **argv) "-r", daemonize ? "" : " -e"); } else { - xasprintf(&rune, "exec %s %s xl migrate-receive %s %s %s %s", + xasprintf(&rune, "exec %s %s xl migrate-receive %s %s %s %s %s", ssh_command, host, "--colo", r_info.netbufscript ? "--coloft-script" : "", r_info.netbufscript ? r_info.netbufscript : "", + libxl_defbool_val(r_info.userspace_colo_proxy) ? + "--userspace-colo-proxy" : "", daemonize ? "" : " -e"); } }
In this patch we add a function to close kernel COLO-Proxy on secondary side. Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> --- tools/libxl/libxl_colo_restore.c | 9 +++++++-- tools/libxl/libxl_create.c | 9 +++++++-- tools/libxl/libxl_types.idl | 1 + tools/libxl/xl_cmdimpl.c | 18 +++++++++++++++--- 4 files changed, 30 insertions(+), 7 deletions(-)