Re: [PATCH v6 09/18] tools/libxl: introduce libxl__domain_common_switch_qemu_logdirty()

[Wen Congyang <wency@cn.fujitsu.com>]

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 <hongyang.yang@easystack.cn>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> ---
>>  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
> 
> 
> .
>