Re: [Qemu-devel] [PATCH COLO-Frame v13 32/39] COLO: Separate the process of saving/loading ram and device state

[Dr. David Alan Gilbert]

* zhanghailiang (address@hidden) wrote:
> We separate the process of saving/loading ram and device state when do 
> checkpoint,
> we add new helpers for save/load ram/device. With this change, we can directly
> transfer ram from master to slave without using QEMUSizeBuffer as assistant,
> which also reduce the size of extra memory been used during checkpoint.
> 
> Besides, we move the colo_flush_ram_cache to the proper position after the
> above change.
> 
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Li Zhijian <address@hidden>
> ---
> v13:
> - Re-use some existed helper functions to realize saving/loading ram and 
> device.
> v11:
> - Remove load configuration section in qemu_loadvm_state_begin()
> ---
>  include/sysemu/sysemu.h |  6 +++++
>  migration/colo.c        | 47 ++++++++++++++++++++++++++---------
>  migration/ram.c         |  5 ----
>  migration/savevm.c      | 66 
> ++++++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 104 insertions(+), 20 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 91eeda3..f553f8a 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -133,7 +133,13 @@ void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, 
> const char *name,
>                                             uint64_t *start_list,
>                                             uint64_t *length_list);
>  
> +void qemu_savevm_live_state(QEMUFile *f);
> +int qemu_save_device_state(QEMUFile *f);
> +
>  int qemu_loadvm_state(QEMUFile *f);
> +int qemu_loadvm_state_begin(QEMUFile *f);
> +int qemu_load_ram_state(QEMUFile *f);
> +int qemu_load_device_state(QEMUFile *f);
>  
>  typedef enum DisplayType
>  {
> diff --git a/migration/colo.c b/migration/colo.c
> index fd87f34..4a99819 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -290,21 +290,34 @@ static int 
> colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
>  
> +    colo_put_cmd(s->to_dst_file, COLO_COMMAND_VMSTATE_SEND, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
>      /* Disable block migration */
>      s->params.blk = 0;
>      s->params.shared = 0;
> -    qemu_savevm_state_header(trans);
> -    qemu_savevm_state_begin(trans, &s->params);
> -    qemu_mutex_lock_iothread();
> -    qemu_savevm_state_complete_precopy(trans, false);
> -    qemu_mutex_unlock_iothread();
> -
> -    qemu_fflush(trans);
> +    qemu_savevm_state_begin(s->to_dst_file, &s->params);
> +    ret = qemu_file_get_error(s->to_dst_file);
> +    if (ret < 0) {
> +        error_report("save vm state begin error");
> +        goto out;
> +    }
>  
> -    colo_put_cmd(s->to_dst_file, COLO_COMMAND_VMSTATE_SEND, &local_err);
> -    if (local_err) {


> +    qemu_mutex_lock_iothread();
> +    /* Only save VM's live state, which not including device state */
> +    qemu_savevm_live_state(s->to_dst_file);

I'm a little worried that this is done with the lock held;
if the destination hung during this point and there was
enough data to fill the network buffers, couldn't we hang
and then we wouldn't be able to issue the colo_lost_heartbeat
command?

> +    /* Note: device state is saved into buffer */
> +    ret = qemu_save_device_state(trans);
> +    if (ret < 0) {
> +        error_report("save device state error\n");
> +        qemu_mutex_unlock_iothread();
>          goto out;
>      }
> +    qemu_fflush(trans);
> +    qemu_mutex_unlock_iothread();

You can save an unlock by doing:
   ret = ...
   qemu_mutex_unlock_iothread();
   if (ret < 0) {


>      /* we send the total size of the vmstate first */
>      size = qsb_get_length(buffer);
>      colo_put_cmd_value(s->to_dst_file, COLO_COMMAND_VMSTATE_SIZE,
> @@ -573,6 +586,16 @@ void *colo_process_incoming_thread(void *opaque)
>              goto out;
>          }
>  
> +        ret = qemu_loadvm_state_begin(mis->from_src_file);
> +        if (ret < 0) {
> +            error_report("load vm state begin error, ret=%d", ret);
> +            goto out;
> +        }
> +        ret = qemu_load_ram_state(mis->from_src_file);
> +        if (ret < 0) {
> +            error_report("load ram state error");
> +            goto out;
> +        }
>          /* read the VM state total size first */
>          value = colo_get_cmd_value(mis->from_src_file,
>                                   COLO_COMMAND_VMSTATE_SIZE, &local_err);
> @@ -605,8 +628,10 @@ void *colo_process_incoming_thread(void *opaque)
>          qemu_mutex_lock_iothread();
>          qemu_system_reset(VMRESET_SILENT);
>          vmstate_loading = true;
> -        if (qemu_loadvm_state(fb) < 0) {
> -            error_report("COLO: loadvm failed");
> +        colo_flush_ram_cache();
> +        ret = qemu_load_device_state(fb);
> +        if (ret < 0) {
> +            error_report("COLO: load device state failed\n");

Unneeded \n

>              vmstate_loading = false;
>              qemu_mutex_unlock_iothread();
>              goto out;
> diff --git a/migration/ram.c b/migration/ram.c
> index 8ff7f7c..45d9332 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2458,7 +2458,6 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>       * be atomic
>       */
>      bool postcopy_running = postcopy_state_get() >= 
> POSTCOPY_INCOMING_LISTENING;
> -    bool need_flush = false;
>  
>      seq_iter++;
>  
> @@ -2493,7 +2492,6 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>              /* After going into COLO, we should load the Page into 
> colo_cache */
>              if (ram_cache_enable) {
>                  host = colo_cache_from_block_offset(block, addr);
> -                need_flush = true;
>              } else {
>                  host = host_from_ram_block_offset(block, addr);
>              }
> @@ -2588,9 +2586,6 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>      rcu_read_unlock();
>  
> -    if (!ret  && ram_cache_enable && need_flush) {
> -        colo_flush_ram_cache();
> -    }
>      DPRINTF("Completed load of VM with exit code %d seq iteration "
>              "%" PRIu64 "\n", ret, seq_iter);
>      return ret;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7ceec2d..7892070 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -50,6 +50,7 @@
>  #include "qemu/iov.h"
>  #include "block/snapshot.h"
>  #include "block/qapi.h"
> +#include "migration/colo.h"
>  
>  
>  #ifndef ETH_P_RARP
> @@ -923,6 +924,10 @@ void qemu_savevm_state_begin(QEMUFile *f,
>              break;
>          }
>      }
> +    if (migration_in_colo_state()) {
> +        qemu_put_byte(f, QEMU_VM_EOF);
> +        qemu_fflush(f);
> +    }
>  }
>  
>  /*
> @@ -1192,13 +1197,20 @@ static int qemu_savevm_state(QEMUFile *f, Error 
> **errp)
>      return ret;
>  }
>  
> -static int qemu_save_device_state(QEMUFile *f)
> +void qemu_savevm_live_state(QEMUFile *f)
>  {
> -    SaveStateEntry *se;
> +    /* save QEMU_VM_SECTION_END section */
> +    qemu_savevm_state_complete_precopy(f, true);
> +    qemu_put_byte(f, QEMU_VM_EOF);
> +}
>  
> -    qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
> -    qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> +int qemu_save_device_state(QEMUFile *f)
> +{
> +    SaveStateEntry *se;
>  
> +    if (!migration_in_colo_state()) {
> +        qemu_savevm_state_header(f);
> +    }
>      cpu_synchronize_all_states();
>  
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> @@ -1957,6 +1969,52 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> +int qemu_loadvm_state_begin(QEMUFile *f)
> +{
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    if (qemu_savevm_state_blocked(&local_err)) {
> +        error_report_err(local_err);
> +        return -EINVAL;
> +    }
> +    /* Load QEMU_VM_SECTION_START section */
> +    ret = qemu_loadvm_state_main(f, mis);
> +    if (ret < 0) {
> +        error_report("Failed to loadvm begin work: %d", ret);
> +    }
> +    return ret;
> +}
> +
> +int qemu_load_ram_state(QEMUFile *f)
> +{
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    int ret;
> +
> +    ret = qemu_loadvm_state_main(f, mis);
> +    if (ret < 0) {
> +        error_report("Failed to load ram state: %d", ret);
> +    }
> +    return ret;
> +}

This function does very little; why not just make qemu_loadvm_state_main
non-static and call that directly?

Dave

> +int qemu_load_device_state(QEMUFile *f)
> +{
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    int ret;
> +    
> +    /* Load QEMU_VM_SECTION_FULL section */
> +    ret = qemu_loadvm_state_main(f, mis);
> +    if (ret < 0) {
> +        error_report("Failed to load device state: %d", ret);
> +        return ret;
> +    }
> +
> +    cpu_synchronize_all_post_init();
> +    return 0;
> +}
> +
>  void hmp_savevm(Monitor *mon, const QDict *qdict)
>  {
>      BlockDriverState *bs, *bs1;
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK