Series comparison

-[PATCH v2 0/7] migration/multifd: Some VFIO / postcopy preparations on flush
+[PATCH v3 0/7] migration/multifd: Some VFIO / postcopy preparations on flush
-CI: https://gitlab.com/peterx/qemu/-/pipelines/1575970314
+CI: https://gitlab.com/peterx/qemu/-/pipelines/1577280033
  (note: it's a pipeline of two patchsets, to save CI credits and time)
-Comparing to v1, this series v2 now contains some patches that may be
+v1: https://lore.kernel.org/r/20241205185303.897010-1-peterx@redhat.com
-helpful for either VFIO or postcopy integration on top of multifd.
+v2: https://lore.kernel.org/r/20241206005834.1050905-1-peterx@redhat.com
-For VFIO, only patch 1 & 2 are relevant.  This is the only part that can be
+v3 changelog:
-compared to v1, where I renamed the sync flag to LOCAL and ALL according to
+- R-bs collected
-Fabiano's comment.
+- Update commit message of patch 1 [Fabiano]
 - English updates [Fabiano]
 - Update comment for MULTIFD_SYNC_ALL [Fabiano]
 - In multifd_send_sync_main(), assert on req type [Fabiano]
 - Some more comments and cleanup for RAM_SAVE_FLAG_* movement [Fabiano]
 - Update the last document patch [Fabiano]
-For postcopy, it's about patches 3-7, but it needs to be based on 1+2.
+This series provides some changes that may be helpful for either VFIO or
 postcopy integration on top of multifd.
-I put them together because the two changes are relevant on the multifd
+For VFIO, only patches 1 & 2 are relevant.
 sync operation, one must depend on another in some form. So I decided to
 send them together here.  All these patches can be seen as cleanups /
 slight optimizations on top of master branch.
-This time I did more test.  Besides CI, qtests, and some real-world multifd
+For postcopy, it's about patches 3-7, but it needs to be based on 1+2
-tests just to monitor the sync events happen all correct, I made sure to
+because of a context dependency.
 cover 7.2 machine type (which uses the legacy sync) so it still works as
 before - basically sync will be more frequent, but all thing keeps working
 smoothly so far.
-Fabiano, let me know what do you think comparing to the other patch [1] on
+All these patches can be seen as cleanups / slight optimizations on top of
-simplifying the flush checks.  I'm open to any comments.
+master branch with/without the VFIO/postcopy work.
-Thanks,
+Besides CI, qtests, and some real-world multifd tests just to monitor the
+sync events happen all correct, I made sure to cover 7.2 machine type
-[1] https://lore.kernel.org/r/875xo8n4ue.fsf@suse.de
+(which uses the legacy sync) so it still works as before - basically sync
 will be more frequent, but all thing keeps working smoothly so far.
 Thanks,
 Peter Xu (7):
   migration/multifd: Further remove the SYNC on complete
 ...
   migration/multifd: Unify RAM_SAVE_FLAG_MULTIFD_FLUSH messages
   migration/multifd: Remove sync processing on postcopy
   migration/multifd: Cleanup src flushes on condition check
   migration/multifd: Document the reason to sync for save_setup()
- migration/multifd.h        | 23 ++++++++--
+ migration/multifd.h        | 27 ++++++++++--
- migration/ram.h            | 25 +++++++++++
+ migration/ram.h            | 28 ++++++++++++
  migration/rdma.h           |  7 ---
  migration/multifd-nocomp.c | 74 ++++++++++++++++++++++++++++++-
- migration/multifd.c        | 15 ++++---
+ migration/multifd.c        | 17 +++++---
- migration/ram.c            | 91 +++++++++++++++++---------------------
+ migration/ram.c            | 89 +++++++++++++++++---------------------
-files changed, 166 insertions(+), 69 deletions(-)
+files changed, 173 insertions(+), 69 deletions(-)
 --
 .47.0

-[PATCH v2 1/7] migration/multifd: Further remove the SYNC on complete
+[PATCH v3 1/7] migration/multifd: Further remove the SYNC on complete
 Commit 637280aeb2 ("migration/multifd: Avoid the final FLUSH in
-complete()") removed the FLUSH operation on complete() which should avoid
+complete()") stopped sending the RAM_SAVE_FLAG_MULTIFD_FLUSH flag at
-one global sync on destination side, because it's not needed.
+ram_save_complete(), because the sync on the destination side is not
 needed due to the last iteration of find_dirty_block() having already
 done it.
-However that commit overlooked multifd_ram_flush_and_sync() part of things,
+However, that commit overlooked that multifd_ram_flush_and_sync() on the
-as that's always used together with the FLUSH message on the main channel.
+source side is also not needed at ram_save_complete(), for the same
 reason.
-For very old binaries (multifd_flush_after_each_section==true), that's
+Moreover, removing the RAM_SAVE_FLAG_MULTIFD_FLUSH but keeping the
-still needed because each EOS received on destination will enforce
+multifd_ram_flush_and_sync() means that currently the recv threads will
-all-channel sync once.
+hang when receiving the MULTIFD_FLAG_SYNC message, waiting for the
 destination sync which only happens when RAM_SAVE_FLAG_MULTIFD_FLUSH is
 received.
-For new binaries (multifd_flush_after_each_section==false), the flush and
+Luckily, multifd is still all working fine because recv side cleanup
-sync shouldn't be needed just like the FLUSH, with the same reasoning.
+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.
-Currently, on new binaries we'll still send SYNC messages on multifd
+This needs to be fixed because in the future VFIO will have data to push
-channels, even if FLUSH is omitted at the end.  It'll make all recv threads
+after ram_save_complete() and we don't want the recv thread to be stuck
-hang at SYNC message.
+in the MULTIFD_FLAG_SYNC message.
-Multifd is still all working fine because luckily recv side cleanup
+Remove the unnecessary (and buggy) invocation of
-code (mostly multifd_recv_sync_main()) is smart enough to make sure even if
+multifd_ram_flush_and_sync().
 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.
-This may be needed for VFIO in the future because VFIO can have data to
+For very old binaries (multifd_flush_after_each_section==true), the
-push after ram_save_complete(), hence we don't want the recv thread to be
+flush_and_sync is still needed because each EOS received on destination
-stuck in SYNC message. Remove this limitation will make src even slightly
+will enforce all-channel sync once.
 faster too to avoid some more code.
-Stable branches do not need this patch, as no real bug I can think of that
+Stable branches do not need this patch, as no real bug I can think of
-will go wrong there.. so not attaching Fixes to be clear on the backport
+that will go wrong there.. so not attaching Fixes to be clear on the
-not needed.
+backport not needed.
+Reviewed-by: Fabiano Rosas <farosas@suse.de>
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
  migration/ram.c | 13 ++++++++++---
 file changed, 10 insertions(+), 3 deletions(-)
 ...

-[PATCH v2 2/7] migration/multifd: Allow to sync with sender threads only
+[PATCH v3 2/7] migration/multifd: Allow to sync with sender threads only
 Teach multifd_send_sync_main() to sync with threads only.
 We already have such requests, which is when mapped-ram is enabled with
 multifd.  In that case, no SYNC messages will be pushed to the stream when
 multifd syncs the sender threads because there's no destination threads
 waiting for that.  The whole point of the sync is to make sure all threads
-flushed their jobs.
+finished their jobs.
 So fundamentally we have a request to do the sync in different ways:
   - Either to sync the threads only,
   - Or to sync the threads but also with the destination side.
 ...
 No functional change intended.
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
- migration/multifd.h        | 19 ++++++++++++++++---
+ migration/multifd.h        | 23 ++++++++++++++++++++---
  migration/multifd-nocomp.c |  7 ++++++-
- migration/multifd.c        | 15 +++++++++------
+ migration/multifd.c        | 17 +++++++++++------
-files changed, 31 insertions(+), 10 deletions(-)
+files changed, 37 insertions(+), 10 deletions(-)
 diff --git a/migration/multifd.h b/migration/multifd.h
 index XXXXXXX..XXXXXXX 100644
 --- a/migration/multifd.h
 +++ b/migration/multifd.h
 ...
 +    /* No sync request */
 +    MULTIFD_SYNC_NONE = 0,
 +    /* Sync locally on the sender threads without pushing messages */
 +    MULTIFD_SYNC_LOCAL,
 +    /*
-+     * Sync not only on the sender threads, but also push "SYNC" message to
++     * Sync not only on the sender threads, but also push MULTIFD_FLAG_SYNC
-+     * the wire (which is for a remote sync).
++     * message to the wire for each iochannel (which is for a remote sync).
 +     *
 +     * When remote sync is used, need to be paired with a follow up
 +     * RAM_SAVE_FLAG_EOS / RAM_SAVE_FLAG_MULTIFD_FLUSH message on the main
 +     * channel.
 +     */
 +    MULTIFD_SYNC_ALL,
 +} MultiFDSyncReq;
 +
  bool multifd_send_setup(void);
 ...
 -int multifd_send_sync_main(void)
 +int multifd_send_sync_main(MultiFDSyncReq req)
  {
      int i;
      bool flush_zero_copy;
++    assert(req != MULTIFD_SYNC_NONE);
++
+     flush_zero_copy = migrate_zero_copy_send();
+     for (i = 0; i < migrate_multifd_channels(); i++) {
 @@ -XXX,XX +XXX,XX @@ int multifd_send_sync_main(void)
           * We should be the only user so far, so not possible to be set by
           * others concurrently.
           */
 -        assert(qatomic_read(&p->pending_sync) == false);
 ...

-[PATCH v2 3/7] migration/ram: Move RAM_SAVE_FLAG* into ram.h
+[PATCH v3 3/7] migration/ram: Move RAM_SAVE_FLAG* into ram.h
 Firstly, we're going to use the multifd flag soon in multifd code, so ram.c
 isn't gonna work.
 Secondly, we have a separate RDMA flag dangling around, which is definitely
 not obvious.  There's one comment that helps, but not too much.
-We should just put it altogether, so nothing will get overlooked.
+Put all RAM save flags altogether, so nothing will get overlooked.
+Add a section explain why we can't use bits over 0x200.
+Remove RAM_SAVE_FLAG_FULL as it's already not used in QEMU, as the comment
+explained.
+Reviewed-by: Fabiano Rosas <farosas@suse.de>
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
- migration/ram.h  | 25 +++++++++++++++++++++++++
+ migration/ram.h  | 28 ++++++++++++++++++++++++++++
  migration/rdma.h |  7 -------
  migration/ram.c  | 21 ---------------------
-files changed, 25 insertions(+), 28 deletions(-)
+files changed, 28 insertions(+), 28 deletions(-)
 diff --git a/migration/ram.h b/migration/ram.h
 index XXXXXXX..XXXXXXX 100644
 --- a/migration/ram.h
 +++ b/migration/ram.h
 ...
 + * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
 + * worked for pages that were filled with the same char.  We switched
 + * it to only search for the zero value.  And to avoid confusion with
 + * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
 + *
-+ * RAM_SAVE_FLAG_FULL was obsoleted in 2009.
++ * RAM_SAVE_FLAG_FULL (0x01) was obsoleted in 2009.
 + *
 + * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
++ *
++ * RAM_SAVE_FLAG_HOOK is only used in RDMA. Whenever this is found in the
++ * data stream, the flags will be passed to rdma functions in the
++ * incoming-migration side.
++ *
++ * We can't use any flag that is bigger than 0x200, because the flags are
++ * always assumed to be encoded in a ramblock address offset, which is
++ * multiple of PAGE_SIZE.  Here it means QEMU supports migration with any
++ * architecture that has PAGE_SIZE>=1K (0x400).
 + */
-+#define RAM_SAVE_FLAG_FULL     0x01
++#define RAM_SAVE_FLAG_ZERO                    0x002
-+#define RAM_SAVE_FLAG_ZERO     0x02
++#define RAM_SAVE_FLAG_MEM_SIZE                0x004
-+#define RAM_SAVE_FLAG_MEM_SIZE 0x04
++#define RAM_SAVE_FLAG_PAGE                    0x008
-+#define RAM_SAVE_FLAG_PAGE     0x08
++#define RAM_SAVE_FLAG_EOS                     0x010
-+#define RAM_SAVE_FLAG_EOS      0x10
++#define RAM_SAVE_FLAG_CONTINUE                0x020
-+#define RAM_SAVE_FLAG_CONTINUE 0x20
++#define RAM_SAVE_FLAG_XBZRLE                  0x040
-+#define RAM_SAVE_FLAG_XBZRLE   0x40
++#define RAM_SAVE_FLAG_HOOK                    0x080
-+/*
++#define RAM_SAVE_FLAG_MULTIFD_FLUSH           0x200
 + * ONLY USED IN RDMA: Whenever this is found in the data stream, the flags
 + * will be passed to rdma functions in the incoming-migration side.
 + */
 +#define RAM_SAVE_FLAG_HOOK     0x80
 +#define RAM_SAVE_FLAG_MULTIFD_FLUSH    0x200
 +/* We can't use any flag that is bigger than 0x200 */
 +
  extern XBZRLECacheStats xbzrle_counters;
  /* Should be holding either ram_list.mutex, or the RCU lock. */
 diff --git a/migration/rdma.h b/migration/rdma.h
 ...

-[PATCH v2 4/7] migration/multifd: Unify RAM_SAVE_FLAG_MULTIFD_FLUSH messages
+[PATCH v3 4/7] migration/multifd: Unify RAM_SAVE_FLAG_MULTIFD_FLUSH messages
 RAM_SAVE_FLAG_MULTIFD_FLUSH message should always be correlated to a sync
 request on src.  Unify such message into one place, and conditionally send
 the message only if necessary.
+Reviewed-by: Fabiano Rosas <farosas@suse.de>
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
  migration/multifd.h        |  2 +-
  migration/multifd-nocomp.c | 27 +++++++++++++++++++++++++--
  migration/ram.c            | 18 ++++--------------
 ...

-[PATCH v2 5/7] migration/multifd: Remove sync processing on postcopy
+[PATCH v3 5/7] migration/multifd: Remove sync processing on postcopy
 Multifd never worked with postcopy, at least yet so far.
 Remove the sync processing there, because it's confusing, and they should
 never appear.  Now if RAM_SAVE_FLAG_MULTIFD_FLUSH is observed, we fail hard
 instead of trying to invoke multifd code.
+Reviewed-by: Fabiano Rosas <farosas@suse.de>
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
  migration/ram.c | 8 --------
 file changed, 8 deletions(-)
 ...

-[PATCH v2 6/7] migration/multifd: Cleanup src flushes on condition check
+[PATCH v3 6/7] migration/multifd: Cleanup src flushes on condition check
 The src flush condition check is over complicated, and it's getting more
 out of control if postcopy will be involved.
 In general, we have two modes to do the sync: legacy or modern ways.
 Legacy uses per-section flush, modern uses per-round flush.
 Mapped-ram always uses the modern, which is per-round.
 Introduce two helpers, which can greatly simplify the code, and hopefully
 make it readable again.
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
  migration/multifd.h        |  2 ++
  migration/multifd-nocomp.c | 42 ++++++++++++++++++++++++++++++++++++++
  migration/ram.c            | 10 +++------
 files changed, 47 insertions(+), 7 deletions(-)
 diff --git a/migration/multifd.h b/migration/multifd.h
 index XXXXXXX..XXXXXXX 100644
 --- a/migration/multifd.h
 +++ b/migration/multifd.h
 @@ -XXX,XX +XXX,XX @@ static inline uint32_t multifd_ram_page_count(void)
  void multifd_ram_save_setup(void);
  void multifd_ram_save_cleanup(void);
  int multifd_ram_flush_and_sync(QEMUFile *f);
 +bool multifd_ram_sync_per_round(void);
 +bool multifd_ram_sync_per_section(void);
  size_t multifd_ram_payload_size(void);
  void multifd_ram_fill_packet(MultiFDSendParams *p);
  int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp);
 diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
 index XXXXXXX..XXXXXXX 100644
 --- a/migration/multifd-nocomp.c
 +++ b/migration/multifd-nocomp.c
 @@ -XXX,XX +XXX,XX @@ retry:
      return true;
  }
 +/*
 + * We have two modes for multifd flushes:
 + *
 + * - Per-section mode: this is the legacy way to flush, it requires one
 + *   MULTIFD_FLAG_SYNC message for each RAM_SAVE_FLAG_EOS.
 + *
 + * - Per-round mode: this is the modern way to flush, it requires one
 + *   MULTIFD_FLAG_SYNC message only for each round of RAM scan.  Normally
 + *   it's paired with a new RAM_SAVE_FLAG_MULTIFD_FLUSH message in network
 + *   based migrations.
 + *
 + * One thing to mention is mapped-ram always use the modern way to sync.
 + */
 +
 +/* Do we need a per-section multifd flush (legacy way)? */
 +bool multifd_ram_sync_per_section(void)
 +{
 +    if (!migrate_multifd()) {
 +        return false;
 +    }
 +
 +    if (migrate_mapped_ram()) {
 +        return false;
 +    }
 +
 +    return migrate_multifd_flush_after_each_section();
 +}
 +
 +/* Do we need a per-round multifd flush (modern way)? */
 +bool multifd_ram_sync_per_round(void)
 +{
 +    if (!migrate_multifd()) {
 +        return false;
 +    }
 +
 +    if (migrate_mapped_ram()) {
 +        return true;
 +    }
 +
 +    return !migrate_multifd_flush_after_each_section();
 +}
 +
  int multifd_ram_flush_and_sync(QEMUFile *f)
  {
      MultiFDSyncReq req;
 diff --git a/migration/ram.c b/migration/ram.c
 index XXXXXXX..XXXXXXX 100644
 --- a/migration/ram.c
 +++ b/migration/ram.c
 @@ -XXX,XX +XXX,XX @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
          pss->page = 0;
          pss->block = QLIST_NEXT_RCU(pss->block, next);
          if (!pss->block) {
 -            if (migrate_multifd() &&
 -                (!migrate_multifd_flush_after_each_section() ||
 -                 migrate_mapped_ram())) {
 +            if (multifd_ram_sync_per_round()) {
                  QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
                  int ret = multifd_ram_flush_and_sync(f);
                  if (ret < 0) {
 @@ -XXX,XX +XXX,XX @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
  out:
      if (ret >= 0 && migration_is_running()) {
 -        if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
 -            !migrate_mapped_ram()) {
 +        if (multifd_ram_sync_per_section()) {
              ret = multifd_ram_flush_and_sync(f);
              if (ret < 0) {
                  return ret;
 @@ -XXX,XX +XXX,XX @@ static int ram_save_complete(QEMUFile *f, void *opaque)
          }
      }
 -    if (migrate_multifd() &&
 -        migrate_multifd_flush_after_each_section()) {
 +    if (multifd_ram_sync_per_section()) {
          /*
           * Only the old dest QEMU will need this sync, because each EOS
           * will require one SYNC message on each channel.
 --
 .47.0

-[PATCH v2 7/7] migration/multifd: Document the reason to sync for save_setup()
+[PATCH v3 7/7] migration/multifd: Document the reason to sync for save_setup()
 ...
 again and do the debug one more time, or anyone confused on why this ever
 existed.
 Signed-off-by: Peter Xu <peterx@redhat.com>
 ---
- migration/ram.c | 27 +++++++++++++++++++++++++++
+ migration/ram.c | 25 +++++++++++++++++++++++++
-file changed, 27 insertions(+)
+file changed, 25 insertions(+)
 diff --git a/migration/ram.c b/migration/ram.c
 index XXXXXXX..XXXXXXX 100644
 --- a/migration/ram.c
 +++ b/migration/ram.c
 ...
 +     * per-channel to work.
 +     *
 +     * For modern QEMUs using per-round sync
 +     * =====================================
 +     *
-+     * Logically this sync is not needed (because we know there's nothing
++     * Logically such sync is not needed, and recv threads should not run
-+     * in the multifd queue yet!).  However as a side effect, this makes
++     * until setup ready (using things like channels_ready on src).  Then
-+     * sure the dest side won't receive any data before it properly reaches
++     * we should be all fine.
 +     * ram_load_precopy().
 +     *
-+     * Without this sync, src QEMU can send data too soon so that dest may
++     * However even if we add channels_ready to recv side in new QEMUs, old
-+     * not have been ready to receive it (e.g., rb->receivedmap may be
++     * QEMU won't have them so this sync will still be needed to make sure
-+     * uninitialized, for example).
++     * multifd recv threads won't start processing guest pages early before
 +     * ram_load_setup() is properly done.
 +     *
-+     * Logically "wait for recv setup ready" shouldn't need to involve src
++     * Let's stick with this.  Fortunately the overhead is low to sync
-+     * QEMU at all, however to be compatible with old QEMUs, let's stick
++     * during setup because the VM is running, so at least it's not
-+     * with this.  Fortunately the overhead is low to sync during setup
++     * accounted as part of downtime.
 +     * because the VM is running, so at least it's not accounted as part of
 +     * downtime.
 +     */
      bql_unlock();
      ret = multifd_ram_flush_and_sync(f);
      bql_lock();
 --
 .47.0

CI: https://gitlab.com/peterx/qemu/-/pipelines/1575970314

Comparing to v1, this series v2 now contains some patches that may be
helpful for either VFIO or postcopy integration on top of multifd.

For VFIO, only patch 1 & 2 are relevant.  This is the only part that can be
compared to v1, where I renamed the sync flag to LOCAL and ALL according to
Fabiano's comment.

For postcopy, it's about patches 3-7, but it needs to be based on 1+2.

I put them together because the two changes are relevant on the multifd
sync operation, one must depend on another in some form. So I decided to
send them together here.  All these patches can be seen as cleanups /
slight optimizations on top of master branch.

This time I did more test.  Besides CI, qtests, and some real-world multifd
tests just to monitor the sync events happen all correct, I made sure to
cover 7.2 machine type (which uses the legacy sync) so it still works as
before - basically sync will be more frequent, but all thing keeps working
smoothly so far.

Fabiano, let me know what do you think comparing to the other patch [1] on
simplifying the flush checks.  I'm open to any comments.

Thanks,

[1] https://lore.kernel.org/r/875xo8n4ue.fsf@suse.de

Thanks,

Peter Xu (7):
  migration/multifd: Further remove the SYNC on complete
  migration/multifd: Allow to sync with sender threads only
  migration/ram: Move RAM_SAVE_FLAG* into ram.h
  migration/multifd: Unify RAM_SAVE_FLAG_MULTIFD_FLUSH messages
  migration/multifd: Remove sync processing on postcopy
  migration/multifd: Cleanup src flushes on condition check
  migration/multifd: Document the reason to sync for save_setup()

migration/multifd.h        | 23 ++++++++--
 migration/ram.h            | 25 +++++++++++
 migration/rdma.h           |  7 ---
 migration/multifd-nocomp.c | 74 ++++++++++++++++++++++++++++++-
 migration/multifd.c        | 15 ++++---
 migration/ram.c            | 91 +++++++++++++++++---------------------
 6 files changed, 166 insertions(+), 69 deletions(-)

-- 
2.47.0

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.

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.

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.

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.

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 XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ 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;
+        }
     }
 
     if (migrate_mapped_ram()) {
-- 
2.47.0

Teach multifd_send_sync_main() to sync with threads only.

We already have such requests, which is when mapped-ram is enabled with
multifd.  In that case, no SYNC messages will be pushed to the stream when
multifd syncs the sender threads because there's no destination threads
waiting for that.  The whole point of the sync is to make sure all threads
flushed their jobs.

So fundamentally we have a request to do the sync in different ways:

- Either to sync the threads only,
  - Or to sync the threads but also with the destination side.

Mapped-ram did it already because of the use_packet check in the sync
handler of the sender thread.  It works.

However it may stop working when e.g. VFIO may start to reuse multifd
channels to push device states.  In that case VFIO has similar request on
"thread-only sync" however we can't check a flag because such sync request
can still come from RAM which needs the on-wire notifications.

Paving way for that by allowing the multifd_send_sync_main() to specify
what kind of sync the caller needs.  We can use it for mapped-ram already.

No functional change intended.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h        | 19 ++++++++++++++++---
 migration/multifd-nocomp.c |  7 ++++++-
 migration/multifd.c        | 15 +++++++++------
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -XXX,XX +XXX,XX @@
 typedef struct MultiFDRecvData MultiFDRecvData;
 typedef struct MultiFDSendData MultiFDSendData;
 
+typedef enum {
+    /* No sync request */
+    MULTIFD_SYNC_NONE = 0,
+    /* Sync locally on the sender threads without pushing messages */
+    MULTIFD_SYNC_LOCAL,
+    /*
+     * Sync not only on the sender threads, but also push "SYNC" message to
+     * the wire (which is for a remote sync).
+     */
+    MULTIFD_SYNC_ALL,
+} MultiFDSyncReq;
+
 bool multifd_send_setup(void);
 void multifd_send_shutdown(void);
 void multifd_send_channel_created(void);
@@ -XXX,XX +XXX,XX @@ void multifd_recv_shutdown(void);
 bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
-int multifd_send_sync_main(void);
+int multifd_send_sync_main(MultiFDSyncReq req);
 bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
 bool multifd_recv(void);
 MultiFDRecvData *multifd_get_recv_data(void);
@@ -XXX,XX +XXX,XX @@ typedef struct {
     /* multifd flags for each packet */
     uint32_t flags;
     /*
-     * The sender thread has work to do if either of below boolean is set.
+     * The sender thread has work to do if either of below field is set.
      *
      * @pending_job:  a job is pending
      * @pending_sync: a sync request is pending
@@ -XXX,XX +XXX,XX @@ typedef struct {
      * cleared by the multifd sender threads.
      */
     bool pending_job;
-    bool pending_sync;
+    MultiFDSyncReq pending_sync;
+
     MultiFDSendData *data;
 
     /* thread local variables. No locking required */
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -XXX,XX +XXX,XX @@ retry:
 
 int multifd_ram_flush_and_sync(void)
 {
+    MultiFDSyncReq req;
+
     if (!migrate_multifd()) {
         return 0;
     }
@@ -XXX,XX +XXX,XX @@ int multifd_ram_flush_and_sync(void)
         }
     }
 
-    return multifd_send_sync_main();
+    /* File migrations only need to sync with threads */
+    req = migrate_mapped_ram() ? MULTIFD_SYNC_LOCAL : MULTIFD_SYNC_ALL;
+
+    return multifd_send_sync_main(req);
 }
 
 bool multifd_send_prepare_common(MultiFDSendParams *p)
diff --git a/migration/multifd.c b/migration/multifd.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -XXX,XX +XXX,XX @@ static int multifd_zero_copy_flush(QIOChannel *c)
     return ret;
 }
 
-int multifd_send_sync_main(void)
+int multifd_send_sync_main(MultiFDSyncReq req)
 {
     int i;
     bool flush_zero_copy;
@@ -XXX,XX +XXX,XX @@ int multifd_send_sync_main(void)
          * We should be the only user so far, so not possible to be set by
          * others concurrently.
          */
-        assert(qatomic_read(&p->pending_sync) == false);
-        qatomic_set(&p->pending_sync, true);
+        assert(qatomic_read(&p->pending_sync) == MULTIFD_SYNC_NONE);
+        qatomic_set(&p->pending_sync, req);
         qemu_sem_post(&p->sem);
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -XXX,XX +XXX,XX @@ static void *multifd_send_thread(void *opaque)
              */
             qatomic_store_release(&p->pending_job, false);
         } else {
+            MultiFDSyncReq req = qatomic_read(&p->pending_sync);
+
             /*
              * If not a normal job, must be a sync request.  Note that
              * pending_sync is a standalone flag (unlike pending_job), so
              * it doesn't require explicit memory barriers.
              */
-            assert(qatomic_read(&p->pending_sync));
+            assert(req != MULTIFD_SYNC_NONE);
 
-            if (use_packets) {
+            /* Only push the SYNC message if it involves a remote sync */
+            if (req == MULTIFD_SYNC_ALL) {
                 p->flags = MULTIFD_FLAG_SYNC;
                 multifd_send_fill_packet(p);
                 ret = qio_channel_write_all(p->c, (void *)p->packet,
@@ -XXX,XX +XXX,XX @@ static void *multifd_send_thread(void *opaque)
                 stat64_add(&mig_stats.multifd_bytes, p->packet_len);
             }
 
-            qatomic_set(&p->pending_sync, false);
+            qatomic_set(&p->pending_sync, MULTIFD_SYNC_NONE);
             qemu_sem_post(&p->sem_sync);
         }
     }
-- 
2.47.0

Firstly, we're going to use the multifd flag soon in multifd code, so ram.c
isn't gonna work.

Secondly, we have a separate RDMA flag dangling around, which is definitely
not obvious.  There's one comment that helps, but not too much.

We should just put it altogether, so nothing will get overlooked.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.h  | 25 +++++++++++++++++++++++++
 migration/rdma.h |  7 -------
 migration/ram.c  | 21 ---------------------
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/migration/ram.h b/migration/ram.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -XXX,XX +XXX,XX @@
 #include "exec/cpu-common.h"
 #include "io/channel.h"
 
+/*
+ * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
+ * worked for pages that were filled with the same char.  We switched
+ * it to only search for the zero value.  And to avoid confusion with
+ * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
+ *
+ * RAM_SAVE_FLAG_FULL was obsoleted in 2009.
+ *
+ * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
+ */
+#define RAM_SAVE_FLAG_FULL     0x01
+#define RAM_SAVE_FLAG_ZERO     0x02
+#define RAM_SAVE_FLAG_MEM_SIZE 0x04
+#define RAM_SAVE_FLAG_PAGE     0x08
+#define RAM_SAVE_FLAG_EOS      0x10
+#define RAM_SAVE_FLAG_CONTINUE 0x20
+#define RAM_SAVE_FLAG_XBZRLE   0x40
+/*
+ * ONLY USED IN RDMA: Whenever this is found in the data stream, the flags
+ * will be passed to rdma functions in the incoming-migration side.
+ */
+#define RAM_SAVE_FLAG_HOOK     0x80
+#define RAM_SAVE_FLAG_MULTIFD_FLUSH    0x200
+/* We can't use any flag that is bigger than 0x200 */
+
 extern XBZRLECacheStats xbzrle_counters;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
diff --git a/migration/rdma.h b/migration/rdma.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -XXX,XX +XXX,XX @@ void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
 #define RAM_CONTROL_ROUND     1
 #define RAM_CONTROL_FINISH    3
 
-/*
- * Whenever this is found in the data stream, the flags
- * will be passed to rdma functions in the incoming-migration
- * side.
- */
-#define RAM_SAVE_FLAG_HOOK     0x80
-
 #define RAM_SAVE_CONTROL_NOT_SUPP -1000
 #define RAM_SAVE_CONTROL_DELAYED  -2000
 
diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@
 /***********************************************************/
 /* ram save/restore */
 
-/*
- * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
- * worked for pages that were filled with the same char.  We switched
- * it to only search for the zero value.  And to avoid confusion with
- * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
- *
- * RAM_SAVE_FLAG_FULL was obsoleted in 2009.
- *
- * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
- */
-#define RAM_SAVE_FLAG_FULL     0x01
-#define RAM_SAVE_FLAG_ZERO     0x02
-#define RAM_SAVE_FLAG_MEM_SIZE 0x04
-#define RAM_SAVE_FLAG_PAGE     0x08
-#define RAM_SAVE_FLAG_EOS      0x10
-#define RAM_SAVE_FLAG_CONTINUE 0x20
-#define RAM_SAVE_FLAG_XBZRLE   0x40
-/* 0x80 is reserved in rdma.h for RAM_SAVE_FLAG_HOOK */
-#define RAM_SAVE_FLAG_MULTIFD_FLUSH    0x200
-/* We can't use any flag that is bigger than 0x200 */
-
 /*
  * mapped-ram migration supports O_DIRECT, so we need to make sure the
  * userspace buffer, the IO operation size and the file offset are
-- 
2.47.0

RAM_SAVE_FLAG_MULTIFD_FLUSH message should always be correlated to a sync
request on src.  Unify such message into one place, and conditionally send
the message only if necessary.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h        |  2 +-
 migration/multifd-nocomp.c | 27 +++++++++++++++++++++++++--
 migration/ram.c            | 18 ++++--------------
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -XXX,XX +XXX,XX @@ static inline uint32_t multifd_ram_page_count(void)
 
 void multifd_ram_save_setup(void);
 void multifd_ram_save_cleanup(void);
-int multifd_ram_flush_and_sync(void);
+int multifd_ram_flush_and_sync(QEMUFile *f);
 size_t multifd_ram_payload_size(void);
 void multifd_ram_fill_packet(MultiFDSendParams *p);
 int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp);
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -XXX,XX +XXX,XX @@
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "trace.h"
+#include "qemu-file.h"
 
 static MultiFDSendData *multifd_ram_send;
 
@@ -XXX,XX +XXX,XX @@ retry:
     return true;
 }
 
-int multifd_ram_flush_and_sync(void)
+int multifd_ram_flush_and_sync(QEMUFile *f)
 {
     MultiFDSyncReq req;
+    int ret;
 
     if (!migrate_multifd()) {
         return 0;
@@ -XXX,XX +XXX,XX @@ int multifd_ram_flush_and_sync(void)
     /* File migrations only need to sync with threads */
     req = migrate_mapped_ram() ? MULTIFD_SYNC_LOCAL : MULTIFD_SYNC_ALL;
 
-    return multifd_send_sync_main(req);
+    ret = multifd_send_sync_main(req);
+    if (ret) {
+        return ret;
+    }
+
+    /* If we don't need to sync with remote at all, nothing else to do */
+    if (req == MULTIFD_SYNC_LOCAL) {
+        return 0;
+    }
+
+    /*
+     * Old QEMUs don't understand RAM_SAVE_FLAG_MULTIFD_FLUSH, it relies
+     * on RAM_SAVE_FLAG_EOS instead.
+     */
+    if (migrate_multifd_flush_after_each_section()) {
+        return 0;
+    }
+
+    qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+    qemu_fflush(f);
+
+    return 0;
 }
 
 bool multifd_send_prepare_common(MultiFDSendParams *p)
diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
                 (!migrate_multifd_flush_after_each_section() ||
                  migrate_mapped_ram())) {
                 QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
-                int ret = multifd_ram_flush_and_sync();
+                int ret = multifd_ram_flush_and_sync(f);
                 if (ret < 0) {
                     return ret;
                 }
-
-                if (!migrate_mapped_ram()) {
-                    qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
-                    qemu_fflush(f);
-                }
             }
 
             /* Hit the end of the list */
@@ -XXX,XX +XXX,XX @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
     }
 
     bql_unlock();
-    ret = multifd_ram_flush_and_sync();
+    ret = multifd_ram_flush_and_sync(f);
     bql_lock();
     if (ret < 0) {
         error_setg(errp, "%s: multifd synchronization failed", __func__);
         return ret;
     }
 
-    if (migrate_multifd() && !migrate_multifd_flush_after_each_section()
-        && !migrate_mapped_ram()) {
-        qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
-    }
-
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
     ret = qemu_fflush(f);
     if (ret < 0) {
@@ -XXX,XX +XXX,XX @@ out:
     if (ret >= 0 && migration_is_running()) {
         if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
             !migrate_mapped_ram()) {
-            ret = multifd_ram_flush_and_sync();
+            ret = multifd_ram_flush_and_sync(f);
             if (ret < 0) {
                 return ret;
             }
@@ -XXX,XX +XXX,XX @@ static int ram_save_complete(QEMUFile *f, void *opaque)
          * 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();
+        ret = multifd_ram_flush_and_sync(f);
         if (ret < 0) {
             return ret;
         }
-- 
2.47.0

Multifd never worked with postcopy, at least yet so far.

Remove the sync processing there, because it's confusing, and they should
never appear.  Now if RAM_SAVE_FLAG_MULTIFD_FLUSH is observed, we fail hard
instead of trying to invoke multifd code.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ int ram_load_postcopy(QEMUFile *f, int channel)
                                          TARGET_PAGE_SIZE);
             }
             break;
-        case RAM_SAVE_FLAG_MULTIFD_FLUSH:
-            multifd_recv_sync_main();
-            break;
         case RAM_SAVE_FLAG_EOS:
-            /* normal exit */
-            if (migrate_multifd() &&
-                migrate_multifd_flush_after_each_section()) {
-                multifd_recv_sync_main();
-            }
             break;
         default:
             error_report("Unknown combination of migration flags: 0x%x"
-- 
2.47.0

The src flush condition check is over complicated, and it's getting more
out of control if postcopy will be involved.

In general, we have two modes to do the sync: legacy or modern ways.
Legacy uses per-section flush, modern uses per-round flush.

Mapped-ram always uses the modern, which is per-round.

Introduce two helpers, which can greatly simplify the code, and hopefully
make it readable again.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h        |  2 ++
 migration/multifd-nocomp.c | 42 ++++++++++++++++++++++++++++++++++++++
 migration/ram.c            | 10 +++------
 3 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -XXX,XX +XXX,XX @@ static inline uint32_t multifd_ram_page_count(void)
 void multifd_ram_save_setup(void);
 void multifd_ram_save_cleanup(void);
 int multifd_ram_flush_and_sync(QEMUFile *f);
+bool multifd_ram_sync_per_round(void);
+bool multifd_ram_sync_per_section(void);
 size_t multifd_ram_payload_size(void);
 void multifd_ram_fill_packet(MultiFDSendParams *p);
 int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp);
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -XXX,XX +XXX,XX @@ retry:
     return true;
 }
 
+/*
+ * We have two modes for multifd flushes:
+ *
+ * - Per-section mode: this is the legacy way to flush, it requires one
+ *   MULTIFD_FLAG_SYNC message for each RAM_SAVE_FLAG_EOS.
+ *
+ * - Per-round mode: this is the modern way to flush, it requires one
+ *   MULTIFD_FLAG_SYNC message only for each round of RAM scan.  Normally
+ *   it's paired with a new RAM_SAVE_FLAG_MULTIFD_FLUSH message in network
+ *   based migrations.
+ *
+ * One thing to mention is mapped-ram always use the modern way to sync.
+ */
+
+/* Do we need a per-section multifd flush (legacy way)? */
+bool multifd_ram_sync_per_section(void)
+{
+    if (!migrate_multifd()) {
+        return false;
+    }
+
+    if (migrate_mapped_ram()) {
+        return false;
+    }
+
+    return migrate_multifd_flush_after_each_section();
+}
+
+/* Do we need a per-round multifd flush (modern way)? */
+bool multifd_ram_sync_per_round(void)
+{
+    if (!migrate_multifd()) {
+        return false;
+    }
+
+    if (migrate_mapped_ram()) {
+        return true;
+    }
+
+    return !migrate_multifd_flush_after_each_section();
+}
+
 int multifd_ram_flush_and_sync(QEMUFile *f)
 {
     MultiFDSyncReq req;
diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
         pss->page = 0;
         pss->block = QLIST_NEXT_RCU(pss->block, next);
         if (!pss->block) {
-            if (migrate_multifd() &&
-                (!migrate_multifd_flush_after_each_section() ||
-                 migrate_mapped_ram())) {
+            if (multifd_ram_sync_per_round()) {
                 QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
                 int ret = multifd_ram_flush_and_sync(f);
                 if (ret < 0) {
@@ -XXX,XX +XXX,XX @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
 out:
     if (ret >= 0 && migration_is_running()) {
-        if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
-            !migrate_mapped_ram()) {
+        if (multifd_ram_sync_per_section()) {
             ret = multifd_ram_flush_and_sync(f);
             if (ret < 0) {
                 return ret;
@@ -XXX,XX +XXX,XX @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         }
     }
 
-    if (migrate_multifd() &&
-        migrate_multifd_flush_after_each_section()) {
+    if (multifd_ram_sync_per_section()) {
         /*
          * Only the old dest QEMU will need this sync, because each EOS
          * will require one SYNC message on each channel.
-- 
2.47.0

It's not straightforward to see why src QEMU needs to sync multifd during
setup() phase.  After all, there's no page queued at that point.

For old QEMUs, there's a solid reason: EOS requires it to work.  While it's
clueless on the new QEMUs which do not take EOS message as sync requests.

One will figure that out only when this is conditionally removed.  In fact,
the author did try it out.  Logically we could still avoid doing this on
new machine types, however that needs a separate compat field and that can
be an overkill in some trivial overhead in setup() phase.

Let's instead document it completely, to avoid someone else tries this
again and do the debug one more time, or anyone confused on why this ever
existed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
         migration_ops->ram_save_target_page = ram_save_target_page_legacy;
     }
 
+    /*
+     * This operation is unfortunate..
+     *
+     * For legacy QEMUs using per-section sync
+     * =======================================
+     *
+     * This must exist because the EOS below requires the SYNC messages
+     * per-channel to work.
+     *
+     * For modern QEMUs using per-round sync
+     * =====================================
+     *
+     * Logically this sync is not needed (because we know there's nothing
+     * in the multifd queue yet!).  However as a side effect, this makes
+     * sure the dest side won't receive any data before it properly reaches
+     * ram_load_precopy().
+     *
+     * Without this sync, src QEMU can send data too soon so that dest may
+     * not have been ready to receive it (e.g., rb->receivedmap may be
+     * uninitialized, for example).
+     *
+     * Logically "wait for recv setup ready" shouldn't need to involve src
+     * QEMU at all, however to be compatible with old QEMUs, let's stick
+     * with this.  Fortunately the overhead is low to sync during setup
+     * because the VM is running, so at least it's not accounted as part of
+     * downtime.
+     */
     bql_unlock();
     ret = multifd_ram_flush_and_sync(f);
     bql_lock();
-- 
2.47.0

CI: https://gitlab.com/peterx/qemu/-/pipelines/1577280033
 (note: it's a pipeline of two patchsets, to save CI credits and time)

v1: https://lore.kernel.org/r/20241205185303.897010-1-peterx@redhat.com
v2: https://lore.kernel.org/r/20241206005834.1050905-1-peterx@redhat.com

v3 changelog:
- R-bs collected
- Update commit message of patch 1 [Fabiano]
- English updates [Fabiano]
- Update comment for MULTIFD_SYNC_ALL [Fabiano]
- In multifd_send_sync_main(), assert on req type [Fabiano]
- Some more comments and cleanup for RAM_SAVE_FLAG_* movement [Fabiano]
- Update the last document patch [Fabiano]

This series provides some changes that may be helpful for either VFIO or
postcopy integration on top of multifd.

For VFIO, only patches 1 & 2 are relevant.

For postcopy, it's about patches 3-7, but it needs to be based on 1+2
because of a context dependency.

All these patches can be seen as cleanups / slight optimizations on top of
master branch with/without the VFIO/postcopy work.

Besides CI, qtests, and some real-world multifd tests just to monitor the
sync events happen all correct, I made sure to cover 7.2 machine type
(which uses the legacy sync) so it still works as before - basically sync
will be more frequent, but all thing keeps working smoothly so far.

Thanks,

migration/multifd.h        | 27 ++++++++++--
 migration/ram.h            | 28 ++++++++++++
 migration/rdma.h           |  7 ---
 migration/multifd-nocomp.c | 74 ++++++++++++++++++++++++++++++-
 migration/multifd.c        | 17 +++++---
 migration/ram.c            | 89 +++++++++++++++++---------------------
 6 files changed, 173 insertions(+), 69 deletions(-)

-- 
2.47.0

Commit 637280aeb2 ("migration/multifd: Avoid the final FLUSH in
complete()") stopped sending the RAM_SAVE_FLAG_MULTIFD_FLUSH flag at
ram_save_complete(), because the sync on the destination side is not
needed due to the last iteration of find_dirty_block() having already
done it.

However, that commit overlooked that multifd_ram_flush_and_sync() on the
source side is also not needed at ram_save_complete(), for the same
reason.

Moreover, removing the RAM_SAVE_FLAG_MULTIFD_FLUSH but keeping the
multifd_ram_flush_and_sync() means that currently the recv threads will
hang when receiving the MULTIFD_FLAG_SYNC message, waiting for the
destination sync which only happens when RAM_SAVE_FLAG_MULTIFD_FLUSH is
received.

Luckily, multifd is still all working fine because 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.

This needs to be fixed because in the future VFIO will have data to push
after ram_save_complete() and we don't want the recv thread to be stuck
in the MULTIFD_FLAG_SYNC message.

Remove the unnecessary (and buggy) invocation of
multifd_ram_flush_and_sync().

For very old binaries (multifd_flush_after_each_section==true), the
flush_and_sync is still needed because each EOS received on destination
will enforce all-channel sync once.

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.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Teach multifd_send_sync_main() to sync with threads only.

So fundamentally we have a request to do the sync in different ways:

- Either to sync the threads only,
  - Or to sync the threads but also with the destination side.

Mapped-ram did it already because of the use_packet check in the sync
handler of the sender thread.  It works.

Paving way for that by allowing the multifd_send_sync_main() to specify
what kind of sync the caller needs.  We can use it for mapped-ram already.

No functional change intended.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h        | 23 ++++++++++++++++++++---
 migration/multifd-nocomp.c |  7 ++++++-
 migration/multifd.c        | 17 +++++++++++------
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -XXX,XX +XXX,XX @@
 typedef struct MultiFDRecvData MultiFDRecvData;
 typedef struct MultiFDSendData MultiFDSendData;
 
+typedef enum {
+    /* No sync request */
+    MULTIFD_SYNC_NONE = 0,
+    /* Sync locally on the sender threads without pushing messages */
+    MULTIFD_SYNC_LOCAL,
+    /*
+     * Sync not only on the sender threads, but also push MULTIFD_FLAG_SYNC
+     * message to the wire for each iochannel (which is for a remote sync).
+     *
+     * When remote sync is used, need to be paired with a follow up
+     * RAM_SAVE_FLAG_EOS / RAM_SAVE_FLAG_MULTIFD_FLUSH message on the main
+     * channel.
+     */
+    MULTIFD_SYNC_ALL,
+} MultiFDSyncReq;
+
 bool multifd_send_setup(void);
 void multifd_send_shutdown(void);
 void multifd_send_channel_created(void);
@@ -XXX,XX +XXX,XX @@ void multifd_recv_shutdown(void);
 bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
-int multifd_send_sync_main(void);
+int multifd_send_sync_main(MultiFDSyncReq req);
 bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
 bool multifd_recv(void);
 MultiFDRecvData *multifd_get_recv_data(void);
@@ -XXX,XX +XXX,XX @@ typedef struct {
     /* multifd flags for each packet */
     uint32_t flags;
     /*
-     * The sender thread has work to do if either of below boolean is set.
+     * The sender thread has work to do if either of below field is set.
      *
      * @pending_job:  a job is pending
      * @pending_sync: a sync request is pending
@@ -XXX,XX +XXX,XX @@ typedef struct {
      * cleared by the multifd sender threads.
      */
     bool pending_job;
-    bool pending_sync;
+    MultiFDSyncReq pending_sync;
+
     MultiFDSendData *data;
 
     /* thread local variables. No locking required */
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -XXX,XX +XXX,XX @@ retry:
 
 int multifd_ram_flush_and_sync(void)
 {
+    MultiFDSyncReq req;
+
     if (!migrate_multifd()) {
         return 0;
     }
@@ -XXX,XX +XXX,XX @@ int multifd_ram_flush_and_sync(void)
         }
     }
 
-    return multifd_send_sync_main();
+    /* File migrations only need to sync with threads */
+    req = migrate_mapped_ram() ? MULTIFD_SYNC_LOCAL : MULTIFD_SYNC_ALL;
+
+    return multifd_send_sync_main(req);
 }
 
 bool multifd_send_prepare_common(MultiFDSendParams *p)
diff --git a/migration/multifd.c b/migration/multifd.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -XXX,XX +XXX,XX @@ static int multifd_zero_copy_flush(QIOChannel *c)
     return ret;
 }
 
-int multifd_send_sync_main(void)
+int multifd_send_sync_main(MultiFDSyncReq req)
 {
     int i;
     bool flush_zero_copy;
 
+    assert(req != MULTIFD_SYNC_NONE);
+
     flush_zero_copy = migrate_zero_copy_send();
 
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -XXX,XX +XXX,XX @@ int multifd_send_sync_main(void)
          * We should be the only user so far, so not possible to be set by
          * others concurrently.
          */
-        assert(qatomic_read(&p->pending_sync) == false);
-        qatomic_set(&p->pending_sync, true);
+        assert(qatomic_read(&p->pending_sync) == MULTIFD_SYNC_NONE);
+        qatomic_set(&p->pending_sync, req);
         qemu_sem_post(&p->sem);
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -XXX,XX +XXX,XX @@ static void *multifd_send_thread(void *opaque)
              */
             qatomic_store_release(&p->pending_job, false);
         } else {
+            MultiFDSyncReq req = qatomic_read(&p->pending_sync);
+
             /*
              * If not a normal job, must be a sync request.  Note that
              * pending_sync is a standalone flag (unlike pending_job), so
              * it doesn't require explicit memory barriers.
              */
-            assert(qatomic_read(&p->pending_sync));
+            assert(req != MULTIFD_SYNC_NONE);
 
-            if (use_packets) {
+            /* Only push the SYNC message if it involves a remote sync */
+            if (req == MULTIFD_SYNC_ALL) {
                 p->flags = MULTIFD_FLAG_SYNC;
                 multifd_send_fill_packet(p);
                 ret = qio_channel_write_all(p->c, (void *)p->packet,
@@ -XXX,XX +XXX,XX @@ static void *multifd_send_thread(void *opaque)
                 stat64_add(&mig_stats.multifd_bytes, p->packet_len);
             }
 
-            qatomic_set(&p->pending_sync, false);
+            qatomic_set(&p->pending_sync, MULTIFD_SYNC_NONE);
             qemu_sem_post(&p->sem_sync);
         }
     }
-- 
2.47.0

Firstly, we're going to use the multifd flag soon in multifd code, so ram.c
isn't gonna work.

Secondly, we have a separate RDMA flag dangling around, which is definitely
not obvious.  There's one comment that helps, but not too much.

Put all RAM save flags altogether, so nothing will get overlooked.

Add a section explain why we can't use bits over 0x200.

Remove RAM_SAVE_FLAG_FULL as it's already not used in QEMU, as the comment
explained.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.h  | 28 ++++++++++++++++++++++++++++
 migration/rdma.h |  7 -------
 migration/ram.c  | 21 ---------------------
 3 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/migration/ram.h b/migration/ram.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -XXX,XX +XXX,XX @@
 #include "exec/cpu-common.h"
 #include "io/channel.h"
 
+/*
+ * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
+ * worked for pages that were filled with the same char.  We switched
+ * it to only search for the zero value.  And to avoid confusion with
+ * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
+ *
+ * RAM_SAVE_FLAG_FULL (0x01) was obsoleted in 2009.
+ *
+ * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
+ *
+ * RAM_SAVE_FLAG_HOOK is only used in RDMA. Whenever this is found in the
+ * data stream, the flags will be passed to rdma functions in the
+ * incoming-migration side.
+ *
+ * We can't use any flag that is bigger than 0x200, because the flags are
+ * always assumed to be encoded in a ramblock address offset, which is
+ * multiple of PAGE_SIZE.  Here it means QEMU supports migration with any
+ * architecture that has PAGE_SIZE>=1K (0x400).
+ */
+#define RAM_SAVE_FLAG_ZERO                    0x002
+#define RAM_SAVE_FLAG_MEM_SIZE                0x004
+#define RAM_SAVE_FLAG_PAGE                    0x008
+#define RAM_SAVE_FLAG_EOS                     0x010
+#define RAM_SAVE_FLAG_CONTINUE                0x020
+#define RAM_SAVE_FLAG_XBZRLE                  0x040
+#define RAM_SAVE_FLAG_HOOK                    0x080
+#define RAM_SAVE_FLAG_MULTIFD_FLUSH           0x200
+
 extern XBZRLECacheStats xbzrle_counters;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
diff --git a/migration/rdma.h b/migration/rdma.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -XXX,XX +XXX,XX @@ void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
 #define RAM_CONTROL_ROUND     1
 #define RAM_CONTROL_FINISH    3
 
-/*
- * Whenever this is found in the data stream, the flags
- * will be passed to rdma functions in the incoming-migration
- * side.
- */
-#define RAM_SAVE_FLAG_HOOK     0x80
-
 #define RAM_SAVE_CONTROL_NOT_SUPP -1000
 #define RAM_SAVE_CONTROL_DELAYED  -2000
 
diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@
 /***********************************************************/
 /* ram save/restore */
 
-/*
- * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
- * worked for pages that were filled with the same char.  We switched
- * it to only search for the zero value.  And to avoid confusion with
- * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
- *
- * RAM_SAVE_FLAG_FULL was obsoleted in 2009.
- *
- * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
- */
-#define RAM_SAVE_FLAG_FULL     0x01
-#define RAM_SAVE_FLAG_ZERO     0x02
-#define RAM_SAVE_FLAG_MEM_SIZE 0x04
-#define RAM_SAVE_FLAG_PAGE     0x08
-#define RAM_SAVE_FLAG_EOS      0x10
-#define RAM_SAVE_FLAG_CONTINUE 0x20
-#define RAM_SAVE_FLAG_XBZRLE   0x40
-/* 0x80 is reserved in rdma.h for RAM_SAVE_FLAG_HOOK */
-#define RAM_SAVE_FLAG_MULTIFD_FLUSH    0x200
-/* We can't use any flag that is bigger than 0x200 */
-
 /*
  * mapped-ram migration supports O_DIRECT, so we need to make sure the
  * userspace buffer, the IO operation size and the file offset are
-- 
2.47.0

RAM_SAVE_FLAG_MULTIFD_FLUSH message should always be correlated to a sync
request on src.  Unify such message into one place, and conditionally send
the message only if necessary.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h        |  2 +-
 migration/multifd-nocomp.c | 27 +++++++++++++++++++++++++--
 migration/ram.c            | 18 ++++--------------
 3 files changed, 30 insertions(+), 17 deletions(-)

Multifd never worked with postcopy, at least yet so far.

Remove the sync processing there, because it's confusing, and they should
never appear.  Now if RAM_SAVE_FLAG_MULTIFD_FLUSH is observed, we fail hard
instead of trying to invoke multifd code.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 8 --------
 1 file changed, 8 deletions(-)

The src flush condition check is over complicated, and it's getting more
out of control if postcopy will be involved.

In general, we have two modes to do the sync: legacy or modern ways.
Legacy uses per-section flush, modern uses per-round flush.

Mapped-ram always uses the modern, which is per-round.

Introduce two helpers, which can greatly simplify the code, and hopefully
make it readable again.

diff --git a/migration/multifd.h b/migration/multifd.h
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -XXX,XX +XXX,XX @@ static inline uint32_t multifd_ram_page_count(void)
 void multifd_ram_save_setup(void);
 void multifd_ram_save_cleanup(void);
 int multifd_ram_flush_and_sync(QEMUFile *f);
+bool multifd_ram_sync_per_round(void);
+bool multifd_ram_sync_per_section(void);
 size_t multifd_ram_payload_size(void);
 void multifd_ram_fill_packet(MultiFDSendParams *p);
 int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp);
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -XXX,XX +XXX,XX @@ retry:
     return true;
 }
 
+/*
+ * We have two modes for multifd flushes:
+ *
+ * - Per-section mode: this is the legacy way to flush, it requires one
+ *   MULTIFD_FLAG_SYNC message for each RAM_SAVE_FLAG_EOS.
+ *
+ * - Per-round mode: this is the modern way to flush, it requires one
+ *   MULTIFD_FLAG_SYNC message only for each round of RAM scan.  Normally
+ *   it's paired with a new RAM_SAVE_FLAG_MULTIFD_FLUSH message in network
+ *   based migrations.
+ *
+ * One thing to mention is mapped-ram always use the modern way to sync.
+ */
+
+/* Do we need a per-section multifd flush (legacy way)? */
+bool multifd_ram_sync_per_section(void)
+{
+    if (!migrate_multifd()) {
+        return false;
+    }
+
+    if (migrate_mapped_ram()) {
+        return false;
+    }
+
+    return migrate_multifd_flush_after_each_section();
+}
+
+/* Do we need a per-round multifd flush (modern way)? */
+bool multifd_ram_sync_per_round(void)
+{
+    if (!migrate_multifd()) {
+        return false;
+    }
+
+    if (migrate_mapped_ram()) {
+        return true;
+    }
+
+    return !migrate_multifd_flush_after_each_section();
+}
+
 int multifd_ram_flush_and_sync(QEMUFile *f)
 {
     MultiFDSyncReq req;
diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
         pss->page = 0;
         pss->block = QLIST_NEXT_RCU(pss->block, next);
         if (!pss->block) {
-            if (migrate_multifd() &&
-                (!migrate_multifd_flush_after_each_section() ||
-                 migrate_mapped_ram())) {
+            if (multifd_ram_sync_per_round()) {
                 QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
                 int ret = multifd_ram_flush_and_sync(f);
                 if (ret < 0) {
@@ -XXX,XX +XXX,XX @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
 out:
     if (ret >= 0 && migration_is_running()) {
-        if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
-            !migrate_mapped_ram()) {
+        if (multifd_ram_sync_per_section()) {
             ret = multifd_ram_flush_and_sync(f);
             if (ret < 0) {
                 return ret;
@@ -XXX,XX +XXX,XX @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         }
     }
 
-    if (migrate_multifd() &&
-        migrate_multifd_flush_after_each_section()) {
+    if (multifd_ram_sync_per_section()) {
         /*
          * Only the old dest QEMU will need this sync, because each EOS
          * will require one SYNC message on each channel.
-- 
2.47.0

It's not straightforward to see why src QEMU needs to sync multifd during
setup() phase.  After all, there's no page queued at that point.

For old QEMUs, there's a solid reason: EOS requires it to work.  While it's
clueless on the new QEMUs which do not take EOS message as sync requests.

Let's instead document it completely, to avoid someone else tries this
again and do the debug one more time, or anyone confused on why this ever
existed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index XXXXXXX..XXXXXXX 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -XXX,XX +XXX,XX @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
         migration_ops->ram_save_target_page = ram_save_target_page_legacy;
     }
 
+    /*
+     * This operation is unfortunate..
+     *
+     * For legacy QEMUs using per-section sync
+     * =======================================
+     *
+     * This must exist because the EOS below requires the SYNC messages
+     * per-channel to work.
+     *
+     * For modern QEMUs using per-round sync
+     * =====================================
+     *
+     * Logically such sync is not needed, and recv threads should not run
+     * until setup ready (using things like channels_ready on src).  Then
+     * we should be all fine.
+     *
+     * However even if we add channels_ready to recv side in new QEMUs, old
+     * QEMU won't have them so this sync will still be needed to make sure
+     * multifd recv threads won't start processing guest pages early before
+     * ram_load_setup() is properly done.
+     *
+     * Let's stick with this.  Fortunately the overhead is low to sync
+     * during setup because the VM is running, so at least it's not
+     * accounted as part of downtime.
+     */
     bql_unlock();
     ret = multifd_ram_flush_and_sync(f);
     bql_lock();
-- 
2.47.0