Re: [PATCH 1/2] migration/multifd: Further remove the SYNC on complete

[Peter Xu]

On Thu, Dec 05, 2024 at 04:55:08PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Commit 637280aeb2 ("migration/multifd: Avoid the final FLUSH in
> > complete()") removed the FLUSH operation on complete() which should avoid
> > one global sync on destination side, because it's not needed.
> >
> > However that commit overlooked multifd_ram_flush_and_sync() part of things,
> > as that's always used together with the FLUSH message on the main channel.
> 
> Let's please write the full name of the flags, functions, etc. As we've
> seen in the discussion with Prasad, we're stumbling over ambiguous
> terminology. This is RAM_SAVE_FLAG_MULTIFD_FLUSH.

Sure.

> 
> >
> > For very old binaries (multifd_flush_after_each_section==true), that's
> > still needed because each EOS received on destination will enforce
> > all-channel sync once.
> 
> Ok, but why does this patch not reinstate the flag?

RAM_SAVE_FLAG_MULTIFD_FLUSH?  Because it's not needed?

> 
> >
> > For new binaries (multifd_flush_after_each_section==false), the flush and
> > sync shouldn't be needed just like the FLUSH, with the same reasoning.
> >
> > Currently, on new binaries we'll still send SYNC messages on multifd
> > channels, even if FLUSH is omitted at the end.  It'll make all recv threads
> > hang at SYNC message.
> 
> I don't get this part, is this a bug you're describing? There's not SYNC
> message on the recv side, I think you mean the MULTIFD_FLAG_SYNC and
> this code?
> 
>     if (flags & MULTIFD_FLAG_SYNC) {
>         qemu_sem_post(&multifd_recv_state->sem_sync);
>         qemu_sem_wait(&p->sem_sync);
>     }

Yes.

> 
> That's not a hang, that's the sync.

When recv side never collect that SYNC (by invoke multifd_recv_sync_main),
then it is a hang.

> 
> >
> > Multifd is still all working fine because luckily recv side cleanup
> > code (mostly multifd_recv_sync_main()) is smart enough to make sure even if
> > recv threads are stuck at SYNC it'll get kicked out.  And since this is the
> > completion phase of migration, nothing else will be sent after the SYNCs.
> 
> Hmm, a last sync on the recv side might indeed not be needed.
> 
> >
> > This may be needed for VFIO in the future because VFIO can have data to
> > push after ram_save_complete(), hence we don't want the recv thread to be
> > stuck in SYNC message. Remove this limitation will make src even slightly
> > faster too to avoid some more code.
> >
> > Stable branches do not need this patch, as no real bug I can think of that
> > will go wrong there.. so not attaching Fixes to be clear on the backport
> > not needed.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/ram.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 05ff9eb328..7284c34bd8 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3283,9 +3283,16 @@ static int ram_save_complete(QEMUFile *f, void 
> > *opaque)
> >          }
> >      }
> >  
> > -    ret = multifd_ram_flush_and_sync();
> > -    if (ret < 0) {
> > -        return ret;
> > +    if (migrate_multifd() &&
> > +        migrate_multifd_flush_after_each_section()) {
> > +        /*
> > +         * Only the old dest QEMU will need this sync, because each EOS
> > +         * will require one SYNC message on each channel.
> > +         */
> > +        ret = multifd_ram_flush_and_sync();
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> 
> I don't think this works. We need one last flush to not lose the last
> few pages of ram. And we need the src side sync of multifd threads to
> make sure this function doesn't return before all IO has been put on the
> wire.

This should be the question for commit 637280aeb2, I thought it got
answered there.. It's almost what the commit message there in 637280aeb2
wanted to justify too.

We don't need to flush the last pages here, because we flushed it already,
in the last find_dirty_block() call where src QEMU finished scanning the
last round.  Then we set complete_round=true, scan one more round, and the
iteration won't complete until the new round sees zero dirty page.

So when reaching this line, multifd send buffer must be empty.  We need to
flush for each round of RAM scan, we can't avoid the flush there, but we
can save this one in complete().

To explain that with code, I hacked a QEMU and assert that.  It ran all
fine here (this is definitely not for merge.. but to show what I meant):

===8<===
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index f64c4c9abd..0bd42c7627 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -21,7 +21,7 @@
 #include "qemu/error-report.h"
 #include "trace.h"

-static MultiFDSendData *multifd_ram_send;
+MultiFDSendData *multifd_ram_send;

 size_t multifd_ram_payload_size(void)
 {
diff --git a/migration/ram.c b/migration/ram.c
index 7284c34bd8..edeb9e28ff 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3228,6 +3228,8 @@ out:
     return done;
 }

+extern MultiFDSendData *multifd_ram_send;
+
 /**
  * ram_save_complete: function called to send the remaining amount of ram
  *
@@ -3283,6 +3285,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         }
     }

+    if (migrate_multifd()) {
+        assert(multifd_payload_empty(multifd_ram_send));
+    }
+
     if (migrate_multifd() &&
         migrate_multifd_flush_after_each_section()) {
         /*
===8<===

> 
> This also doesn't address what the commit message says about the recv
> side never getting the RAM_SAVE_FLAG_MULTIFD_FLUSH message.

The new binaries now always not send RAM_SAVE_FLAG_MULTIFD_FLUSH when
complete(), however it always sends the multifd SYNC messages on the wire.
It means those SYNC messages will always arrive on dest multifd channels,
and then all these channels will wait for the main thread to collect that.
However since RAM_SAVE_FLAG_MULTIFD_FLUSH is not there, they'll hang until
multifd recv cleanups.

-- 
Peter Xu