Re: [Qemu-devel] [PATCH 2/5] migration: Fix a potential issue

[Juan Quintela <quintela@redhat.com>]

Liang Li <liang.z.li@intel.com> wrote:
> At the end of live migration and before vm_start() on the destination
> side, we should make sure all the decompression tasks are finished, if
> this can not be guaranteed, the VM may get the incorrect memory data,
> or the updated memory may be overwritten by the decompression thread.
> Add the code to fix this potential issue.
>
> Suggested-by: David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  include/migration/migration.h |  1 +
>  migration/migration.c         |  2 +-
>  migration/ram.c               | 20 ++++++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ac2c12c..1c9051e 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -223,6 +223,7 @@ void migrate_compress_threads_create(void);
>  void migrate_compress_threads_join(void);
>  void migrate_decompress_threads_create(void);
>  void migrate_decompress_threads_join(void);
> +void wait_for_decompress_done(void);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_transferred(void);
>  uint64_t ram_bytes_total(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 991313a..5228c28 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -347,7 +347,7 @@ static void process_incoming_migration_bh(void *opaque)
>      /* If global state section was not received or we are in running
>         state, we need to obey autostart. Any other state is set with
>         runstate_set. */
> -
> +    wait_for_decompress_done();
>      if (!global_state_received() ||
>          global_state_get_runstate() == RUN_STATE_RUNNING) {
>          if (autostart) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 7ab6ab5..4459b38 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2220,6 +2220,26 @@ static void *do_data_decompress(void *opaque)
>      return NULL;
>  }
>  

why?

> +void wait_for_decompress_done(void)
> +{
> +    int idx, thread_count;
> +
> +    if (!migrate_use_compression()) {
> +        return;
> +    }
> +    thread_count = migrate_decompress_threads();
> +    for (idx = 0; idx < thread_count; idx++) {
> +        if (!decomp_param[idx].done) {
> +            qemu_mutex_lock(&decomp_done_lock);
> +            while (!decomp_param[idx].done) {
> +                qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> +            }
> +            qemu_mutex_unlock(&decomp_done_lock);
> +        }
> +    }
> +
> +}
> +

    thread_count = migrate_decompress_threads();
    qemu_mutex_lock(&decomp_done_lock);
    for (idx = 0; idx < thread_count; idx++) {
       while (!decomp_param[idx].done) {
           qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
       }
    }
    qemu_mutex_unlock(&decomp_done_lock);

Simpler and correct, no?

Later, Juan.