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

[Peter Xu]

On Mon, Nov 28, 2022 at 05:42:43PM +0800, Chuang Xu wrote:
> 
> 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.

AFAIK the major thing we do during commit of memory regions is to apply the
memory region changes to the rest (flatviews, or ioeventfds), basically it
makes everything matching with the new memory region layout.

If we allow memory region commit to be postponed for the whole loading
process, it means at least from flat view pov any further things like:

  address_space_write(&address_space_memory, ...)

Could write to wrong places because the flat views are not updated.

> 
> > 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.

Yes that's something I'd worry about.

The current memory API should be defined as: when we release the bql we
should guarantee the memory layout is persistent and no pending
transactions.  I used to have a patchset just for that because when
violating that rule it's prone to very weird bugs:

https://lore.kernel.org/all/20210728183151.195139-8-peterx@redhat.com/

One example report that was caused by wrongly releasing bql and you can
have a feeling of it by the stack dumped (after above patchset applied):

https://lore.kernel.org/qemu-devel/CH0PR02MB7898BBD73D0F3F7D5003BB178BE19@CH0PR02MB7898.namprd02.prod.outlook.com/

Said that, it's not exact the case here since it's not releasing bql during
a memory commit phase, so probably no immediate problem as rcu thread will
just ignore any updates to be committed.  It might be safe to do it like
that (and making sure no vcpu is running), but worth serious thoughts.

As a start, maybe you can try with poison address_space_to_flatview() (by
e.g. checking the start_pack_mr_change flag and assert it is not set)
during this process to see whether any call stack can even try to
dereference a flatview.

It's just that I didn't figure a good way to "prove" its validity, even if
I think this is an interesting idea worth thinking to shrink the downtime.

> 
> > > ---
> > >   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?

I don't worry much on the iterative migration phase, because they should be
mostly pure data movements unless I miss something important.  Having them
wrap qemu_loadvm_state_main() can avoid the flag completely and avoid the
special treatment within these migration internal flags which is hacky, imo.

> 
> > 
> > > +            }
> > > +
> > >               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!

-- 
Peter Xu