On Tuesday, April 2, 2024 2:56 PM, Wang, Lei4 wrote:
> On 4/2/2024 0:13, Peter Xu wrote:> On Fri, Mar 29, 2024 at 08:54:07AM +0000,
> Wang, Wei W wrote:
> >> On Friday, March 29, 2024 11:32 AM, Wang, Lei4 wrote:
> >>> When using the post-copy preemption feature to perform post-copy
> >>> live migration, the below scenario could lead to a deadlock and the
> >>> migration will never finish:
> >>>
> >>> - Source connect() the preemption channel in postcopy_start().
> >>> - Source and the destination side TCP stack finished the 3-way handshake
> >>> thus the connection is successful.
> >>> - The destination side main thread is handling the loading of the bulk
> RAM
> >>> pages thus it doesn't start to handle the pending connection event in
> >>> the
> >>> event loop. and doesn't post the semaphore
> postcopy_qemufile_dst_done for
> >>> the preemption thread.
> >>> - The source side sends non-iterative device states, such as the virtio
> >>> states.
> >>> - The destination main thread starts to receive the virtio states, this
> >>> process may lead to a page fault (e.g.,
> >>> virtio_load()->vring_avail_idx()
> >>> may trigger a page fault since the avail ring page may not be received
> >>> yet).
> >
> > Ouch. Yeah I think this part got overlooked when working on the
> > preempt channel.
> >
> >>> - The page request is sent back to the source side. Source sends the page
> >>> content to the destination side preemption thread.
> >>> - Since the event is not arrived and the semaphore
> >>> postcopy_qemufile_dst_done is not posted, the preemption thread in
> >>> destination side is blocked, and cannot handle receiving the page.
> >>> - The QEMU main load thread on the destination side is stuck at the page
> >>> fault, and cannot yield and handle the connect() event for the
> >>> preemption channel to unblock the preemption thread.
> >>> - The postcopy will stuck there forever since this is a deadlock.
> >>>
> >>> The key point to reproduce this bug is that the source side is
> >>> sending pages at a rate faster than the destination handling,
> >>> otherwise, the qemu_get_be64() in
> >>> ram_load_precopy() will have a chance to yield since at that time
> >>> there are no pending data in the buffer to get. This will make this
> >>> bug harder to be reproduced.
> >
> > How hard would this reproduce?
>
> We can manually make this easier to reproduce by adding the following code
> to make the destination busier to load the pages:
>
> diff --git a/migration/ram.c b/migration/ram.c index 0ad9fbba48..0b42877e1f
> 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4232,6 +4232,7 @@ static int ram_load_precopy(QEMUFile *f) {
> MigrationIncomingState *mis = migration_incoming_get_current();
> int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0;
> + volatile unsigned long long a;
>
> if (!migrate_compress()) {
> invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE; @@ -4347,6
> +4348,7 @@ static int ram_load_precopy(QEMUFile *f)
> break;
>
> case RAM_SAVE_FLAG_PAGE:
> + for (a = 0; a < 100000000; a++);
> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> break;
>
Which version of QEMU are you using?
I tried with the latest upstream QEMU (e.g. v8.2.0 release, 1600b9f46b1bd), it's
always reproducible without any changes (with local migration tests).
> >
> > I'm thinking whether this should be 9.0 material or 9.1. It's pretty
> > late for 9.0 though, but we can still discuss.
> >
> >>>
> >>> Fix this by yielding the load coroutine when receiving
> >>> MIG_CMD_POSTCOPY_LISTEN so the main event loop can handle the
> >>> connection event before loading the non-iterative devices state to
> >>> avoid the deadlock condition.
> >>>
> >>> Signed-off-by: Lei Wang <lei4.w...@intel.com>
> >>
> >> This seems to be a regression issue caused by this commit:
> >> 737840e2c6ea (migration: Use the number of transferred bytes
> >> directly)
> >>
> >> Adding qemu_fflush back to migration_rate_exceeded() or
> >> ram_save_iterate seems to work (might not be a good fix though).
> >>
> >>> ---
> >>> migration/savevm.c | 5 +++++
> >>> 1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/migration/savevm.c b/migration/savevm.c index
> >>> e386c5267f..8fd4dc92f2 100644
> >>> --- a/migration/savevm.c
> >>> +++ b/migration/savevm.c
> >>> @@ -2445,6 +2445,11 @@ static int
> loadvm_process_command(QEMUFile *f)
> >>> return loadvm_postcopy_handle_advise(mis, len);
> >>>
> >>> case MIG_CMD_POSTCOPY_LISTEN:
> >>> + if (migrate_postcopy_preempt() && qemu_in_coroutine()) {
> >>> + aio_co_schedule(qemu_get_current_aio_context(),
> >>> + qemu_coroutine_self());
> >>> + qemu_coroutine_yield();
> >>> + }
> >>
> >> The above could be moved to loadvm_postcopy_handle_listen().
> >
> > I'm not 100% sure such thing (no matter here or moved into it, which
> > does look cleaner) would work for us.
> >
> > The problem is I still don't yet see an ordering restricted on top of
> > (1)
> > accept() happens, and (2) receive LISTEN cmd here. What happens if
> > the
> > accept() request is not yet received when reaching LISTEN? Or is it
> > always guaranteed the accept(fd) will always be polled here?
> >
> > For example, the source QEMU (no matter pre-7.2 or later) will always
> > setup the preempt channel asynchrounously, then IIUC it can connect()
> > after sending the whole chunk of packed data which should include this
> > LISTEN. I think it means it's not guaranteed this will 100% work, but
> > maybe further reduce the possibility of the race.
>
> I think the following code:
>
> postcopy_start() ->
> postcopy_preempt_establish_channel() ->
> qemu_sem_wait(&s->postcopy_qemufile_src_sem);
>
> can guarantee that the connect() syscall is successful so the destination side
> receives the connect() request before it loads the LISTEN command, otherwise
> it won't post the sem:
>
> postcopy_preempt_send_channel_new() ->
> postcopy_preempt_send_channel_done() ->
> qemu_sem_post(&s->postcopy_qemufile_src_sem);
>
Yes. But as mentioned in another thread, connect() and accept() are async.
So in theory accept() could still come later after the LISTEN command.
> >
> > One right fix that I can think of is moving the sem_wait(&done) into
> > the main thread too, so we wait for the sem _before_ reading the
> > packed data, so there's no chance of fault. However I don't think
> > sem_wait() will be smart enough to yield when in a coroutine.. In the
> > long term run I think we should really make migration loadvm to do
> > work in the thread rather than the main thread. I think it means we
> > have one more example to be listed in this todo so that's preferred..
> >
> > https://wiki.qemu.org/ToDo/LiveMigration#Create_a_thread_for_migration
> > _destination
> >
> > I attached such draft patch below, but I'm not sure it'll work. Let
> > me know how both of you think about it.
>
> Sadly it doesn't work, there is an unknown segfault.
>
> >
> >>
> >> Another option is to follow the old way (i.e. pre_7_2) to do
> >> postcopy_preempt_setup in migrate_fd_connect. This can save the above
> >> overhead of switching to the main thread during the downtime. Seems
> >> Peter's previous patch already solved the channel disordering issue. Let's
> see Peter and others' opinions.
> >
> > IIUC we still need that pre_7_2 stuff and keep the postponed connect()
> > to make sure the ordering is done properly. Wei, could you elaborate
> > the patch you mentioned? Maybe I missed some spots.
> >
> > You raised a good point that this may introduce higher downtime. Did
> > you or Lei tried to measure how large it is? If that is too high, we
> > may need to think another solution, e.g., wait the channel connection
> > before vm stop happens.
>
> Per my very simple test, using post-copy preemption to live migrate an 8G VM:
>
> w/o this patch: 121ms in avg in 5 tries
> w/ this patch: 115ms in avg in 5 tries
>
> So it seems the overhead introduced is not too high (maybe ignorable?).
You could just measure the time for the added qemu_coroutine_yield() part.
The time will depend on how many events happen to be there waiting for a
dispatch.
