From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wen Congyang Subject: Re: [PATCH v6 09/18] tools/libxl: introduce libxl__domain_common_switch_qemu_logdirty() Date: Tue, 26 Jan 2016 15:04:39 +0800 Message-ID: <56A71A87.5010007@cn.fujitsu.com> References: <1451442548-26974-1-git-send-email-wency@cn.fujitsu.com> <1451442548-26974-10-git-send-email-wency@cn.fujitsu.com> <20160125185910.GO14977@char.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160125185910.GO14977@char.us.oracle.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: Konrad Rzeszutek Wilk Cc: Lars Kurth , Changlong Xie , Wei Liu , Ian Campbell , Andrew Cooper , Jiang Yunhong , Ian Jackson , xen devel , Dong Eddie , Gui Jianfeng , Shriram Rajagopalan , Yang Hongyang List-Id: xen-devel@lists.xenproject.org On 01/26/2016 02:59 AM, Konrad Rzeszutek Wilk wrote: > On Wed, Dec 30, 2015 at 10:28:59AM +0800, Wen Congyang wrote: >> Secondary vm is running in colo mode, we need to send >> secondary vm's dirty page information to master at checkpoint, > > In previous patch you called it primary, so perhaps: > s/master/primary/ ? > >> so we have to enable qemu logdirty on secondary. >> >> libxl__domain_suspend_common_switch_qemu_logdirty() is to enable >> qemu logdirty. But it uses domain_save_state, and calls > > s/domain_save_state/libxl__domain_save_state/ >> libxl__xc_domain_saverestore_async_callback_done() >> before exits. This can not be used for secondary vm. >> >> Update libxl__domain_suspend_common_switch_qemu_logdirty() to >> introduce a new API libxl__domain_common_switch_qemu_logdirty(). >> This API only uses libxl__logdirty_switch, and calls >> lds->callback before exits. > > One question - that perhaps had been part of the review earlier > (if so it may be good to include this in the description > so I don't ask silly questions): > > Why add this extra API? You could squash libxl__domain_suspend_common_switch_qemu_logdirty > and libxl__domain_common_switch_qemu_logdirty code together > and call it libxl_domain_common_and_suspend_common_switch_qemu_logdirty > (ok, just kidding on the name). But - why not have one function > instead of splitting the functionality in two? Do you mean that auto switch qemu logdirty when suspend the guest? Thanks Wen Congyang > > Is there another patch that depends on it? If so it may be good > to spell it out, like: > > Patch blah blah is going to use it. > > Thanks! >> >> Signed-off-by: Yang Hongyang >> Signed-off-by: Wen Congyang >> CC: Andrew Cooper >> Acked-by: Ian Campbell >> --- >> tools/libxl/libxl_dom_save.c | 95 ++++++++++++++++++++++++-------------------- >> tools/libxl/libxl_internal.h | 8 ++++ >> 2 files changed, 60 insertions(+), 43 deletions(-) >> >> diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c >> index b3ecad7..79e43f1 100644 >> --- a/tools/libxl/libxl_dom_save.c >> +++ b/tools/libxl/libxl_dom_save.c >> @@ -42,7 +42,7 @@ static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev, >> static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch*, >> const char *watch_path, const char *event_path); >> static void switch_logdirty_done(libxl__egc *egc, >> - libxl__domain_save_state *dss, int rc); >> + libxl__logdirty_switch *lds, int rc); >> >> static void logdirty_init(libxl__logdirty_switch *lds) >> { >> @@ -52,13 +52,10 @@ static void logdirty_init(libxl__logdirty_switch *lds) >> } >> >> static void domain_suspend_switch_qemu_xen_traditional_logdirty >> - (int domid, unsigned enable, >> - libxl__save_helper_state *shs) >> + (libxl__egc *egc, int domid, unsigned enable, >> + libxl__logdirty_switch *lds) >> { >> - libxl__egc *egc = shs->egc; >> - libxl__domain_save_state *dss = shs->caller_state; >> - libxl__logdirty_switch *lds = &dss->logdirty; >> - STATE_AO_GC(dss->ao); >> + STATE_AO_GC(lds->ao); >> int rc; >> xs_transaction_t t = 0; >> const char *got; >> @@ -120,26 +117,34 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty >> out: >> LOG(ERROR,"logdirty switch failed (rc=%d), abandoning suspend",rc); >> libxl__xs_transaction_abort(gc, &t); >> - switch_logdirty_done(egc,dss,rc); >> + switch_logdirty_done(egc,lds,rc); >> } >> >> static void domain_suspend_switch_qemu_xen_logdirty >> - (int domid, unsigned enable, >> - libxl__save_helper_state *shs) >> + (libxl__egc *egc, int domid, unsigned enable, >> + libxl__logdirty_switch *lds) >> { >> - libxl__egc *egc = shs->egc; >> - libxl__domain_save_state *dss = shs->caller_state; >> - STATE_AO_GC(dss->ao); >> + STATE_AO_GC(lds->ao); >> int rc; >> >> rc = libxl__qmp_set_global_dirty_log(gc, domid, enable); >> - if (!rc) { >> - libxl__xc_domain_saverestore_async_callback_done(egc, shs, 0); >> - } else { >> + if (rc) >> LOG(ERROR,"logdirty switch failed (rc=%d), abandoning suspend",rc); >> + >> + lds->callback(egc, lds, rc); >> +} >> + >> +static void domain_suspend_switch_qemu_logdirty_done >> + (libxl__egc *egc, libxl__logdirty_switch *lds, int rc) >> +{ >> + libxl__domain_save_state *dss = CONTAINER_OF(lds, *dss, logdirty); >> + >> + if (rc) { >> dss->rc = rc; >> - libxl__xc_domain_saverestore_async_callback_done(egc, shs, -1); >> - } >> + libxl__xc_domain_saverestore_async_callback_done(egc, >> + &dss->sws.shs, -1); >> + } else >> + libxl__xc_domain_saverestore_async_callback_done(egc, &dss->sws.shs, 0); >> } >> >> void libxl__domain_suspend_common_switch_qemu_logdirty >> @@ -148,42 +153,52 @@ void libxl__domain_suspend_common_switch_qemu_logdirty >> libxl__save_helper_state *shs = user; >> libxl__egc *egc = shs->egc; >> libxl__domain_save_state *dss = shs->caller_state; >> - STATE_AO_GC(dss->ao); >> + >> + /* convenience aliases */ > > /* Convenience aliases. */ > >> + libxl__logdirty_switch *const lds = &dss->logdirty; >> + >> + lds->callback = domain_suspend_switch_qemu_logdirty_done; >> + libxl__domain_common_switch_qemu_logdirty(egc, domid, enable, lds); >> +} >> + >> +void libxl__domain_common_switch_qemu_logdirty(libxl__egc *egc, >> + int domid, unsigned enable, >> + libxl__logdirty_switch *lds) >> +{ >> + STATE_AO_GC(lds->ao); >> >> switch (libxl__device_model_version_running(gc, domid)) { >> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: >> - domain_suspend_switch_qemu_xen_traditional_logdirty(domid, enable, shs); >> + domain_suspend_switch_qemu_xen_traditional_logdirty(egc, domid, enable, >> + lds); >> break; >> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: >> - domain_suspend_switch_qemu_xen_logdirty(domid, enable, shs); >> + domain_suspend_switch_qemu_xen_logdirty(egc, domid, enable, lds); >> break; >> case LIBXL_DEVICE_MODEL_VERSION_NONE: >> - libxl__xc_domain_saverestore_async_callback_done(egc, shs, 0); >> + lds->callback(egc, lds, 0); >> break; >> default: >> LOG(ERROR,"logdirty switch failed" >> ", no valid device model version found, abandoning suspend"); >> - dss->rc = ERROR_FAIL; >> - libxl__xc_domain_saverestore_async_callback_done(egc, shs, -1); >> + lds->callback(egc, lds, ERROR_FAIL); >> } >> } >> static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev, >> const struct timeval *requested_abs, >> int rc) >> { >> - libxl__domain_save_state *dss = CONTAINER_OF(ev, *dss, logdirty.timeout); >> - STATE_AO_GC(dss->ao); >> + libxl__logdirty_switch *lds = CONTAINER_OF(ev, *lds, timeout); >> + STATE_AO_GC(lds->ao); >> LOG(ERROR,"logdirty switch: wait for device model timed out"); >> - switch_logdirty_done(egc,dss,ERROR_FAIL); >> + switch_logdirty_done(egc,lds,ERROR_FAIL); >> } >> >> static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch *watch, >> const char *watch_path, const char *event_path) >> { >> - libxl__domain_save_state *dss = >> - CONTAINER_OF(watch, *dss, logdirty.watch); >> - libxl__logdirty_switch *lds = &dss->logdirty; >> - STATE_AO_GC(dss->ao); >> + libxl__logdirty_switch *lds = CONTAINER_OF(watch, *lds, watch); >> + STATE_AO_GC(lds->ao); >> const char *got; >> xs_transaction_t t = 0; >> int rc; >> @@ -229,28 +244,20 @@ static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch *watch, >> if (rc <= 0) { >> if (rc < 0) >> LOG(ERROR,"logdirty switch: failed (rc=%d)",rc); >> - switch_logdirty_done(egc,dss,rc); >> + switch_logdirty_done(egc,lds,rc); >> } >> } >> >> static void switch_logdirty_done(libxl__egc *egc, >> - libxl__domain_save_state *dss, >> + libxl__logdirty_switch *lds, >> int rc) >> { >> - STATE_AO_GC(dss->ao); >> - libxl__logdirty_switch *lds = &dss->logdirty; >> + STATE_AO_GC(lds->ao); >> >> libxl__ev_xswatch_deregister(gc, &lds->watch); >> libxl__ev_time_deregister(gc, &lds->timeout); >> >> - int broke; >> - if (rc) { >> - broke = -1; >> - dss->rc = rc; >> - } else { >> - broke = 0; >> - } >> - libxl__xc_domain_saverestore_async_callback_done(egc, &dss->sws.shs, broke); >> + lds->callback(egc, lds, rc); >> } >> >> /*----- callbacks, called by xc_domain_save -----*/ >> @@ -346,6 +353,8 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_save_state *dss) >> >> dss->rc = 0; >> logdirty_init(&dss->logdirty); >> + dss->logdirty.ao = ao; >> + >> dsps->ao = ao; >> dsps->domid = domid; >> rc = libxl__domain_suspend_init(egc, dsps); >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index 4872619..552692f 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -3071,6 +3071,11 @@ libxl__stream_write_inuse(const libxl__stream_write_state *stream) >> } >> >> typedef struct libxl__logdirty_switch { >> + /* set by caller of libxl__domain_common_switch_qemu_logdirty */ > > s/set/Set/ > >> + libxl__ao *ao; >> + void (*callback)(libxl__egc *egc, struct libxl__logdirty_switch *lds, >> + int rc); >> + >> const char *cmd; >> const char *cmd_path; >> const char *ret_path; >> @@ -3490,6 +3495,9 @@ void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc, >> >> _hidden void libxl__domain_suspend_common_switch_qemu_logdirty >> (int domid, unsigned int enable, void *data); >> +_hidden void libxl__domain_common_switch_qemu_logdirty(libxl__egc *egc, >> + int domid, unsigned enable, >> + libxl__logdirty_switch *lds); >> _hidden int libxl__save_emulator_xenstore_data(libxl__domain_save_state *dss, >> char **buf, uint32_t *len); >> _hidden int libxl__restore_emulator_xenstore_data >> -- >> 2.5.0 >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > > > . >