Re: [RFC PATCH] migration: reduce time of loading non-iterable vmstate

[Peter Xu <peterx@redhat.com>]

On Fri, Nov 18, 2022 at 04:36:48PM +0800, Chuang Xu wrote:
> The duration of loading non-iterable vmstate accounts for a significant
> portion of downtime (starting with the timestamp of source qemu stop and
> ending with the timestamp of target qemu start). Most of the time is spent
> committing memory region changes repeatedly.
> 
> This patch packs all the changes to memory region during the period of
> loading non-iterable vmstate in a single memory transaction. With the
> increase of devices, this patch will greatly improve the performance.
> 
> Here are the test results:
> test vm info:
> - 32 CPUs 128GB RAM
> - 8 16-queue vhost-net device
> - 16 4-queue vhost-user-blk device.
> 
> 	time of loading non-iterable vmstate
> before		about 210 ms
> after		about 40 ms
> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>

This is an interesting idea..  I think it means at least the address space
operations will all be messed up if happening during the precopy loading
progress, but I don't directly see its happening either.  For example, in
most post_load()s of vmsd I think the devices should just write directly to
its buffers, accessing MRs directly, even if they want DMAs or just update
fields to correct states.  Even so, I'm not super confident that holds
true, not to mention any other side effects (e.g., would we release bql
during precopy for any reason?).

Copy Paolo and PeterM for some extra eyes.

> ---
>  migration/migration.c | 1 +
>  migration/migration.h | 2 ++
>  migration/savevm.c    | 8 ++++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index e6f8bc2478..ed20704552 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -224,6 +224,7 @@ void migration_object_init(void)
>      qemu_sem_init(&current_incoming->postcopy_pause_sem_fast_load, 0);
>      qemu_mutex_init(&current_incoming->page_request_mutex);
>      current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
> +    current_incoming->start_pack_mr_change = false;
>  
>      migration_object_check(current_migration, &error_fatal);
>  
> diff --git a/migration/migration.h b/migration/migration.h
> index 58b245b138..86597f5feb 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -186,6 +186,8 @@ struct MigrationIncomingState {
>       * contains valid information.
>       */
>      QemuMutex page_request_mutex;
> +
> +    bool start_pack_mr_change;
>  };
>  
>  MigrationIncomingState *migration_incoming_get_current(void);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 48e85c052c..a073009a74 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2630,6 +2630,12 @@ retry:
>          switch (section_type) {
>          case QEMU_VM_SECTION_START:
>          case QEMU_VM_SECTION_FULL:
> +            /* call memory_region_transaction_begin() before loading non-iterable vmstate */
> +            if (section_type == QEMU_VM_SECTION_FULL && !mis->start_pack_mr_change) {
> +                memory_region_transaction_begin();
> +                mis->start_pack_mr_change = true;

This is slightly hacky to me.  Can we just wrap the begin/commit inside the
whole qemu_loadvm_state_main() call?

> +            }
> +
>              ret = qemu_loadvm_section_start_full(f, mis);
>              if (ret < 0) {
>                  goto out;
> @@ -2650,6 +2656,8 @@ retry:
>              }
>              break;
>          case QEMU_VM_EOF:
> +            /* call memory_region_transaction_commit() after loading non-iterable vmstate */
> +            memory_region_transaction_commit();
>              /* This is the end of migration */
>              goto out;
>          default:
> -- 
> 2.20.1
> 

-- 
Peter Xu