Re: [RFC PATCH COLO v5 02/29] Refactor domain_suspend_callback_common()

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

On 04/09/2015 02:11 AM, Wei Liu wrote:
> On Wed, Apr 01, 2015 at 02:41:38PM +0800, Yang Hongyang wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> libxl__domain_suspend() is to save the guest. I think
>> we should call it libxl__domain_save(), but I don't
>> rename it.
>>
> 
> FWIW this is not public API so we have certain degree of liberty to
> rename it if the new name is deemed more appropriate.

OK

> 
>> Secondary vm is running in colo mode. So we will do
>> the following things again and again:
>> 1. suspend both primay vm and secondary vm
>> 2. sync the state
>> 3. resume both primary vm and secondary vm
>> To suspend secondary vm, we need an independent API to
>> suspend vm.
>>
> [...]
>>  
>> +/*
>> + * libxl__domain_suspend_state is for saving guest, not
>> + * for suspending guest. We need to an independent API
>> + * to suspend guest only.
>> + */
>> +struct libxl__domain_suspend_state2 {
>> +    /* set by caller of libxl__domain_suspend2 */
>> +    libxl__ao *ao;
>> +
>> +    uint32_t domid;
>> +    libxl__ev_evtchn guest_evtchn;;
>> +    int guest_evtchn_lockfd;
>> +    int hvm;
>> +    const char *dm_savefile;
>> +    void (*callback_common_done)(libxl__egc*,
>> +                                 libxl__domain_suspend_state2*, int ok);
>> +    int save_dm;
>> +    int guest_responded;
>> +    libxl__xswait_state pvcontrol;
>> +    libxl__ev_xswatch guest_watch;
>> +    libxl__ev_time guest_timeout;
>> +};
>> +
>>  struct libxl__domain_suspend_state {
>>      /* set by caller of libxl__domain_suspend */
>>      libxl__ao *ao;
>> @@ -2827,22 +2851,14 @@ struct libxl__domain_suspend_state {
>>      int debug;
>>      const libxl_domain_remus_info *remus;
>>      /* private */
>> -    libxl__ev_evtchn guest_evtchn;
>> -    int guest_evtchn_lockfd;
>> +    libxl__domain_suspend_state2 dss2;
>>      int hvm;
>>      int xcflags;
>> -    int guest_responded;
>> -    libxl__xswait_state pvcontrol;
>> -    libxl__ev_xswatch guest_watch;
>> -    libxl__ev_time guest_timeout;
>> -    const char *dm_savefile;
>>      libxl__remus_devices_state rds;
>>      libxl__ev_time checkpoint_timeout; /* used for Remus checkpoint */
>>      int interval; /* checkpoint interval (for Remus) */
>>      libxl__save_helper_state shs;
>>      libxl__logdirty_switch logdirty;
>> -    void (*callback_common_done)(libxl__egc*,
>> -                                 struct libxl__domain_suspend_state*, int ok);
>>      /* private for libxl__domain_save_device_model */
>>      libxl__save_device_model_cb *save_dm_callback;
>>      libxl__datacopier_state save_dm_datacopier;
> 
> You moved dm_savefile to new struct but not these two fields, why?
> Since your suspend2 function is just a wrapper around
> domain_suspend_callback_common, it doesn't seem to need access to
> dm_savefile.

Hmm, there are two operations:
1. save qemu state to dm_savefile
2. transfer qemu state to dm_savefile

The function libxl__domain_suspend_device_model() does operation 1, and
the function libxl__domain_save_device_model() does operation 2.

The function libxl__domain_suspend_device_model() is called when we suspend
the guest, and it only uses the filed dm_savefile.

I think we should split it out: save qemu state to dm_savefile if the caller
needs to do it.

Thanks
Wen Congyang

> 
> Wei.
> 
>> @@ -3116,6 +3132,9 @@ struct libxl__domain_create_state {
>>  
>>  /*----- Domain suspend (save) functions -----*/
>>  
>> +/* calls dss2->callback_common_done when done */
>> +_hidden void libxl__domain_suspend2(libxl__egc *egc,
>> +                                    libxl__domain_suspend_state2 *dss2);
>>  /* calls dss->callback when done */
>>  _hidden void libxl__domain_suspend(libxl__egc *egc,
>>                                     libxl__domain_suspend_state *dss);
>> @@ -3155,7 +3174,7 @@ _hidden void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
>>  
>>  /* Each time the dm needs to be saved, we must call suspend and then save */
>>  _hidden int libxl__domain_suspend_device_model(libxl__gc *gc,
>> -                                           libxl__domain_suspend_state *dss);
>> +                                           libxl__domain_suspend_state2 *dss2);
>>  _hidden void libxl__domain_save_device_model(libxl__egc *egc,
>>                                       libxl__domain_suspend_state *dss,
>>                                       libxl__save_device_model_cb *callback);
>> -- 
>> 1.9.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> .
>