| Message ID | 20190802153606.32061-7-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxl refactoring to use ev_qmp (with API changes) | expand |
Anthony PERARD writes ("[PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
...
> + rc = libxl__ev_time_register_rel(ao, &lds->timeout,
> + switch_logdirty_timeout, 10 * 1000);
> + if (rc) goto out;
> +
> + qmp->ao = ao;
> + qmp->domid = domid;
> + qmp->payload_fd = -1;
> + qmp->callback = switch_qemu_xen_logdirty_done;
> + libxl__qmp_param_add_bool(gc, &args, "enable", enable);
> + rc = libxl__ev_qmp_send(gc, qmp, "xen-set-global-dirty-log", args);
I hate to suggest this at this stage, but: maybe the timeout could be
incorporated into libxl__ev_qmp ?
I think every libxl__qmp caller is going to need a timeout ?
Ian.
Ian Jackson writes ("Re: [PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"):
> Anthony PERARD writes ("[PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"):
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ...
> > + rc = libxl__ev_time_register_rel(ao, &lds->timeout,
> > + switch_logdirty_timeout, 10 * 1000);
> > + if (rc) goto out;
> > +
> > + qmp->ao = ao;
> > + qmp->domid = domid;
> > + qmp->payload_fd = -1;
> > + qmp->callback = switch_qemu_xen_logdirty_done;
> > + libxl__qmp_param_add_bool(gc, &args, "enable", enable);
> > + rc = libxl__ev_qmp_send(gc, qmp, "xen-set-global-dirty-log", args);
>
> I hate to suggest this at this stage, but: maybe the timeout could be
> incorporated into libxl__ev_qmp ?
>
> I think every libxl__qmp caller is going to need a timeout ?
I should say that apart from this question, the patch is fine.
I will not repeat this point in my review of further patches in this
series. If we add a timeout parameter to the qmp calls, they will all
need to be updated but the compiler should catch it.
Thanks,
Ian.
Ian Jackson writes ("Re: [PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"):
> Anthony PERARD writes ("[PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"):
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ...
> > + rc = libxl__ev_time_register_rel(ao, &lds->timeout,
> > + switch_logdirty_timeout, 10 * 1000);
> > + if (rc) goto out;
> > +
> > + qmp->ao = ao;
> > + qmp->domid = domid;
> > + qmp->payload_fd = -1;
> > + qmp->callback = switch_qemu_xen_logdirty_done;
> > + libxl__qmp_param_add_bool(gc, &args, "enable", enable);
> > + rc = libxl__ev_qmp_send(gc, qmp, "xen-set-global-dirty-log", args);
>
> I hate to suggest this at this stage, but: maybe the timeout could be
> incorporated into libxl__ev_qmp ?
>
> I think every libxl__qmp caller is going to need a timeout ?
If you agree, then maybe it would be more convenient to do this at the
end of the series, rather than making a complete rebase of it.
Anyway, let me know what you think.
Thanks,
Ian.
On Tue, Sep 17, 2019 at 05:52:24PM +0100, Ian Jackson wrote: > Anthony PERARD writes ("[PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"): > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > ... > > + rc = libxl__ev_time_register_rel(ao, &lds->timeout, > > + switch_logdirty_timeout, 10 * 1000); > > + if (rc) goto out; > > + > > + qmp->ao = ao; > > + qmp->domid = domid; > > + qmp->payload_fd = -1; > > + qmp->callback = switch_qemu_xen_logdirty_done; > > + libxl__qmp_param_add_bool(gc, &args, "enable", enable); > > + rc = libxl__ev_qmp_send(gc, qmp, "xen-set-global-dirty-log", args); > > I hate to suggest this at this stage, but: maybe the timeout could be > incorporated into libxl__ev_qmp ? > > I think every libxl__qmp caller is going to need a timeout ? Yes, every user of libxl__ev_qmp should have a timeout set up. But there are different way to set it up. When we have several command to send via QMP should we have a new timeout for every command or set only one when sending the first command, and only stopping the timeout when the last command's response have been received? In this patch series I've choose the second option. I can see several cases: - One QMP command -> easy, one timeout for it. - several commands, related or not. -> one timeout per command? or one timeout which cover all of them? one per commands means that a bad qemu could delay the operation several time longer than if there were a single timeout covering all the commands. - other slow operation are done beside a qmp command -> In this case, it might not be practical to have a timeout attach to the qmp command, we are going to need a timeout for the other operations. I guess we could try to optimise the simpler case when there is only one qmp operation, because that's the most common, but allow to send commands without having libxl__qmp setting a timeout.
Anthony PERARD writes ("Re: [PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"):
> On Tue, Sep 17, 2019 at 05:52:24PM +0100, Ian Jackson wrote:
> > I hate to suggest this at this stage, but: maybe the timeout could be
> > incorporated into libxl__ev_qmp ?
> >
> > I think every libxl__qmp caller is going to need a timeout ?
>
> Yes, every user of libxl__ev_qmp should have a timeout set up. But
> there are different way to set it up. When we have several command to
> send via QMP should we have a new timeout for every command or set only
> one when sending the first command, and only stopping the timeout when
> the last command's response have been received? In this patch series
> I've choose the second option.
>
> I can see several cases:
> - One QMP command
> -> easy, one timeout for it.
> - several commands, related or not.
> -> one timeout per command? or one timeout which cover all of them?
> one per commands means that a bad qemu could delay the operation
> several time longer than if there were a single timeout covering
> all the commands.
> - other slow operation are done beside a qmp command
> -> In this case, it might not be practical to have a timeout attach
> to the qmp command, we are going to need a timeout for the other
> operations.
Hmmm. Yes. You have obviously considered this better than me.q
> I guess we could try to optimise the simpler case when there is only one
> qmp operation, because that's the most common, but allow to send
> commands without having libxl__qmp setting a timeout.
That would be nice. I think this is a nice-to-have, not a blocker.
Thanks,
Ian.
On Thu, Sep 19, 2019 at 02:03:10PM +0100, Ian Jackson wrote: > Anthony PERARD writes ("Re: [PATCH 06/35] libxl: Use ev_qmp for switch_qemu_xen_logdirty"): > > On Tue, Sep 17, 2019 at 05:52:24PM +0100, Ian Jackson wrote: > > I guess we could try to optimise the simpler case when there is only one > > qmp operation, because that's the most common, but allow to send > > commands without having libxl__qmp setting a timeout. > > That would be nice. I think this is a nice-to-have, not a blocker. Good, I'll put that on a todo list for when further refactoring can be done as I don't think I want to rush this addition to the internal API. Thanks,
diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c index 13d08d6dae..e70aa15859 100644 --- a/tools/libxl/libxl_dom_save.c +++ b/tools/libxl/libxl_dom_save.c @@ -44,6 +44,10 @@ static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch*, static void domain_suspend_switch_qemu_xen_logdirty (libxl__egc *egc, int domid, unsigned enable, libxl__logdirty_switch *lds); +static void switch_qemu_xen_logdirty_done(libxl__egc *egc, + libxl__ev_qmp *qmp, + const libxl__json_object *, + int rc); static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev, const struct timeval *requested_abs, int rc); @@ -55,6 +59,7 @@ void libxl__logdirty_init(libxl__logdirty_switch *lds) lds->cmd_path = 0; libxl__ev_xswatch_init(&lds->watch); libxl__ev_time_init(&lds->timeout); + libxl__ev_qmp_init(&lds->qmp); } void libxl__domain_common_switch_qemu_logdirty(libxl__egc *egc, @@ -207,13 +212,40 @@ static void domain_suspend_switch_qemu_xen_logdirty { STATE_AO_GC(lds->ao); int rc; + libxl__json_object *args = NULL; + + /* Convenience aliases. */ + libxl__ev_qmp *const qmp = &lds->qmp; + + rc = libxl__ev_time_register_rel(ao, &lds->timeout, + switch_logdirty_timeout, 10 * 1000); + if (rc) goto out; + + qmp->ao = ao; + qmp->domid = domid; + qmp->payload_fd = -1; + qmp->callback = switch_qemu_xen_logdirty_done; + libxl__qmp_param_add_bool(gc, &args, "enable", enable); + rc = libxl__ev_qmp_send(gc, qmp, "xen-set-global-dirty-log", args); + if (rc) goto out; + + return; +out: + switch_qemu_xen_logdirty_done(egc, qmp, NULL, rc); +} + +static void switch_qemu_xen_logdirty_done(libxl__egc *egc, + libxl__ev_qmp *qmp, + const libxl__json_object *r, + int rc) +{ + EGC_GC; + libxl__logdirty_switch *lds = CONTAINER_OF(qmp, *lds, qmp); - rc = libxl__qmp_set_global_dirty_log(gc, domid, enable); if (rc) - LOGD(ERROR, domid, + LOGD(ERROR, qmp->domid, "logdirty switch failed (rc=%d), abandoning suspend",rc); - - lds->callback(egc, lds, rc); + switch_logdirty_done(egc, lds, rc); } static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev, @@ -234,6 +266,7 @@ static void switch_logdirty_done(libxl__egc *egc, libxl__ev_xswatch_deregister(gc, &lds->watch); libxl__ev_time_deregister(gc, &lds->timeout); + libxl__ev_qmp_dispose(gc, &lds->qmp); lds->callback(egc, lds, rc); } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 7bd08032cf..effc1c5bf9 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1957,8 +1957,6 @@ _hidden int libxl__qmp_system_wakeup(libxl__gc *gc, int domid); _hidden int libxl__qmp_resume(libxl__gc *gc, int domid); /* Load current QEMU state from file. */ _hidden int libxl__qmp_restore(libxl__gc *gc, int domid, const char *filename); -/* Set dirty bitmap logging status */ -_hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable); /* Add a virtual CPU */ _hidden int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int index); /* Query the bitmap of CPUs */ @@ -3412,6 +3410,7 @@ typedef struct libxl__logdirty_switch { const char *ret_path; libxl__ev_xswatch watch; libxl__ev_time timeout; + libxl__ev_qmp qmp; } libxl__logdirty_switch; _hidden void libxl__logdirty_init(libxl__logdirty_switch *lds); diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c index 505e0e5469..f1529925ee 100644 --- a/tools/libxl/libxl_qmp.c +++ b/tools/libxl/libxl_qmp.c @@ -1108,16 +1108,6 @@ int libxl__qmp_resume(libxl__gc *gc, int domid) return qmp_run_command(gc, domid, "cont", NULL, NULL, NULL); } -int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable) -{ - libxl__json_object *args = NULL; - - libxl__qmp_param_add_bool(gc, &args, "enable", enable); - - return qmp_run_command(gc, domid, "xen-set-global-dirty-log", args, - NULL, NULL); -} - int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int idx) { libxl__json_object *args = NULL;
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- tools/libxl/libxl_dom_save.c | 41 ++++++++++++++++++++++++++++++++---- tools/libxl/libxl_internal.h | 3 +-- tools/libxl/libxl_qmp.c | 10 --------- 3 files changed, 38 insertions(+), 16 deletions(-)