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

[Chuang Xu]

On 2022/11/25 上午12:40, Peter Xu wrote:

> 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
>     
Sorry, I don't quite understand the meaning of "messed up" here.. Maybe I need 
more information about how the address space operations will be messed up.
> 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
>     
And I'm not sure whether the "its happening" means "begin/commit happening" 
or "messed up happening"? If it's the former, Here are what I observe: 
the stage of loading iterable vmstate doesn't call begin/commit, but the 
stage of loading noniterable vmstate calls a large amount of begin/commit 
in field->info->get() operation. For example:

#0  memory_region_transaction_commit () at ../softmmu/memory.c:1085
#1  0x0000559b6f683523 in pci_update_mappings (d=d@entry=0x7f5cd8682010) at ../hw/pci/pci.c:1361
#2  0x0000559b6f683a1f in get_pci_config_device (f=<optimized out>, pv=0x7f5cd86820a0, size=256, field=<optimized out>) at ../hw/pci/pci.c:545
#3  0x0000559b6f5fcd86 in vmstate_load_state (f=f@entry=0x559b757eb4b0, vmsd=vmsd@entry=0x559b70909d40 <vmstate_pci_device>, opaque=opaque@entry=0x7f5cd8682010, version_id=2)
    at ../migration/vmstate.c:143
#4  0x0000559b6f68466f in pci_device_load (s=s@entry=0x7f5cd8682010, f=f@entry=0x559b757eb4b0) at ../hw/pci/pci.c:664
#5  0x0000559b6f6ad38a in virtio_pci_load_config (d=0x7f5cd8682010, f=0x559b757eb4b0) at ../hw/virtio/virtio-pci.c:181
#6  0x0000559b6f7dfe91 in virtio_load (vdev=0x7f5cd868a1a0, f=0x559b757eb4b0, version_id=1) at ../hw/virtio/virtio.c:3071
#7  0x0000559b6f5fcd86 in vmstate_load_state (f=f@entry=0x559b757eb4b0, vmsd=0x559b709ae260 <vmstate_vhost_user_blk>, opaque=0x7f5cd868a1a0, version_id=1) at ../migration/vmstate.c:143
#8  0x0000559b6f62da48 in vmstate_load (f=0x559b757eb4b0, se=0x559b7591c010) at ../migration/savevm.c:913
#9  0x0000559b6f631334 in qemu_loadvm_section_start_full (mis=0x559b73f1a580, f=0x559b757eb4b0) at ../migration/savevm.c:2741
#10 qemu_loadvm_state_main (f=f@entry=0x559b757eb4b0, mis=mis@entry=0x559b73f1a580) at ../migration/savevm.c:2937
#11 0x0000559b6f632faa in qemu_loadvm_state (f=0x559b757eb4b0) at ../migration/savevm.c:3018
#12 0x0000559b6f6d2ece in process_incoming_migration_co (opaque=<optimized out>) at ../migration/migration.c:574
#13 0x0000559b6f9f9f0b in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at ../util/coroutine-ucontext.c:173
#14 0x00007f5cfeecf000 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#15 0x00007fff04a2e8f0 in ?? ()
#16 0x0000000000000000 in ?? ()
> 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.
>
>     
What I observe is that during the loading process, migration thread will call Condwait to 
wait for the vcpu threads to complete tasks, such as kvm_apic_post_load, and rcu thread 
will acquire the bql to do the flatview_destroy operation. So far, I haven't seen the 
side effects of these two situations.
> > ---
> >  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?
>     
The iterative copy phase doesn't call begin/commit, so There seems to be no essential 
difference between these two codes. I did try to wrap the begin/commit inside the whole 
qemu_loadvm_state_main() call, this way also worked well. 
But only calling begin/commit before/after the period of loading non-iterable vmstate may
have less unkown side effect?
> > +            }
> > +
> >              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, Thanks a lot for your advice! Hope for more suggestions from you!