[PATCH v6 0/4] Allow to enable multifd and postcopy migration together

[PATCH v6 0/4] Allow to enable multifd and postcopy migration together

[Prasad Pandit]

From: Prasad Pandit <pjp@fedoraproject.org>

Hello,

* This series (v6) shuts down Multifd threads before starting Postcopy
  migration. It helps to avoid an issue of multifd pages arriving late
  at the destination during Postcopy phase and corrupting the vCPU
  state. It also reorders the qtest patches and does some refactoring
  changes as suggested in previous review.
===
67/67 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test                 OK             161.35s   73 subtests passed
===


v5: https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t
* This series (v5) consolidates migration capabilities setting in one
  'set_migration_capabilities()' function, thus simplifying test sources.
  It passes all migration tests.
===
66/66 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test                 OK             143.66s   71 subtests passed
===


v4: https://lore.kernel.org/qemu-devel/20250127120823.144949-1-ppandit@redhat.com/T/#t
* This series (v4) adds more 'multifd+postcopy' qtests which test
  Precopy migration with 'postcopy-ram' attribute set. And run
  Postcopy migrations with 'multifd' channels enabled.
===
$ ../qtest/migration-test --tap -k -r '/x86_64/migration/multifd+postcopy' | grep -i 'slow test'
# slow test /x86_64/migration/multifd+postcopy/plain executed in 1.29 secs
# slow test /x86_64/migration/multifd+postcopy/recovery/tls/psk executed in 2.48 secs
# slow test /x86_64/migration/multifd+postcopy/preempt/plain executed in 1.49 secs
# slow test /x86_64/migration/multifd+postcopy/preempt/recovery/tls/psk executed in 2.52 secs
# slow test /x86_64/migration/multifd+postcopy/tcp/tls/psk/match executed in 3.62 secs
# slow test /x86_64/migration/multifd+postcopy/tcp/plain/zstd executed in 1.34 secs
# slow test /x86_64/migration/multifd+postcopy/tcp/plain/cancel executed in 2.24 secs
...
66/66 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test                 OK             148.41s   71 subtests passed
===


v3: https://lore.kernel.org/qemu-devel/20250121131032.1611245-1-ppandit@redhat.com/T/#t
* This series (v3) passes all existing 'tests/qtest/migration/*' tests
  and adds a new one to enable multifd channels with postcopy migration.


v2: https://lore.kernel.org/qemu-devel/20241129122256.96778-1-ppandit@redhat.com/T/#u
* This series (v2) further refactors the 'ram_save_target_page'
  function to make it independent of the multifd & postcopy change.


v1: https://lore.kernel.org/qemu-devel/20241126115748.118683-1-ppandit@redhat.com/T/#u
* This series removes magic value (4-bytes) introduced in the
  previous series for the Postcopy channel.


v0: https://lore.kernel.org/qemu-devel/20241029150908.1136894-1-ppandit@redhat.com/T/#u
* Currently Multifd and Postcopy migration can not be used together.
  QEMU shows "Postcopy is not yet compatible with multifd" message.

  When migrating guests with large (100's GB) RAM, Multifd threads
  help to accelerate migration, but inability to use it with the
  Postcopy mode delays guest start up on the destination side.

* This patch series allows to enable both Multifd and Postcopy
  migration together. Precopy and Multifd threads work during
  the initial guest (RAM) transfer. When migration moves to the
  Postcopy phase, Multifd threads are restrained and the Postcopy
  threads start to request pages from the source side.

* This series introduces magic value (4-bytes) to be sent on the
  Postcopy channel. It helps to differentiate channels and properly
  setup incoming connections on the destination side.


Thank you.
---
Prasad Pandit (4):
  migration/multifd: move macros to multifd header
  migration: enable multifd and postcopy together
  tests/qtest/migration: consolidate set capabilities
  tests/qtest/migration: add postcopy tests with multifd

 migration/migration.c                     | 107 ++++++++++++----------
 migration/multifd-nocomp.c                |   3 +-
 migration/multifd.c                       |   9 +-
 migration/multifd.h                       |   5 +
 migration/options.c                       |   5 -
 migration/ram.c                           |   7 +-
 tests/qtest/migration/compression-tests.c |  23 ++++-
 tests/qtest/migration/cpr-tests.c         |   4 +-
 tests/qtest/migration/file-tests.c        |  44 +++------
 tests/qtest/migration/framework.c         |  58 +++++++++---
 tests/qtest/migration/framework.h         |   4 +-
 tests/qtest/migration/postcopy-tests.c    |  27 +++++-
 tests/qtest/migration/precopy-tests.c     |  38 +++++---
 tests/qtest/migration/tls-tests.c         |  51 ++++++++++-
 14 files changed, 247 insertions(+), 138 deletions(-)

--
2.48.1

[PATCH v6 1/4] migration/multifd: move macros to multifd header

[Prasad Pandit]

From: Prasad Pandit <pjp@fedoraproject.org>

Move MULTIFD_ macros to the header file so that
they are accessible from other source files.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
---
 migration/multifd.c | 5 -----
 migration/multifd.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

v5: no change
- https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t

diff --git a/migration/multifd.c b/migration/multifd.c
index ab73d6d984..97ce831775 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -33,11 +33,6 @@
 #include "io/channel-socket.h"
 #include "yank_functions.h"
 
-/* Multiple fd's */
-
-#define MULTIFD_MAGIC 0x11223344U
-#define MULTIFD_VERSION 1
-
 typedef struct {
     uint32_t magic;
     uint32_t version;
diff --git a/migration/multifd.h b/migration/multifd.h
index bd785b9873..10149af654 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -49,6 +49,11 @@ bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
 bool multifd_recv(void);
 MultiFDRecvData *multifd_get_recv_data(void);
 
+/* Multiple fd's */
+
+#define MULTIFD_MAGIC 0x11223344U
+#define MULTIFD_VERSION 1
+
 /* Multifd Compression flags */
 #define MULTIFD_FLAG_SYNC (1 << 0)
 
--
2.48.1

[PATCH v6 2/4] migration: enable multifd and postcopy together

[Prasad Pandit]

From: Prasad Pandit <pjp@fedoraproject.org>

Enable Multifd and Postcopy migration together.
The migration_ioc_process_incoming() routine
checks magic value sent on each channel and
helps to properly setup multifd and postcopy
channels.

The Precopy and Multifd threads work during the
initial guest RAM transfer. When migration moves
to the Postcopy phase, the multifd threads are
shutdown and Postcopy threads on the destination
request/pull data from the source side.

Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
---
 migration/migration.c      | 107 ++++++++++++++++++++-----------------
 migration/multifd-nocomp.c |   3 +-
 migration/multifd.c        |   4 +-
 migration/options.c        |   5 --
 migration/ram.c            |   7 ++-
 5 files changed, 64 insertions(+), 62 deletions(-)

v6:
- Shutdown multifd threads before postcopy_start()
- Reorder tests/qtest/migration/ patches
- Some refactoring of functions

v5:
- https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t

diff --git a/migration/migration.c b/migration/migration.c
index 396928513a..38697182e8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -95,6 +95,9 @@ enum mig_rp_message_type {
     MIG_RP_MSG_MAX
 };
 
+/* Migration channel types */
+enum { CH_MAIN, CH_MULTIFD, CH_POSTCOPY };
+
 /* When we add fault tolerance, we could have several
    migrations at once.  For now we don't need to add
    dynamic creation of migration */
@@ -959,28 +962,19 @@ void migration_fd_process_incoming(QEMUFile *f)
     migration_incoming_process();
 }
 
-/*
- * Returns true when we want to start a new incoming migration process,
- * false otherwise.
- */
-static bool migration_should_start_incoming(bool main_channel)
+static bool migration_has_main_and_multifd_channels(void)
 {
-    /* Multifd doesn't start unless all channels are established */
-    if (migrate_multifd()) {
-        return migration_has_all_channels();
+    MigrationIncomingState *mis = migration_incoming_get_current();
+    if (!mis->from_src_file) {
+        /* main channel not established */
+        return false;
     }
 
-    /* Preempt channel only starts when the main channel is created */
-    if (migrate_postcopy_preempt()) {
-        return main_channel;
+    if (migrate_multifd() && !multifd_recv_all_channels_created()) {
+        return false;
     }
 
-    /*
-     * For all the rest types of migration, we should only reach here when
-     * it's the main channel that's being created, and we should always
-     * proceed with this channel.
-     */
-    assert(main_channel);
+    /* main channel and all multifd channels are established */
     return true;
 }
 
@@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
     MigrationIncomingState *mis = migration_incoming_get_current();
     Error *local_err = NULL;
     QEMUFile *f;
-    bool default_channel = true;
     uint32_t channel_magic = 0;
+    uint8_t channel = CH_MAIN;
     int ret = 0;
 
-    if (migrate_multifd() && !migrate_mapped_ram() &&
-        !migrate_postcopy_ram() &&
-        qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
+    if (!migration_has_main_and_multifd_channels()) {
+        if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
         /*
          * With multiple channels, it is possible that we receive channels
          * out of order on destination side, causing incorrect mapping of
@@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
          * tls handshake while initializing main channel so with tls this
          * issue is not possible.
          */
-        ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
-                                          sizeof(channel_magic), errp);
+            ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
+                                              sizeof(channel_magic), errp);
+            if (ret != 0) {
+                return;
+            }
 
-        if (ret != 0) {
-            return;
+            if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
+                channel = CH_MAIN;
+            } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
+                channel = CH_MULTIFD;
+            } else if (!mis->from_src_file
+                        && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+                /* reconnect default channel for postcopy recovery */
+                channel = CH_MAIN;
+            } else {
+                error_report("%s: unknown channel magic: %u",
+                                __func__, channel_magic);
+                return;
+            }
+        } else if (mis->from_src_file
+                && (!strcmp(ioc->name, "migration-tls-incoming")
+                || !strcmp(ioc->name, "migration-file-incoming"))) {
+            channel = CH_MULTIFD;
         }
-
-        default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC));
-    } else {
-        default_channel = !mis->from_src_file;
+    } else if (mis->from_src_file) {
+        channel = CH_POSTCOPY;
     }
 
     if (multifd_recv_setup(errp) != 0) {
         return;
     }
 
-    if (default_channel) {
+    if (channel == CH_MAIN) {
         f = qemu_file_new_input(ioc);
         migration_incoming_setup(f);
-    } else {
+    } else if (channel == CH_MULTIFD) {
         /* Multiple connections */
-        assert(migration_needs_multiple_sockets());
         if (migrate_multifd()) {
             multifd_recv_new_channel(ioc, &local_err);
-        } else {
-            assert(migrate_postcopy_preempt());
-            f = qemu_file_new_input(ioc);
-            postcopy_preempt_new_channel(mis, f);
         }
         if (local_err) {
             error_propagate(errp, local_err);
             return;
         }
+    } else if (channel == CH_POSTCOPY) {
+        assert(migrate_postcopy_preempt());
+        assert(!mis->postcopy_qemufile_dst);
+        f = qemu_file_new_input(ioc);
+        postcopy_preempt_new_channel(mis, f);
     }
 
-    if (migration_should_start_incoming(default_channel)) {
+    if (channel != CH_POSTCOPY && migration_has_main_and_multifd_channels()) {
         /* If it's a recovery, we're done */
         if (postcopy_try_recover()) {
             return;
@@ -1058,20 +1067,15 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
  */
 bool migration_has_all_channels(void)
 {
+    if (!migration_has_main_and_multifd_channels()) {
+        return false;
+    }
+
     MigrationIncomingState *mis = migration_incoming_get_current();
-
-    if (!mis->from_src_file) {
+    if (migrate_postcopy_preempt() && !mis->postcopy_qemufile_dst) {
         return false;
     }
 
-    if (migrate_multifd()) {
-        return multifd_recv_all_channels_created();
-    }
-
-    if (migrate_postcopy_preempt()) {
-        return mis->postcopy_qemufile_dst != NULL;
-    }
-
     return true;
 }
 
@@ -1482,6 +1486,8 @@ static void migrate_fd_cleanup(MigrationState *s)
 
     assert(!migration_is_active());
 
+    file_cleanup_outgoing_migration();
+    socket_cleanup_outgoing_migration();
     if (s->state == MIGRATION_STATUS_CANCELLING) {
         migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                           MIGRATION_STATUS_CANCELLED);
@@ -3373,8 +3379,11 @@ static MigIterateState migration_iteration_run(MigrationState *s)
     }
 
     /* Still a significant amount to transfer */
-    if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover &&
-        qatomic_read(&s->start_postcopy)) {
+    if (!in_postcopy && must_precopy <= s->threshold_size
+        && can_switchover && qatomic_read(&s->start_postcopy)) {
+        if (migrate_multifd()) {
+            multifd_send_shutdown();
+        }
         if (postcopy_start(s, &local_err)) {
             migrate_set_error(s, local_err);
             error_report_err(local_err);
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index 1325dba97c..d0edec7cd1 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -16,6 +16,7 @@
 #include "file.h"
 #include "multifd.h"
 #include "options.h"
+#include "migration.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
@@ -391,7 +392,7 @@ int multifd_ram_flush_and_sync(QEMUFile *f)
     MultiFDSyncReq req;
     int ret;
 
-    if (!migrate_multifd()) {
+    if (!migrate_multifd() || migration_in_postcopy()) {
         return 0;
     }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index 97ce831775..fa83a43778 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -467,8 +467,6 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 
 static void multifd_send_cleanup_state(void)
 {
-    file_cleanup_outgoing_migration();
-    socket_cleanup_outgoing_migration();
     qemu_sem_destroy(&multifd_send_state->channels_created);
     qemu_sem_destroy(&multifd_send_state->channels_ready);
     g_free(multifd_send_state->params);
@@ -481,7 +479,7 @@ void multifd_send_shutdown(void)
 {
     int i;
 
-    if (!migrate_multifd()) {
+    if (!migrate_multifd() || !multifd_send_state) {
         return;
     }
 
diff --git a/migration/options.c b/migration/options.c
index 4db340b502..c4dfe89edd 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -480,11 +480,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
             error_setg(errp, "Postcopy is not compatible with ignore-shared");
             return false;
         }
-
-        if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
-            error_setg(errp, "Postcopy is not yet compatible with multifd");
-            return false;
-        }
     }
 
     if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
diff --git a/migration/ram.c b/migration/ram.c
index 6f460fd22d..8f22745aba 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1297,7 +1297,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
         pss->page = 0;
         pss->block = QLIST_NEXT_RCU(pss->block, next);
         if (!pss->block) {
-            if (multifd_ram_sync_per_round()) {
+            if (multifd_ram_sync_per_round() && !migration_in_postcopy()) {
                 QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
                 int ret = multifd_ram_flush_and_sync(f);
                 if (ret < 0) {
@@ -1971,9 +1971,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
         }
     }
 
-    if (migrate_multifd()) {
-        RAMBlock *block = pss->block;
-        return ram_save_multifd_page(block, offset);
+    if (migrate_multifd() && !migration_in_postcopy()) {
+        return ram_save_multifd_page(pss->block, offset);
     }
 
     if (control_save_page(pss, offset, &res)) {
--
2.48.1

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> From: Prasad Pandit <pjp@fedoraproject.org>
>
> Enable Multifd and Postcopy migration together.
> The migration_ioc_process_incoming() routine
> checks magic value sent on each channel and
> helps to properly setup multifd and postcopy
> channels.
>
> The Precopy and Multifd threads work during the
> initial guest RAM transfer. When migration moves
> to the Postcopy phase, the multifd threads are
> shutdown and Postcopy threads on the destination
> request/pull data from the source side.
>
> Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
> ---
>  migration/migration.c      | 107 ++++++++++++++++++++-----------------
>  migration/multifd-nocomp.c |   3 +-
>  migration/multifd.c        |   4 +-
>  migration/options.c        |   5 --
>  migration/ram.c            |   7 ++-
>  5 files changed, 64 insertions(+), 62 deletions(-)
>
> v6:
> - Shutdown multifd threads before postcopy_start()
> - Reorder tests/qtest/migration/ patches
> - Some refactoring of functions
>
> v5:
> - https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 396928513a..38697182e8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -95,6 +95,9 @@ enum mig_rp_message_type {
>      MIG_RP_MSG_MAX
>  };
>  
> +/* Migration channel types */
> +enum { CH_MAIN, CH_MULTIFD, CH_POSTCOPY };
> +
>  /* When we add fault tolerance, we could have several
>     migrations at once.  For now we don't need to add
>     dynamic creation of migration */
> @@ -959,28 +962,19 @@ void migration_fd_process_incoming(QEMUFile *f)
>      migration_incoming_process();
>  }
>  
> -/*
> - * Returns true when we want to start a new incoming migration process,
> - * false otherwise.
> - */
> -static bool migration_should_start_incoming(bool main_channel)
> +static bool migration_has_main_and_multifd_channels(void)
>  {
> -    /* Multifd doesn't start unless all channels are established */
> -    if (migrate_multifd()) {
> -        return migration_has_all_channels();
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    if (!mis->from_src_file) {
> +        /* main channel not established */
> +        return false;
>      }
>  
> -    /* Preempt channel only starts when the main channel is created */
> -    if (migrate_postcopy_preempt()) {
> -        return main_channel;
> +    if (migrate_multifd() && !multifd_recv_all_channels_created()) {
> +        return false;
>      }
>  
> -    /*
> -     * For all the rest types of migration, we should only reach here when
> -     * it's the main channel that's being created, and we should always
> -     * proceed with this channel.
> -     */
> -    assert(main_channel);
> +    /* main channel and all multifd channels are established */
>      return true;
>  }

How will this avoid peeking the preempt channel? You're assuming preempt
is mutually exclusive with multifd it seems. Otherwise you could get the
preempt channel creation racing with multifd channels creation.

>  
> @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      Error *local_err = NULL;
>      QEMUFile *f;
> -    bool default_channel = true;
>      uint32_t channel_magic = 0;
> +    uint8_t channel = CH_MAIN;
>      int ret = 0;
>  
> -    if (migrate_multifd() && !migrate_mapped_ram() &&
> -        !migrate_postcopy_ram() &&
> -        qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> +    if (!migration_has_main_and_multifd_channels()) {

I'm not entirely sure we need these checks here. They will happen anyway
later. Could this be replaced by migration_needs_multiple_sockets()
instead?

And I'd put this whole channel discovery business in channel.c since
it's encoding several assumptions about channels. Some helpers used here
might need to be exported, but that's ok.

Also, please make a separate patch, we need to be really confident that
changing the discovery code around won't introduce any regression, and
if it does, we'll want it separate from the postcopy+multifd
enablement. It's ok if you have the patch assume that multifd+postcopy
will happen later in the series.

> +        if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
>          /*
>           * With multiple channels, it is possible that we receive channels
>           * out of order on destination side, causing incorrect mapping of
> @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>           * tls handshake while initializing main channel so with tls this
>           * issue is not possible.
>           */

This comment block needs to be indented properly.

> -        ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> -                                          sizeof(channel_magic), errp);
> +            ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> +                                              sizeof(channel_magic), errp);
> +            if (ret != 0) {
> +                return;
> +            }
>  
> -        if (ret != 0) {
> -            return;
> +            if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
> +                channel = CH_MAIN;
> +            } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
> +                channel = CH_MULTIFD;
> +            } else if (!mis->from_src_file
> +                        && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {

The usual style is to keep the && on the previous line.

> +                /* reconnect default channel for postcopy recovery */

s/default/main/

> +                channel = CH_MAIN;
> +            } else {
> +                error_report("%s: unknown channel magic: %u",
> +                                __func__, channel_magic);
> +                return;

This needs to set errp instead of reporting.

> +            }
> +        } else if (mis->from_src_file
> +                && (!strcmp(ioc->name, "migration-tls-incoming")
> +                || !strcmp(ioc->name, "migration-file-incoming"))) {
> +            channel = CH_MULTIFD;

This is quite misleading. These channels are used without multifd as
well. For instance, file-backed fd migration goes past this because
!mis->from_src_file but it still uses the file channel.

I agree with Peter that checking for channel names is not ideal. I don't
see an alternative at the moment (hiding the assumption is of course not
a fix). Maybe check migrate_multifd() here and explain in a comment that
at the moment, the non-peekable channels happen to be used with multifd
only.

>          }

No else clause here and in the rest of the patch makes this is as opaque
as the previous version, IMO. We need to define what's supposed to
happen whenever the conditionals don't match. Is it an error,
g_assert_not_reached(), a fallthrough? Better to set CH_MAIN explicitly
wherenever that's the case.

> -
> -        default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC));
> -    } else {
> -        default_channel = !mis->from_src_file;
> +    } else if (mis->from_src_file) {
> +        channel = CH_POSTCOPY;
>      }

Same here.

>  
>      if (multifd_recv_setup(errp) != 0) {
>          return;
>      }
>  
> -    if (default_channel) {
> +    if (channel == CH_MAIN) {
>          f = qemu_file_new_input(ioc);
>          migration_incoming_setup(f);
> -    } else {
> +    } else if (channel == CH_MULTIFD) {
>          /* Multiple connections */
> -        assert(migration_needs_multiple_sockets());
>          if (migrate_multifd()) {
>              multifd_recv_new_channel(ioc, &local_err);
> -        } else {
> -            assert(migrate_postcopy_preempt());
> -            f = qemu_file_new_input(ioc);
> -            postcopy_preempt_new_channel(mis, f);
>          }
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
>          }
> +    } else if (channel == CH_POSTCOPY) {
> +        assert(migrate_postcopy_preempt());
> +        assert(!mis->postcopy_qemufile_dst);
> +        f = qemu_file_new_input(ioc);
> +        postcopy_preempt_new_channel(mis, f);
>      }

else {
   g_assert_not_reached();
}

>  
> -    if (migration_should_start_incoming(default_channel)) {
> +    if (channel != CH_POSTCOPY && migration_has_main_and_multifd_channels()) {

You could check CH_POSTCOPY first in the block above this one and return
early.

>          /* If it's a recovery, we're done */
>          if (postcopy_try_recover()) {
>              return;
> @@ -1058,20 +1067,15 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>   */
>  bool migration_has_all_channels(void)
>  {
> +    if (!migration_has_main_and_multifd_channels()) {
> +        return false;
> +    }
> +
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -
> -    if (!mis->from_src_file) {
> +    if (migrate_postcopy_preempt() && !mis->postcopy_qemufile_dst) {
>          return false;
>      }
>  
> -    if (migrate_multifd()) {
> -        return multifd_recv_all_channels_created();
> -    }
> -
> -    if (migrate_postcopy_preempt()) {
> -        return mis->postcopy_qemufile_dst != NULL;
> -    }
> -
>      return true;
>  }
>  
> @@ -1482,6 +1486,8 @@ static void migrate_fd_cleanup(MigrationState *s)
>  
>      assert(!migration_is_active());
>  
> +    file_cleanup_outgoing_migration();
> +    socket_cleanup_outgoing_migration();
>      if (s->state == MIGRATION_STATUS_CANCELLING) {
>          migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
>                            MIGRATION_STATUS_CANCELLED);
> @@ -3373,8 +3379,11 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>      }
>  
>      /* Still a significant amount to transfer */
> -    if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover &&
> -        qatomic_read(&s->start_postcopy)) {
> +    if (!in_postcopy && must_precopy <= s->threshold_size
> +        && can_switchover && qatomic_read(&s->start_postcopy)) {
> +        if (migrate_multifd()) {
> +            multifd_send_shutdown();
> +        }
>          if (postcopy_start(s, &local_err)) {
>              migrate_set_error(s, local_err);
>              error_report_err(local_err);
> diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
> index 1325dba97c..d0edec7cd1 100644
> --- a/migration/multifd-nocomp.c
> +++ b/migration/multifd-nocomp.c
> @@ -16,6 +16,7 @@
>  #include "file.h"
>  #include "multifd.h"
>  #include "options.h"
> +#include "migration.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/error-report.h"
> @@ -391,7 +392,7 @@ int multifd_ram_flush_and_sync(QEMUFile *f)
>      MultiFDSyncReq req;
>      int ret;
>  
> -    if (!migrate_multifd()) {
> +    if (!migrate_multifd() || migration_in_postcopy()) {
>          return 0;
>      }
>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 97ce831775..fa83a43778 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -467,8 +467,6 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>  
>  static void multifd_send_cleanup_state(void)
>  {
> -    file_cleanup_outgoing_migration();
> -    socket_cleanup_outgoing_migration();
>      qemu_sem_destroy(&multifd_send_state->channels_created);
>      qemu_sem_destroy(&multifd_send_state->channels_ready);
>      g_free(multifd_send_state->params);
> @@ -481,7 +479,7 @@ void multifd_send_shutdown(void)
>  {
>      int i;
>  
> -    if (!migrate_multifd()) {
> +    if (!migrate_multifd() || !multifd_send_state) {
>          return;
>      }
>  
> diff --git a/migration/options.c b/migration/options.c
> index 4db340b502..c4dfe89edd 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -480,11 +480,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>              error_setg(errp, "Postcopy is not compatible with ignore-shared");
>              return false;
>          }
> -
> -        if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
> -            error_setg(errp, "Postcopy is not yet compatible with multifd");
> -            return false;
> -        }
>      }
>  
>      if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 6f460fd22d..8f22745aba 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1297,7 +1297,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>          pss->page = 0;
>          pss->block = QLIST_NEXT_RCU(pss->block, next);
>          if (!pss->block) {
> -            if (multifd_ram_sync_per_round()) {
> +            if (multifd_ram_sync_per_round() && !migration_in_postcopy()) {
>                  QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
>                  int ret = multifd_ram_flush_and_sync(f);
>                  if (ret < 0) {
> @@ -1971,9 +1971,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
>          }
>      }
>  
> -    if (migrate_multifd()) {
> -        RAMBlock *block = pss->block;
> -        return ram_save_multifd_page(block, offset);
> +    if (migrate_multifd() && !migration_in_postcopy()) {
> +        return ram_save_multifd_page(pss->block, offset);
>      }
>  
>      if (control_save_page(pss, offset, &res)) {
> --
> 2.48.1

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Prasad Pandit]

Hello Fabiano,

On Tue, 18 Feb 2025 at 03:20, Fabiano Rosas <farosas@suse.de> wrote:
> > -static bool migration_should_start_incoming(bool main_channel)
> > +static bool migration_has_main_and_multifd_channels(void)
> >  {
...
> > +    /* main channel and all multifd channels are established */
> >      return true;
> >  }
>
> How will this avoid peeking the preempt channel? You're assuming preempt
> is mutually exclusive with multifd it seems. Otherwise you could get the
> preempt channel creation racing with multifd channels creation.

* IIUC postcopy preempt channel is created towards the end of the
migration when the Postcopy phase starts. At that time, the 'main' and
'multifd' channels are already established and working. Having the
'main' and when multifd is enabled 'multifd' channels in place is a
requirement for starting new migration. So when both the 'main' and
'multifd' channels are established, the new incoming connection is
seen as the 'postcopy' one; And it falls to the 'else' block of the
'if' conditional ->  if (!migration_has_main_and_multifd_channels()) {

* When does postcopy preempt channel creation race with the multifd
channel creation?

> > @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
> > +    if (!migration_has_main_and_multifd_channels()) {
> I'm not entirely sure we need these checks here. They will happen anyway
> later. Could this be replaced by migration_needs_multiple_sockets()
> instead?

* migration_needs_multiple_sockets() => return migrate_multifd() ||
migrate_postcopy_preempt();

* That logical OR should have been AND, no? It returns true even when
one of them is true. That's not multiple types (multifd/postcopy) of
sockets. I don't think it helps much.

* Let's try this:
    - First differentiation: peekable Vs non-peekable channels
    - Peekable channels
         - Main
         - Multifd
    - Non-peekable channels
         - Postcopy preempt
         - TLS
         - File/mapped_ram

    if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK))
    {
            if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
                channel = CH_MAIN;
            } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
                channel = CH_MULTIFD;
    } else {
            if (!strcmp(ioc->name, "migration-tls-incoming")
                || !strcmp(ioc->name, "migration-file-incoming"))) {
               channel = CH_MULTIFD;
            } else {
               channel = CH_POSTCOPY;
            }
    }

* With above, the 'main' channel shall have to send 'magic value' even
for reconnection during the postcopy recovery phase. If all channels
were consistent and sent a magic value, this code would be much
simpler and we may not have to care/worry about the _order_ in which
these connections are made.

   if (channel == 'main')
       process_main_channel()
   else if (channel == 'multifd')
       process_multifd_channel()
   else if (channel == 'tls')
       process_tls_channel()
    else if (channel == 'file')
       process_file_channel()
    else if (channel == 'postcopy')
       process_postcopy_channel()

> And I'd put this whole channel discovery business in channel.c since
> it's encoding several assumptions about channels. Some helpers used here
> might need to be exported, but that's ok.
>
> Also, please make a separate patch, we need to be really confident that
> changing the discovery code around won't introduce any regression, and
> if it does, we'll want it separate from the postcopy+multifd
> enablement. It's ok if you have the patch assume that multifd+postcopy
> will happen later in the series.

* TBH, I think we have complicated this whole thing with multiple
channel types, their inconsistent behaviour and dependence on the
_order_ in which connections are made. Do we really need channel
types? Could we consider rationalising them?

> > +        if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> >          /*
> >           * With multiple channels, it is possible that we receive channels
> >           * out of order on destination side, causing incorrect mapping of
> > @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
> >           * tls handshake while initializing main channel so with tls this
> >           * issue is not possible.
> >           */
>
> This comment block needs to be indented properly.
>
> > -        ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> > -                                          sizeof(channel_magic), errp);
> > +            ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> > +                                              sizeof(channel_magic), errp);
> > +            if (ret != 0) {
> > +                return;
> > +            }
> >
> > -        if (ret != 0) {
> > -            return;
> > +            if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
> > +                channel = CH_MAIN;
> > +            } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
> > +                channel = CH_MULTIFD;
> > +            } else if (!mis->from_src_file
> > +                        && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>
> The usual style is to keep the && on the previous line.
>
> > +                /* reconnect default channel for postcopy recovery */
>
> s/default/main/

* Okay, will fix these.


> > +                channel = CH_MAIN;
> > +            } else {
> > +                error_report("%s: unknown channel magic: %u",
> > +                                __func__, channel_magic);
> > +                return;
>
> This needs to set errp instead of reporting.

* Okay.

> > +            }
> > +        } else if (mis->from_src_file
> > +                && (!strcmp(ioc->name, "migration-tls-incoming")
> > +                || !strcmp(ioc->name, "migration-file-incoming"))) {
> > +            channel = CH_MULTIFD;
>
> This is quite misleading. These channels are used without multifd as
> well. For instance, file-backed fd migration goes past this because
> !mis->from_src_file but it still uses the file channel.
>
> I agree with Peter that checking for channel names is not ideal. I don't
> see an alternative at the moment (hiding the assumption is of course not
> a fix). Maybe check migrate_multifd() here and explain in a comment that
> at the moment, the non-peekable channels happen to be used with multifd
> only.

* The first paragraph says these channels are used without
migrate_multifd(); And the second paragraph says they are used with
migrate_multifd() only....??
===
} else {
        /* Multiple connections */
        assert(migration_needs_multiple_sockets());
        if (migrate_multifd()) {
            multifd_recv_new_channel(ioc, &local_err);
        } else {
            assert(migrate_postcopy_preempt());
            f = qemu_file_new_input(ioc);
            postcopy_preempt_new_channel(mis, f);
        }
===
* IIUC in the current code, when migrate_multifd() is true, these
channels get processed via - multifd_recv_new_channel(). And when
migrate_multifd() is false, it falls to postcopy_preempt_new_channel()
part. Not sure if/how that works really. It is possible that currently
these (tls/file) channels are used only with migrate_multifd() enabled
and so are processed with multifd_recv_new_channel() function. The
current patch handles them the same way.

* About checking channel names, in the non-peekable category above,
how do we differentiate between 'TLS', 'File' and 'Postcopy' channels?
Without magic values, we don't have much choice really.  And seeing
those names in the code also tells the reader that 'TLS' and 'File'
channels are processed as CH_MULTIFD via - multifd_recv_new_channel().

> No else clause here and in the rest of the patch makes this is as opaque
> as the previous version, IMO. We need to define what's supposed to
> happen whenever the conditionals don't match. Is it an error,
> g_assert_not_reached(), a fallthrough? Better to set CH_MAIN explicitly
> wherenever that's the case.

* I'd say let's treat unmatched conditions as an error and return.

> > -        default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC));
> > -    } else {
> > -        default_channel = !mis->from_src_file;
> > +    } else if (mis->from_src_file) {
> > +        channel = CH_POSTCOPY;
> >      }
>
> Same here.

* Here final else means the main channel (mis->from_src_file) is not
established, so it defaults to CH_MAIN.

>
> You could check CH_POSTCOPY first in the block above this one and return
> early.
>

* Okay.

* My request is the same - let's think about having consistent channel
behaviour. The restructuring and overhauling of the channel processing
part could be done separately, outside the current scope of enabling
multifd+postcopy together with this series. Let's not try to fix
everything in one go, in one series.


Thank you.
---
  - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> Hello Fabiano,
>
> On Tue, 18 Feb 2025 at 03:20, Fabiano Rosas <farosas@suse.de> wrote:
>> > -static bool migration_should_start_incoming(bool main_channel)
>> > +static bool migration_has_main_and_multifd_channels(void)
>> >  {
> ...
>> > +    /* main channel and all multifd channels are established */
>> >      return true;
>> >  }
>>
>> How will this avoid peeking the preempt channel? You're assuming preempt
>> is mutually exclusive with multifd it seems. Otherwise you could get the
>> preempt channel creation racing with multifd channels creation.
>
> * IIUC postcopy preempt channel is created towards the end of the
> migration when the Postcopy phase starts. At that time, the 'main' and
> 'multifd' channels are already established and working. Having the
> 'main' and when multifd is enabled 'multifd' channels in place is a
> requirement for starting new migration. So when both the 'main' and
> 'multifd' channels are established, the new incoming connection is
> seen as the 'postcopy' one; And it falls to the 'else' block of the
> 'if' conditional ->  if (!migration_has_main_and_multifd_channels()) {
>

Do you concede that this code has a hidden assumption? Either that
migrate_multifd() != migrate_postcopy_preempt() or that multifd channels
must be set up before postcopy preempt channel? Because that is enough
for us to have to do something about it. Either restructuring or a
comment explaining.

> * When does postcopy preempt channel creation race with the multifd
> channel creation?
>

Well, taking that this is the destination side, I don't think we can
predict when the other end of the migration will open a channel, can we?
For instance, postcopy_do_resume() has this comment:

    /*
     * If preempt is enabled, re-establish the preempt channel.  Note that
     * we do it after resume prepare to make sure the main channel will be
     * created before the preempt channel.  E.g. with weak network, the
     * dest QEMU may get messed up with the preempt and main channels on
     * the order of connection setup.  This guarantees the correct order.
     */

It looks like if the main channel can race, so do the multifd channels,
no?

In any case, I'm fine with just documenting any assumption for now.

>> > @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>> > +    if (!migration_has_main_and_multifd_channels()) {
>> I'm not entirely sure we need these checks here. They will happen anyway
>> later. Could this be replaced by migration_needs_multiple_sockets()
>> instead?
>
> * migration_needs_multiple_sockets() => return migrate_multifd() ||
> migrate_postcopy_preempt();
>
> * That logical OR should have been AND, no? It returns true even when
> one of them is true. That's not multiple types (multifd/postcopy) of
> sockets. I don't think it helps much.
>

Nope, this is just saying whether a single channel is expected, or more
than one. That's why I think it would be a good gate for this peeking
code. Since postcopy preempt could be a peekable channel, it's
misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK
only. This is a time-bomb for the next person to refactor this code.

> * Let's try this:
>     - First differentiation: peekable Vs non-peekable channels

As I said above, this is not entirely what we're doing.

>     - Peekable channels
>          - Main
>          - Multifd
>     - Non-peekable channels
>          - Postcopy preempt
>          - TLS
>          - File/mapped_ram
>
>     if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK))
>     {
>             if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
>                 channel = CH_MAIN;
>             } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
>                 channel = CH_MULTIFD;
>     } else {
>             if (!strcmp(ioc->name, "migration-tls-incoming")
>                 || !strcmp(ioc->name, "migration-file-incoming"))) {
>                channel = CH_MULTIFD;
>             } else {
>                channel = CH_POSTCOPY;
>             }
>     }
>
> * With above, the 'main' channel shall have to send 'magic value' even
> for reconnection during the postcopy recovery phase. If all channels
> were consistent and sent a magic value, this code would be much
> simpler and we may not have to care/worry about the _order_ in which
> these connections are made.
>

Right, but that's not what we have today. Changing this requires
figuring out how to keep the stream compatible when channels now start
sending extra stuff at the start. It's not trivial. There's also
mapped-ram which is asynchronous and there might be something special to
be done about the TLS handshake, I'm not sure.

>    if (channel == 'main')
>        process_main_channel()
>    else if (channel == 'multifd')
>        process_multifd_channel()
>    else if (channel == 'tls')
>        process_tls_channel()
>     else if (channel == 'file')
>        process_file_channel()
>     else if (channel == 'postcopy')
>        process_postcopy_channel()
>
>> And I'd put this whole channel discovery business in channel.c since
>> it's encoding several assumptions about channels. Some helpers used here
>> might need to be exported, but that's ok.
>>
>> Also, please make a separate patch, we need to be really confident that
>> changing the discovery code around won't introduce any regression, and
>> if it does, we'll want it separate from the postcopy+multifd
>> enablement. It's ok if you have the patch assume that multifd+postcopy
>> will happen later in the series.
>
> * TBH, I think we have complicated this whole thing with multiple
> channel types, their inconsistent behaviour and dependence on the
> _order_ in which connections are made. Do we really need channel
> types? Could we consider rationalising them?
>

Well, aside from preempt, they're *not* dependent on the order. That's
the point of having to do all of this dance. In fact we might be better
off if we could serialize the connections somehow.

I havent't followed this series closely, could you point me to the
discussion that led to the channels concept being introduced?

>> > +        if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
>> >          /*
>> >           * With multiple channels, it is possible that we receive channels
>> >           * out of order on destination side, causing incorrect mapping of
>> > @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>> >           * tls handshake while initializing main channel so with tls this
>> >           * issue is not possible.
>> >           */
>>
>> This comment block needs to be indented properly.
>>
>> > -        ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
>> > -                                          sizeof(channel_magic), errp);
>> > +            ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
>> > +                                              sizeof(channel_magic), errp);
>> > +            if (ret != 0) {
>> > +                return;
>> > +            }
>> >
>> > -        if (ret != 0) {
>> > -            return;
>> > +            if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
>> > +                channel = CH_MAIN;
>> > +            } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
>> > +                channel = CH_MULTIFD;
>> > +            } else if (!mis->from_src_file
>> > +                        && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>
>> The usual style is to keep the && on the previous line.
>>
>> > +                /* reconnect default channel for postcopy recovery */
>>
>> s/default/main/
>
> * Okay, will fix these.
>
>
>> > +                channel = CH_MAIN;
>> > +            } else {
>> > +                error_report("%s: unknown channel magic: %u",
>> > +                                __func__, channel_magic);
>> > +                return;
>>
>> This needs to set errp instead of reporting.
>
> * Okay.
>
>> > +            }
>> > +        } else if (mis->from_src_file
>> > +                && (!strcmp(ioc->name, "migration-tls-incoming")
>> > +                || !strcmp(ioc->name, "migration-file-incoming"))) {
>> > +            channel = CH_MULTIFD;
>>
>> This is quite misleading. These channels are used without multifd as
>> well. For instance, file-backed fd migration goes past this because
>> !mis->from_src_file but it still uses the file channel.
>>
>> I agree with Peter that checking for channel names is not ideal. I don't
>> see an alternative at the moment (hiding the assumption is of course not
>> a fix). Maybe check migrate_multifd() here and explain in a comment that
>> at the moment, the non-peekable channels happen to be used with multifd
>> only.
>
> * The first paragraph says these channels are used without
> migrate_multifd(); And the second paragraph says they are used with
> migrate_multifd() only....??

Yes. They *can* be used without multifd. The comment would explain that
at that point in the code, these are the only types possible. So as to
not mislead future readers that whenever tls/file, then multifd must be
used.

> ===
> } else {
>         /* Multiple connections */
>         assert(migration_needs_multiple_sockets());
>         if (migrate_multifd()) {
>             multifd_recv_new_channel(ioc, &local_err);
>         } else {
>             assert(migrate_postcopy_preempt());
>             f = qemu_file_new_input(ioc);
>             postcopy_preempt_new_channel(mis, f);
>         }

See? Multifd mutually exclusive with postcopy preempt. You carried that
assumption (well done), but made it more subtle (not good), since
if/else is by definition showing the relationship between the two while
migration_has_main_and_multifd_channels() makes it hidden under the
multifd check allowing the last return true to happen.

If we're enabling multifd along with postcopy, we need to be aware that
the relationship with preempt might not hold true anymore.

> ===
> * IIUC in the current code, when migrate_multifd() is true, these
> channels get processed via - multifd_recv_new_channel(). And when
> migrate_multifd() is false, it falls to postcopy_preempt_new_channel()
> part.

This relies on the fact that the management layer will certainly have
set multifd on both sides of the migration and that multifd will not be
used with postcopy.

>Not sure if/how that works really. It is possible that currently
> these (tls/file) channels are used only with migrate_multifd() enabled
> and so are processed with multifd_recv_new_channel() function. The
> current patch handles them the same way.
>

That's the entire point I'm making when I ask to not omit the else
clauses. The tls and file channels can be the main channel as well, but
that is never explicit in the code. That uint8 channel = CH_MAIN up
there is doing some heavy-lifting.

> * About checking channel names, in the non-peekable category above,
> how do we differentiate between 'TLS', 'File' and 'Postcopy' channels?
> Without magic values, we don't have much choice really.

Indeed.

> And seeing
> those names in the code also tells the reader that 'TLS' and 'File'
> channels are processed as CH_MULTIFD via - multifd_recv_new_channel().

We shouldn't rely on the underlying iochannel type. That's why they
expose the QIO_CHANNEL_FEATUREs. It's fine if we double check at that
point that the channels are either file/tls, but that cannot be the
algorithm. The current code can work without relying on it after all and
I don't think your postcopy enablement affects that particular part.

>
>> No else clause here and in the rest of the patch makes this is as opaque
>> as the previous version, IMO. We need to define what's supposed to
>> happen whenever the conditionals don't match. Is it an error,
>> g_assert_not_reached(), a fallthrough? Better to set CH_MAIN explicitly
>> wherenever that's the case.
>
> * I'd say let's treat unmatched conditions as an error and return.
>
>> > -        default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC));
>> > -    } else {
>> > -        default_channel = !mis->from_src_file;
>> > +    } else if (mis->from_src_file) {
>> > +        channel = CH_POSTCOPY;
>> >      }
>>
>> Same here.
>
> * Here final else means the main channel (mis->from_src_file) is not
> established, so it defaults to CH_MAIN.
>

Yes, but let's make it explicit. We have this thing in our minds now. In
6 months time no one remembers anything anymore.

>>
>> You could check CH_POSTCOPY first in the block above this one and return
>> early.
>>
>
> * Okay.
>
> * My request is the same - let's think about having consistent channel
> behaviour. The restructuring and overhauling of the channel processing
> part could be done separately, outside the current scope of enabling
> multifd+postcopy together with this series. Let's not try to fix
> everything in one go, in one series.
>

Ok, no one is asking for this to be done in one go. You're free to split
any side efforts into a new series and proceed with that. Having to do
prerequisite work is pretty common.

Do you think this series can work without touching the channel discovery
code? As I said earlier, I'm missing a bit of context, but to me it
seems it cannot.

If instead of this refactoring you want to start working on a model for
consistent channel advertisement, then that's fine. But we'll have to
put this series on hold (which is also fine). It also looks like it
could be considerably more work, although I haven't looked at it in
detail. Granted, it's work that makes sense, instead of the heuristics
we have today.

>
> Thank you.
> ---
>   - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Prasad Pandit]

Hello Fabiano,

On Tue, 18 Feb 2025 at 19:52, Fabiano Rosas <farosas@suse.de> wrote:
> Do you concede that this code has a hidden assumption? Either that
> migrate_multifd() != migrate_postcopy_preempt() or that multifd channels
> must be set up before postcopy preempt channel? Because that is enough
> for us to have to do something about it. Either restructuring or a
> comment explaining.

* Not a hidden assumption, but it is an observation that 'main' and
'multifd' channels are established before 'postcopy' ones. And for new
migration to start, it is necessary that 'main' and 'multifd' channels
(when enabled) are established before migration starts.

> > * When does postcopy preempt channel creation race with the multifd
> > channel creation?
>
> For instance, postcopy_do_resume() has this comment:
>     /*
>      * If preempt is enabled, re-establish the preempt channel.  Note that
>      * we do it after resume prepare to make sure the main channel will be
>      * created before the preempt channel.  E.g. with weak network, the
>      * dest QEMU may get messed up with the preempt and main channels on
>      * the order of connection setup.  This guarantees the correct order.
>      */
> It looks like if the main channel can race, so do the multifd channels,
> no? In any case, I'm fine with just documenting any assumption for now.

* The first requirement for this race to occur is that two types of
channels are created together at the same time. Let's see:

   * Postcopy migration:  without multifd enabled
      - 'main' channel is created before the migration starts. And
'postcopy' channels are created towards the end of precopy migration,
when the Postcopy phase starts. So in this scenario the race does not
happen.

   * Postcopy resume: without multifd enabled
      - As described in the comment above, preempt channel is created
_after_ the 'main' channel to avoid the race condition.

   * Postcopy migration: with multifd enabled
      - 'main' and 'multifd' channels are created before migration
starts. And 'postcopy' channels are created towards the end of precopy
migration, when the Postcopy phase starts. No race occurs.

   * Postcopy resume: with multifd enabled
      - 'multifd' channels are shutdown before Postcopy starts, ie. no
'multifd' channels exist during Postcopy resume. So no race between
'postcopy' and 'multifd' channels.
      - And 'postcopy' channels are created after the 'main' channel
to avoid the race between them.
      - postcopy_do_resume() does not seem to create 'multifd' channels.

   * Multifd migration: without Postcopy enabled
      - 'main' and 'multifd' channels are created before the migration
starts. They both send 'magic value' bytes, so are easier to
differentiate. No race occurs.


> > * migration_needs_multiple_sockets() => return migrate_multifd() ||
> > migrate_postcopy_preempt();
> >
> Nope, this is just saying whether a single channel is expected, or more
> than one.

* If we read it as a question:
    - migration_needs_multiple_sockets() ? True => Yes, migration
needs multiple sockets.
    - migration_needs_multiple_sockets() ? False => No, migration does
not need multiple sockets.

Then it should return 'True' when both migrate_multifd() and
postcopy_preempt() are enabled.

>That's why I think it would be a good gate for this peeking
> code. Since postcopy preempt could be a peekable channel, it's
> misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK
> only. This is a time-bomb for the next person to refactor this code.

* Postcopy preempt could be a peekable channel ? Currently it does not
send magic value, does it?

> Right, but that's not what we have today. Changing this requires
> figuring out how to keep the stream compatible when channels now start
> sending extra stuff at the start. It's not trivial. There's also
> mapped-ram which is asynchronous and there might be something special to
> be done about the TLS handshake, I'm not sure.

* True, it's not trivial.

> Well, aside from preempt, they're *not* dependent on the order. That's
> the point of having to do all of this dance. In fact we might be better
> off if we could serialize the connections somehow.
>
> I havent't followed this series closely, could you point me to the
> discussion that led to the channels concept being introduced?

* Channels concept was not introduced in this series. It has been
there since the beginning, no?

> Yes. They *can* be used without multifd. The comment would explain that
> at that point in the code, these are the only types possible. So as to
> not mislead future readers that whenever tls/file, then multifd must be
> used.
....
> See? Multifd mutually exclusive with postcopy preempt. You carried that
> assumption (well done), but made it more subtle (not good), since
> if/else is by definition showing the relationship between the two while
> migration_has_main_and_multifd_channels() makes it hidden under the
> multifd check allowing the last return true to happen.
>
> If we're enabling multifd along with postcopy, we need to be aware that
> the relationship with preempt might not hold true anymore.

* Sorry, I did not get that. Enabling them together means that they
are _not_ exclusive, no? It is not Either 'multifd'  OR 'postcopy'
case, anymore.

>>Not sure if/how that works really. It is possible that currently
>> these (tls/file) channels are used only with migrate_multifd() enabled
>> and so are processed with multifd_recv_new_channel() function. The
>> current patch handles them the same way.
>
> That's the entire point I'm making when I ask to not omit the else
> clauses.

* ie. we set 'channel = CH_MAIN' in the final else clause as well? - Okay.

> Do you think this series can work without touching the channel discovery
> code? As I said earlier, I'm missing a bit of context, but to me it
> seems it cannot.

* The reason we need to touch the channel discovery part is: with
'multifd' and 'postcopy' both enabled, towards the end of migration,
when 'postcopy' connection comes in
     migration_ioc_process_incoming(...)
    {
        if (migrate_multifd() && !migrate_mapped_ram() &&
!migrate_postcopy_ram() &&
             qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
             ...
         } else {
             default_channel = !mis->from_src_file;
         }
        ...
        if (migrate_multifd()) {
            multifd_recv_new_channel(ioc, &local_err);
        } else {
           postcopy_preempt_new_channel(mis, f);
        }
    }

* The first 'if (migrate_multifd() ... !migrate_postcopy_ram()')
evaluates to false, in the else part 'default_channel' also evaluates
to false, because the 'main' channel is established. Now the new
incoming connection falls in the second migrate_multifd() block and
gets processed via - multifd_recv_new_channel(ioc, &local_err); call
and migration would not complete/finish.

* To identify the incoming postcopy connection, in the very first
version of this series, a magic value for the postcopy channel was
introduced and everything else remained the same.

* Would that be an acceptable solution for now?

> If instead of this refactoring you want to start working on a model for
> consistent channel advertisement, then that's fine. But we'll have to
> put this series on hold (which is also fine). It also looks like it
> could be considerably more work, although I haven't looked at it in
> detail. Granted, it's work that makes sense, instead of the heuristics
> we have today.

* IMHO, we need not put this series on hold, for now we could go ahead
with the postcopy magic value patch if that works. And the larger
overhaul of the channel discovery part could be done as a separate
series of its own.

Thank you.
---
  - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> Hello Fabiano,
>
> On Tue, 18 Feb 2025 at 19:52, Fabiano Rosas <farosas@suse.de> wrote:
>> Do you concede that this code has a hidden assumption? Either that
>> migrate_multifd() != migrate_postcopy_preempt() or that multifd channels
>> must be set up before postcopy preempt channel? Because that is enough
>> for us to have to do something about it. Either restructuring or a
>> comment explaining.
>
> * Not a hidden assumption, but it is an observation that 'main' and
> 'multifd' channels are established before 'postcopy' ones. And for new
> migration to start, it is necessary that 'main' and 'multifd' channels
> (when enabled) are established before migration starts.
>
>> > * When does postcopy preempt channel creation race with the multifd
>> > channel creation?
>>
>> For instance, postcopy_do_resume() has this comment:
>>     /*
>>      * If preempt is enabled, re-establish the preempt channel.  Note that
>>      * we do it after resume prepare to make sure the main channel will be
>>      * created before the preempt channel.  E.g. with weak network, the
>>      * dest QEMU may get messed up with the preempt and main channels on
>>      * the order of connection setup.  This guarantees the correct order.
>>      */
>> It looks like if the main channel can race, so do the multifd channels,
>> no? In any case, I'm fine with just documenting any assumption for now.
>
> * The first requirement for this race to occur is that two types of
> channels are created together at the same time. Let's see:
>
>    * Postcopy migration:  without multifd enabled
>       - 'main' channel is created before the migration starts. And
> 'postcopy' channels are created towards the end of precopy migration,
> when the Postcopy phase starts. So in this scenario the race does not
> happen.
>
>    * Postcopy resume: without multifd enabled
>       - As described in the comment above, preempt channel is created
> _after_ the 'main' channel to avoid the race condition.
>
>    * Postcopy migration: with multifd enabled
>       - 'main' and 'multifd' channels are created before migration
> starts. And 'postcopy' channels are created towards the end of precopy
> migration, when the Postcopy phase starts. No race occurs.
>
>    * Postcopy resume: with multifd enabled
>       - 'multifd' channels are shutdown before Postcopy starts, ie. no
> 'multifd' channels exist during Postcopy resume. So no race between
> 'postcopy' and 'multifd' channels.
>       - And 'postcopy' channels are created after the 'main' channel
> to avoid the race between them.
>       - postcopy_do_resume() does not seem to create 'multifd' channels.
>
>    * Multifd migration: without Postcopy enabled
>       - 'main' and 'multifd' channels are created before the migration
> starts. They both send 'magic value' bytes, so are easier to
> differentiate. No race occurs.
>

I don't see anything stopping postcopy_start() from being called in the
source in relation to multifd recv threads being setup in the
destination. So far it seems possible that the source is opening the
preempt channel while multifd still hasn't seen all threads. There's
also pre-7.2 machines which create the postcopy channel early.

>
>> > * migration_needs_multiple_sockets() => return migrate_multifd() ||
>> > migrate_postcopy_preempt();
>> >
>> Nope, this is just saying whether a single channel is expected, or more
>> than one.
>
> * If we read it as a question:
>     - migration_needs_multiple_sockets() ? True => Yes, migration
> needs multiple sockets.
>     - migration_needs_multiple_sockets() ? False => No, migration does
> not need multiple sockets.
>
> Then it should return 'True' when both migrate_multifd() and
> postcopy_preempt() are enabled.
>

Why?

>>That's why I think it would be a good gate for this peeking
>> code. Since postcopy preempt could be a peekable channel, it's
>> misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK
>> only. This is a time-bomb for the next person to refactor this code.
>
> * Postcopy preempt could be a peekable channel ? Currently it does not
> send magic value, does it?
>

Peekable means it can read ahead and rollback without consuming that
part of the stream. We need it because there's code later on that will
validate the MAGIC.

>> Right, but that's not what we have today. Changing this requires
>> figuring out how to keep the stream compatible when channels now start
>> sending extra stuff at the start. It's not trivial. There's also
>> mapped-ram which is asynchronous and there might be something special to
>> be done about the TLS handshake, I'm not sure.
>
> * True, it's not trivial.
>
>> Well, aside from preempt, they're *not* dependent on the order. That's
>> the point of having to do all of this dance. In fact we might be better
>> off if we could serialize the connections somehow.
>>
>> I havent't followed this series closely, could you point me to the
>> discussion that led to the channels concept being introduced?
>
> * Channels concept was not introduced in this series. It has been
> there since the beginning, no?
>

I thought you meant the CH_MAIN stuff. So now I don't know what you
mean. You want to do away with multifd?

>> Yes. They *can* be used without multifd. The comment would explain that
>> at that point in the code, these are the only types possible. So as to
>> not mislead future readers that whenever tls/file, then multifd must be
>> used.
> ....
>> See? Multifd mutually exclusive with postcopy preempt. You carried that
>> assumption (well done), but made it more subtle (not good), since
>> if/else is by definition showing the relationship between the two while
>> migration_has_main_and_multifd_channels() makes it hidden under the
>> multifd check allowing the last return true to happen.
>>
>> If we're enabling multifd along with postcopy, we need to be aware that
>> the relationship with preempt might not hold true anymore.
>
> * Sorry, I did not get that. Enabling them together means that they
> are _not_ exclusive, no? It is not Either 'multifd'  OR 'postcopy'
> case, anymore.
>

Yes, as we're seeing, there's this assumption that multifd is never
enabled along with postcopy_preempt. Now with multifd+postcopy we need
to be careful to stop the places where that assumption was made.

>>>Not sure if/how that works really. It is possible that currently
>>> these (tls/file) channels are used only with migrate_multifd() enabled
>>> and so are processed with multifd_recv_new_channel() function. The
>>> current patch handles them the same way.
>>
>> That's the entire point I'm making when I ask to not omit the else
>> clauses.
>
> * ie. we set 'channel = CH_MAIN' in the final else clause as well? - Okay.
>
>> Do you think this series can work without touching the channel discovery
>> code? As I said earlier, I'm missing a bit of context, but to me it
>> seems it cannot.
>
> * The reason we need to touch the channel discovery part is: with
> 'multifd' and 'postcopy' both enabled, towards the end of migration,
> when 'postcopy' connection comes in
>      migration_ioc_process_incoming(...)
>     {
>         if (migrate_multifd() && !migrate_mapped_ram() &&
> !migrate_postcopy_ram() &&
>              qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
>              ...
>          } else {
>              default_channel = !mis->from_src_file;
>          }
>         ...
>         if (migrate_multifd()) {
>             multifd_recv_new_channel(ioc, &local_err);
>         } else {
>            postcopy_preempt_new_channel(mis, f);
>         }
>     }
>
> * The first 'if (migrate_multifd() ... !migrate_postcopy_ram()')
> evaluates to false, in the else part 'default_channel' also evaluates
> to false, because the 'main' channel is established. Now the new
> incoming connection falls in the second migrate_multifd() block and
> gets processed via - multifd_recv_new_channel(ioc, &local_err); call
> and migration would not complete/finish.
>
> * To identify the incoming postcopy connection, in the very first
> version of this series, a magic value for the postcopy channel was
> introduced and everything else remained the same.
>
> * Would that be an acceptable solution for now?
>

Continue with this patch and fix the stuff I mentioned. You can ignore
the first two paragraphs of that reply.

https://lore.kernel.org/r/87y0y4tf5q.fsf@suse.de

I still think we need to test that preempt + multifd scenario, but it
should be easy to write a test for that once the series is in more of a
final shape.

>> If instead of this refactoring you want to start working on a model for
>> consistent channel advertisement, then that's fine. But we'll have to
>> put this series on hold (which is also fine). It also looks like it
>> could be considerably more work, although I haven't looked at it in
>> detail. Granted, it's work that makes sense, instead of the heuristics
>> we have today.
>
> * IMHO, we need not put this series on hold, for now we could go ahead
> with the postcopy magic value patch if that works. And the larger
> overhaul of the channel discovery part could be done as a separate
> series of its own.
>

We can't add magic values, as we've discussed.

> Thank you.
> ---
>   - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Prasad Pandit]

Hello,

On Wed, 19 Feb 2025 at 22:53, Fabiano Rosas <farosas@suse.de> wrote:
> I don't see anything stopping postcopy_start() from being called in the
> source in relation to multifd recv threads being setup in the
> destination. So far it seems possible that the source is opening the
> preempt channel while multifd still hasn't seen all threads. There's
> also pre-7.2 machines which create the postcopy channel early.

* If we can not predict the sequence/timings of when different types
of connections are initiated and processed, maybe source and
destination QEMUs could work in tandem. ie. before initiating a
connection, source QEMU could send an 'initiate' message saying I'm
initiating 'X' connection. Only when destination QEMU says 'okay',
source QEMU could proceed with actual connection.

     QEMU-A  -> Initiate connection type X -> QEMU-B
     QEMU-A  <-            okay           <-        <- QEMU-B
     QEMU-A ->       connect type X            -> QEMU-B

   (thinking out loud)

>>> > * migration_needs_multiple_sockets()
>> Then it should return 'True' when both migrate_multifd() and postcopy_preempt() are enabled.
> Why?

* I was thinking multiple_sockets is multiple types of sockets:
multifd & postcopy. But it seems here multiple sockets is any type of
multiple sockets.

> I thought you meant the CH_MAIN stuff. So now I don't know what you
> mean. You want to do away with multifd?

* Yes, CH_DEFAULT -> CH_MAIN was introduced in this series to identify
channels and accordingly call relevant functions.

* Not to do away with multifd, but more of making it same as the main
channel, ex: virsh migrate --threads <N> N = 1...255. All precopy
threads/connections behave the same. Differentiation of precopy and
postcopy shall still exist, because they operate/work in opposite
directions.

> Continue with this patch and fix the stuff I mentioned. You can ignore
> the first two paragraphs of that reply.
>
> https://lore.kernel.org/r/87y0y4tf5q.fsf@suse.de
>
> I still think we need to test that preempt + multifd scenario, but it
> should be easy to write a test for that once the series is in more of a
> final shape.

* Okay.

> We can't add magic values, as we've discussed.

Okay.

Thank you.
---
  - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> Hello,
>
> On Wed, 19 Feb 2025 at 22:53, Fabiano Rosas <farosas@suse.de> wrote:
>> I don't see anything stopping postcopy_start() from being called in the
>> source in relation to multifd recv threads being setup in the
>> destination. So far it seems possible that the source is opening the
>> preempt channel while multifd still hasn't seen all threads. There's
>> also pre-7.2 machines which create the postcopy channel early.
>
> * If we can not predict the sequence/timings of when different types
> of connections are initiated and processed, maybe source and
> destination QEMUs could work in tandem. ie. before initiating a
> connection, source QEMU could send an 'initiate' message saying I'm
> initiating 'X' connection. Only when destination QEMU says 'okay',
> source QEMU could proceed with actual connection.
>
>      QEMU-A  -> Initiate connection type X -> QEMU-B
>      QEMU-A  <-            okay           <-        <- QEMU-B
>      QEMU-A ->       connect type X            -> QEMU-B
>
>    (thinking out loud)
>

This is more or less the handshake idea. Or at least it could be
included in that work.

I have parked the handshake idea for now because I'm not seeing an
immediate need for it and there are more pressing issues to be dealt
with first such as bugs and coordinating the new features (and their
possible outcomings) that IMO need to be looked at first.

>>>> > * migration_needs_multiple_sockets()
>>> Then it should return 'True' when both migrate_multifd() and postcopy_preempt() are enabled.
>> Why?
>
> * I was thinking multiple_sockets is multiple types of sockets:
> multifd & postcopy. But it seems here multiple sockets is any type of
> multiple sockets.
>

Yes this means main channel + others.

>> I thought you meant the CH_MAIN stuff. So now I don't know what you
>> mean. You want to do away with multifd?
>
> * Yes, CH_DEFAULT -> CH_MAIN was introduced in this series to identify
> channels and accordingly call relevant functions.
>
> * Not to do away with multifd, but more of making it same as the main
> channel, ex: virsh migrate --threads <N> N = 1...255. All precopy
> threads/connections behave the same. Differentiation of precopy and
> postcopy shall still exist, because they operate/work in opposite
> directions.
>

I'm not opposed to that idea. When I started working with migration I
had the impression that was the direction and that we could put every
workload in a pool of multifd threads. Now, knowing the code better, I'm
not sure that's feasible. Specially the dependence on a "main" channel
seems difficult to do away with. It's also somewhat convenient to have a
maint thread. But we could still attempt to group extra threads, such as
what we're doing with the new thread pool in the device state series. At
least thread management could be done entirely in a separate pool, main
channel and all.

>> Continue with this patch and fix the stuff I mentioned. You can ignore
>> the first two paragraphs of that reply.
>>
>> https://lore.kernel.org/r/87y0y4tf5q.fsf@suse.de
>>
>> I still think we need to test that preempt + multifd scenario, but it
>> should be easy to write a test for that once the series is in more of a
>> final shape.
>
> * Okay.
>
>> We can't add magic values, as we've discussed.
>
> Okay.
>
> Thank you.
> ---
>   - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Prasad Pandit]

Hello Fabiano,

On Thu, 20 Feb 2025 at 19:06, Fabiano Rosas <farosas@suse.de> wrote:
> This is more or less the handshake idea. Or at least it could be
> included in that work.
>
> I have parked the handshake idea for now because I'm not seeing an
> immediate need for it and there are more pressing issues to be dealt
> with first such as bugs and coordinating the new features (and their
> possible outcomings) that IMO need to be looked at first.

* I see, okay.

> I'm not opposed to that idea. When I started working with migration I
> had the impression that was the direction and that we could put every
> workload in a pool of multifd threads. Now, knowing the code better, I'm
> not sure that's feasible. Specially the dependence on a "main" channel
> seems difficult to do away with. It's also somewhat convenient to have a
> maint thread. But we could still attempt to group extra threads, such as
> what we're doing with the new thread pool in the device state series. At
> least thread management could be done entirely in a separate pool, main
> channel and all.
>

* True. To extend the two QEMUs working in tandem OR the handshake
idea further with the 'main' channel, let's say a user invokes
command:

$ virsh migrate --threads 4 --postcopy --postcopy-after-precopy ...

0) Channel = TCP socket connection between two machines.

1) The 'main' channel is the dedicated _control_ channel; And other
channels are dedicated _data_ channels. So with '--threads 4' option,
QEMU creates a total of 5 (main + 4) channels.

        QEMU-A  -> 'main' channel    -> QEMU-B
        QEMU-A ->  'data' channel-1 -> QEMU-B
        QEMU-A ->  'data' channel-2 -> QEMU-B
        QEMU-A ->  'data' channel-3 -> QEMU-B
        QEMU-A ->  'data' channel-4 -> QEMU-B

    * Each channel is used by a thread of its own.

2) All channels are created _before_ the migration starts and stay
till the end of the migration. No asynchronous channels popping up
during migration, like a 'postcopy' channel now.

3) In the beginning source says 'Let's Precopy' to the destination on
the 'main' channel

         QEMU-A  -> main: Let's precopy  -> QEMU-B
         QEMU-A  <- main: Okay              <- QEMU-B

    And migration data flows from QEMU-A  -> to -> QEMU-B  on the
'data' channels.

        QEMU-A ->  'data' -> -> -> QEMU-B
        QEMU-A ->  'data' -> -> -> QEMU-B
        QEMU-A ->  'data' -> -> -> QEMU-B
        QEMU-A ->  'data' -> -> -> QEMU-B

4) When it's time to switch to Postcopy,  source says 'Let's Postcopy'
to the destination on the 'main' channel

        QEMU-A  -> main: Let's postcopy  -> QEMU-B
        QEMU-A  <- main: Okay                <- QEMU-B

    And migration page requests/data use the same 'data' channels.

        QEMU-A <- <- 'request/data'  -> -> QEMU-B
        QEMU-A <- <- 'request/data'  -> -> QEMU-B
        QEMU-A <- <- 'request/data'  -> -> QEMU-B
        QEMU-A <- <- 'request/data'  -> -> QEMU-B

5) This way:
     - 'main' channel could be used to co-ordinate actions of two QEMUs.
     - All data channels may be used during Postcopy too, instead of
one channel now.
     - There may not be race conditions while creating channels.
     - No differentiation of precopy/multifd/postcopy/preempt etc. channels.

(thinking out loud if that sounds workable)

Thank you.
---
  - Prasad

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Juraj Marcin]

Hi Prasad,

On 2025-02-15 18:01, Prasad Pandit wrote:
> From: Prasad Pandit <pjp@fedoraproject.org>
> 
> Enable Multifd and Postcopy migration together.
> The migration_ioc_process_incoming() routine
> checks magic value sent on each channel and
> helps to properly setup multifd and postcopy
> channels.
> 
> The Precopy and Multifd threads work during the
> initial guest RAM transfer. When migration moves
> to the Postcopy phase, the multifd threads are
> shutdown and Postcopy threads on the destination
> request/pull data from the source side.
> 
> Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
> ---
>  migration/migration.c      | 107 ++++++++++++++++++++-----------------
>  migration/multifd-nocomp.c |   3 +-
>  migration/multifd.c        |   4 +-
>  migration/options.c        |   5 --
>  migration/ram.c            |   7 ++-
>  5 files changed, 64 insertions(+), 62 deletions(-)
> 
> v6:
> - Shutdown multifd threads before postcopy_start()
> - Reorder tests/qtest/migration/ patches
> - Some refactoring of functions
> 
> v5:
> - https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 396928513a..38697182e8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -95,6 +95,9 @@ enum mig_rp_message_type {
>      MIG_RP_MSG_MAX
>  };
>  
> +/* Migration channel types */
> +enum { CH_MAIN, CH_MULTIFD, CH_POSTCOPY };
> +
>  /* When we add fault tolerance, we could have several
>     migrations at once.  For now we don't need to add
>     dynamic creation of migration */
> @@ -959,28 +962,19 @@ void migration_fd_process_incoming(QEMUFile *f)
>      migration_incoming_process();
>  }
>  
> -/*
> - * Returns true when we want to start a new incoming migration process,
> - * false otherwise.
> - */
> -static bool migration_should_start_incoming(bool main_channel)
> +static bool migration_has_main_and_multifd_channels(void)
>  {
> -    /* Multifd doesn't start unless all channels are established */
> -    if (migrate_multifd()) {
> -        return migration_has_all_channels();
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    if (!mis->from_src_file) {
> +        /* main channel not established */
> +        return false;
>      }
>  
> -    /* Preempt channel only starts when the main channel is created */
> -    if (migrate_postcopy_preempt()) {
> -        return main_channel;
> +    if (migrate_multifd() && !multifd_recv_all_channels_created()) {
> +        return false;
>      }
>  
> -    /*
> -     * For all the rest types of migration, we should only reach here when
> -     * it's the main channel that's being created, and we should always
> -     * proceed with this channel.
> -     */
> -    assert(main_channel);
> +    /* main channel and all multifd channels are established */
>      return true;
>  }
>  
> @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      Error *local_err = NULL;
>      QEMUFile *f;
> -    bool default_channel = true;
>      uint32_t channel_magic = 0;
> +    uint8_t channel = CH_MAIN;
>      int ret = 0;
>  
> -    if (migrate_multifd() && !migrate_mapped_ram() &&
> -        !migrate_postcopy_ram() &&
> -        qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> +    if (!migration_has_main_and_multifd_channels()) {
> +        if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
>          /*
>           * With multiple channels, it is possible that we receive channels
>           * out of order on destination side, causing incorrect mapping of
> @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>           * tls handshake while initializing main channel so with tls this
>           * issue is not possible.
>           */
> -        ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> -                                          sizeof(channel_magic), errp);
> +            ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> +                                              sizeof(channel_magic), errp);
> +            if (ret != 0) {
> +                return;
> +            }
>  
> -        if (ret != 0) {
> -            return;
> +            if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) {
> +                channel = CH_MAIN;
> +            } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) {
> +                channel = CH_MULTIFD;
> +            } else if (!mis->from_src_file
> +                        && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +                /* reconnect default channel for postcopy recovery */
> +                channel = CH_MAIN;
> +            } else {
> +                error_report("%s: unknown channel magic: %u",
> +                                __func__, channel_magic);

Here, the number reported in the error will have incorrect endianness on
a non-BE system. I think it would be better to convert channel_magic to
the system endianness right after reading it. On top of that, then there
is no need to convert constants with magic numbers when comparing.

> +                return;
> +            }
> +        } else if (mis->from_src_file
> +                && (!strcmp(ioc->name, "migration-tls-incoming")
> +                || !strcmp(ioc->name, "migration-file-incoming"))) {
> +            channel = CH_MULTIFD;
>          }
> -
> -        default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC));
> -    } else {
> -        default_channel = !mis->from_src_file;
> +    } else if (mis->from_src_file) {
> +        channel = CH_POSTCOPY;
>      }
>  
>      if (multifd_recv_setup(errp) != 0) {
>          return;
>      }
>  
> -    if (default_channel) {
> +    if (channel == CH_MAIN) {
>          f = qemu_file_new_input(ioc);
>          migration_incoming_setup(f);
> -    } else {
> +    } else if (channel == CH_MULTIFD) {
>          /* Multiple connections */
> -        assert(migration_needs_multiple_sockets());
>          if (migrate_multifd()) {
>              multifd_recv_new_channel(ioc, &local_err);
> -        } else {
> -            assert(migrate_postcopy_preempt());
> -            f = qemu_file_new_input(ioc);
> -            postcopy_preempt_new_channel(mis, f);
>          }
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
>          }
> +    } else if (channel == CH_POSTCOPY) {
> +        assert(migrate_postcopy_preempt());
> +        assert(!mis->postcopy_qemufile_dst);
> +        f = qemu_file_new_input(ioc);
> +        postcopy_preempt_new_channel(mis, f);
>      }
>  
> -    if (migration_should_start_incoming(default_channel)) {
> +    if (channel != CH_POSTCOPY && migration_has_main_and_multifd_channels()) {
>          /* If it's a recovery, we're done */
>          if (postcopy_try_recover()) {
>              return;
> @@ -1058,20 +1067,15 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>   */
>  bool migration_has_all_channels(void)
>  {
> +    if (!migration_has_main_and_multifd_channels()) {
> +        return false;
> +    }
> +
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -
> -    if (!mis->from_src_file) {
> +    if (migrate_postcopy_preempt() && !mis->postcopy_qemufile_dst) {
>          return false;
>      }
>  
> -    if (migrate_multifd()) {
> -        return multifd_recv_all_channels_created();
> -    }
> -
> -    if (migrate_postcopy_preempt()) {
> -        return mis->postcopy_qemufile_dst != NULL;
> -    }
> -
>      return true;
>  }
>  
> @@ -1482,6 +1486,8 @@ static void migrate_fd_cleanup(MigrationState *s)
>  
>      assert(!migration_is_active());
>  
> +    file_cleanup_outgoing_migration();
> +    socket_cleanup_outgoing_migration();
>      if (s->state == MIGRATION_STATUS_CANCELLING) {
>          migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
>                            MIGRATION_STATUS_CANCELLED);
> @@ -3373,8 +3379,11 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>      }
>  
>      /* Still a significant amount to transfer */
> -    if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover &&
> -        qatomic_read(&s->start_postcopy)) {
> +    if (!in_postcopy && must_precopy <= s->threshold_size
> +        && can_switchover && qatomic_read(&s->start_postcopy)) {
> +        if (migrate_multifd()) {
> +            multifd_send_shutdown();
> +        }
>          if (postcopy_start(s, &local_err)) {
>              migrate_set_error(s, local_err);
>              error_report_err(local_err);
> diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
> index 1325dba97c..d0edec7cd1 100644
> --- a/migration/multifd-nocomp.c
> +++ b/migration/multifd-nocomp.c
> @@ -16,6 +16,7 @@
>  #include "file.h"
>  #include "multifd.h"
>  #include "options.h"
> +#include "migration.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/error-report.h"
> @@ -391,7 +392,7 @@ int multifd_ram_flush_and_sync(QEMUFile *f)
>      MultiFDSyncReq req;
>      int ret;
>  
> -    if (!migrate_multifd()) {
> +    if (!migrate_multifd() || migration_in_postcopy()) {
>          return 0;
>      }
>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 97ce831775..fa83a43778 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -467,8 +467,6 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>  
>  static void multifd_send_cleanup_state(void)
>  {
> -    file_cleanup_outgoing_migration();
> -    socket_cleanup_outgoing_migration();
>      qemu_sem_destroy(&multifd_send_state->channels_created);
>      qemu_sem_destroy(&multifd_send_state->channels_ready);
>      g_free(multifd_send_state->params);
> @@ -481,7 +479,7 @@ void multifd_send_shutdown(void)
>  {
>      int i;
>  
> -    if (!migrate_multifd()) {
> +    if (!migrate_multifd() || !multifd_send_state) {
>          return;
>      }
>  
> diff --git a/migration/options.c b/migration/options.c
> index 4db340b502..c4dfe89edd 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -480,11 +480,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>              error_setg(errp, "Postcopy is not compatible with ignore-shared");
>              return false;
>          }
> -
> -        if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
> -            error_setg(errp, "Postcopy is not yet compatible with multifd");
> -            return false;
> -        }
>      }
>  
>      if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 6f460fd22d..8f22745aba 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1297,7 +1297,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>          pss->page = 0;
>          pss->block = QLIST_NEXT_RCU(pss->block, next);
>          if (!pss->block) {
> -            if (multifd_ram_sync_per_round()) {
> +            if (multifd_ram_sync_per_round() && !migration_in_postcopy()) {
>                  QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
>                  int ret = multifd_ram_flush_and_sync(f);
>                  if (ret < 0) {
> @@ -1971,9 +1971,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
>          }
>      }
>  
> -    if (migrate_multifd()) {
> -        RAMBlock *block = pss->block;
> -        return ram_save_multifd_page(block, offset);
> +    if (migrate_multifd() && !migration_in_postcopy()) {
> +        return ram_save_multifd_page(pss->block, offset);
>      }
>  
>      if (control_save_page(pss, offset, &res)) {
> --
> 2.48.1
> 
> 

Re: [PATCH v6 2/4] migration: enable multifd and postcopy together

[Prasad Pandit]

On Tue, 18 Feb 2025 at 16:47, Juraj Marcin <jmarcin@redhat.com> wrote:
> > +                error_report("%s: unknown channel magic: %u",
> > +                                __func__, channel_magic);
>
> Here, the number reported in the error will have incorrect endianness on
> a non-BE system. I think it would be better to convert channel_magic to
> the system endianness right after reading it. On top of that, then there
> is no need to convert constants with magic numbers when comparing.

* Okay.

Thank you.
---
  - Prasad

[PATCH v6 3/4] tests/qtest/migration: consolidate set capabilities

[Prasad Pandit]

From: Prasad Pandit <pjp@fedoraproject.org>

Migration capabilities are set in multiple '.start_hook'
functions for various tests. Instead, consolidate setting
capabilities in 'set_migration_capabilities()' function
which is called from various 'test_*_common()' functions.
While simplifying the capabilities setting, it helps
to declutter the qtest sources.

Suggested-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
---
 tests/qtest/migration/compression-tests.c | 10 ++--
 tests/qtest/migration/cpr-tests.c         |  4 +-
 tests/qtest/migration/file-tests.c        | 44 +++++-------------
 tests/qtest/migration/framework.c         | 56 +++++++++++++++++------
 tests/qtest/migration/framework.h         |  4 +-
 tests/qtest/migration/postcopy-tests.c    |  4 +-
 tests/qtest/migration/precopy-tests.c     | 19 +++-----
 tests/qtest/migration/tls-tests.c         | 11 ++++-
 8 files changed, 80 insertions(+), 72 deletions(-)

v6:
- Reorder, make this the first qtest patch in this series.

v5:
- https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t

diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
index 8b58401b84..4558a7b9ff 100644
--- a/tests/qtest/migration/compression-tests.c
+++ b/tests/qtest/migration/compression-tests.c
@@ -35,6 +35,7 @@ static void test_multifd_tcp_zstd(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
     };
     test_precopy_common(&args);
@@ -56,6 +57,7 @@ static void test_multifd_tcp_qatzip(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         .start_hook = migrate_hook_start_precopy_tcp_multifd_qatzip,
     };
     test_precopy_common(&args);
@@ -74,6 +76,7 @@ static void test_multifd_tcp_qpl(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         .start_hook = migrate_hook_start_precopy_tcp_multifd_qpl,
     };
     test_precopy_common(&args);
@@ -92,6 +95,7 @@ static void test_multifd_tcp_uadk(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         .start_hook = migrate_hook_start_precopy_tcp_multifd_uadk,
     };
     test_precopy_common(&args);
@@ -103,10 +107,6 @@ migrate_hook_start_xbzrle(QTestState *from,
                           QTestState *to)
 {
     migrate_set_parameter_int(from, "xbzrle-cache-size", 33554432);
-
-    migrate_set_capability(from, "xbzrle", true);
-    migrate_set_capability(to, "xbzrle", true);
-
     return NULL;
 }
 
@@ -118,6 +118,7 @@ static void test_precopy_unix_xbzrle(void)
         .listen_uri = uri,
         .start_hook = migrate_hook_start_xbzrle,
         .iterations = 2,
+        .caps[MIGRATION_CAPABILITY_XBZRLE] = true,
         /*
          * XBZRLE needs pages to be modified when doing the 2nd+ round
          * iteration to have real data pushed to the stream.
@@ -146,6 +147,7 @@ static void test_multifd_tcp_zlib(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         .start_hook = migrate_hook_start_precopy_tcp_multifd_zlib,
     };
     test_precopy_common(&args);
diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
index 4758841824..a175646b57 100644
--- a/tests/qtest/migration/cpr-tests.c
+++ b/tests/qtest/migration/cpr-tests.c
@@ -24,9 +24,6 @@ static void *migrate_hook_start_mode_reboot(QTestState *from, QTestState *to)
     migrate_set_parameter_str(from, "mode", "cpr-reboot");
     migrate_set_parameter_str(to, "mode", "cpr-reboot");
 
-    migrate_set_capability(from, "x-ignore-shared", true);
-    migrate_set_capability(to, "x-ignore-shared", true);
-
     return NULL;
 }
 
@@ -39,6 +36,7 @@ static void test_mode_reboot(void)
         .connect_uri = uri,
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_mode_reboot,
+        .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true,
     };
 
     test_file_common(&args, true);
diff --git a/tests/qtest/migration/file-tests.c b/tests/qtest/migration/file-tests.c
index f260e2871d..6253ece687 100644
--- a/tests/qtest/migration/file-tests.c
+++ b/tests/qtest/migration/file-tests.c
@@ -107,15 +107,6 @@ static void test_precopy_file_offset_bad(void)
     test_file_common(&args, false);
 }
 
-static void *migrate_hook_start_mapped_ram(QTestState *from,
-                                           QTestState *to)
-{
-    migrate_set_capability(from, "mapped-ram", true);
-    migrate_set_capability(to, "mapped-ram", true);
-
-    return NULL;
-}
-
 static void test_precopy_file_mapped_ram_live(void)
 {
     g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
@@ -123,7 +114,7 @@ static void test_precopy_file_mapped_ram_live(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .start_hook = migrate_hook_start_mapped_ram,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
     };
 
     test_file_common(&args, false);
@@ -136,26 +127,12 @@ static void test_precopy_file_mapped_ram(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .start_hook = migrate_hook_start_mapped_ram,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
     };
 
     test_file_common(&args, true);
 }
 
-static void *migrate_hook_start_multifd_mapped_ram(QTestState *from,
-                                                   QTestState *to)
-{
-    migrate_hook_start_mapped_ram(from, to);
-
-    migrate_set_parameter_int(from, "multifd-channels", 4);
-    migrate_set_parameter_int(to, "multifd-channels", 4);
-
-    migrate_set_capability(from, "multifd", true);
-    migrate_set_capability(to, "multifd", true);
-
-    return NULL;
-}
-
 static void test_multifd_file_mapped_ram_live(void)
 {
     g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
@@ -163,7 +140,8 @@ static void test_multifd_file_mapped_ram_live(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .start_hook = migrate_hook_start_multifd_mapped_ram,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
     };
 
     test_file_common(&args, false);
@@ -176,7 +154,8 @@ static void test_multifd_file_mapped_ram(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .start_hook = migrate_hook_start_multifd_mapped_ram,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
     };
 
     test_file_common(&args, true);
@@ -185,8 +164,6 @@ static void test_multifd_file_mapped_ram(void)
 static void *migrate_hook_start_multifd_mapped_ram_dio(QTestState *from,
                                                        QTestState *to)
 {
-    migrate_hook_start_multifd_mapped_ram(from, to);
-
     migrate_set_parameter_bool(from, "direct-io", true);
     migrate_set_parameter_bool(to, "direct-io", true);
 
@@ -201,6 +178,8 @@ static void test_multifd_file_mapped_ram_dio(void)
         .connect_uri = uri,
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_mapped_ram_dio,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
 
     if (!probe_o_direct_support(tmpfs)) {
@@ -246,7 +225,6 @@ static void *migrate_hook_start_multifd_mapped_ram_fdset_dio(QTestState *from,
     fdset_add_fds(from, file, O_WRONLY, 2, true);
     fdset_add_fds(to, file, O_RDONLY, 2, true);
 
-    migrate_hook_start_multifd_mapped_ram(from, to);
     migrate_set_parameter_bool(from, "direct-io", true);
     migrate_set_parameter_bool(to, "direct-io", true);
 
@@ -261,8 +239,6 @@ static void *migrate_hook_start_multifd_mapped_ram_fdset(QTestState *from,
     fdset_add_fds(from, file, O_WRONLY, 2, false);
     fdset_add_fds(to, file, O_RDONLY, 2, false);
 
-    migrate_hook_start_multifd_mapped_ram(from, to);
-
     return NULL;
 }
 
@@ -275,6 +251,8 @@ static void test_multifd_file_mapped_ram_fdset(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_mapped_ram_fdset,
         .end_hook = migrate_hook_end_multifd_mapped_ram_fdset,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
 
     test_file_common(&args, true);
@@ -289,6 +267,8 @@ static void test_multifd_file_mapped_ram_fdset_dio(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_mapped_ram_fdset_dio,
         .end_hook = migrate_hook_end_multifd_mapped_ram_fdset,
+        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
 
     if (!probe_o_direct_support(tmpfs)) {
diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
index 10e1d04b58..82aaa13e85 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -207,6 +207,31 @@ static QList *migrate_start_get_qmp_capabilities(const MigrateStart *args)
     return capabilities;
 }
 
+static void set_migration_capabilities(QTestState *from,
+                                        QTestState *to, MigrateCommon *args)
+{
+    /*
+     * MigrationCapability_lookup and MIGRATION_CAPABILITY_ constants
+     * are from qapi-types-migration.h.
+     */
+    for (uint8_t i = 0; i < MIGRATION_CAPABILITY__MAX; i++)
+    {
+        if (!args->caps[i]) {
+            continue;
+        }
+        if (from) {
+            migrate_set_capability(from,
+                            MigrationCapability_lookup.array[i], true);
+        }
+        if (to) {
+            migrate_set_capability(to,
+                            MigrationCapability_lookup.array[i], true);
+        }
+    }
+
+    return;
+}
+
 int migrate_start(QTestState **from, QTestState **to, const char *uri,
                   MigrateStart *args)
 {
@@ -440,17 +465,12 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
         args->postcopy_data = args->start_hook(from, to);
     }
 
-    migrate_set_capability(from, "postcopy-ram", true);
-    migrate_set_capability(to, "postcopy-ram", true);
-    migrate_set_capability(to, "postcopy-blocktime", true);
-
-    if (args->postcopy_preempt) {
-        migrate_set_capability(from, "postcopy-preempt", true);
-        migrate_set_capability(to, "postcopy-preempt", true);
-    }
+    /* set postcopy capabilities */
+    args->caps[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME] = true;
+    args->caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true;
+    set_migration_capabilities(from, to, args);
 
     migrate_ensure_non_converge(from);
-
     migrate_prepare_for_dirty_mem(from);
     qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
                              "  'arguments': { "
@@ -717,6 +737,12 @@ void test_precopy_common(MigrateCommon *args)
         return;
     }
 
+    set_migration_capabilities(from, to, args);
+    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
+        migrate_set_parameter_int(from, "multifd-channels", 16);
+        migrate_set_parameter_int(to, "multifd-channels", 16);
+    }
+
     if (args->start_hook) {
         data_hook = args->start_hook(from, to);
     }
@@ -888,6 +914,12 @@ void test_file_common(MigrateCommon *args, bool stop_src)
      */
     g_assert_false(args->live);
 
+    set_migration_capabilities(from, to, args);
+    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
+        migrate_set_parameter_int(from, "multifd-channels", 4);
+        migrate_set_parameter_int(to, "multifd-channels", 4);
+    }
+
     if (g_strrstr(args->connect_uri, "offset=")) {
         check_offset = true;
         /*
@@ -948,15 +980,9 @@ void *migrate_hook_start_precopy_tcp_multifd_common(QTestState *from,
                                                     QTestState *to,
                                                     const char *method)
 {
-    migrate_set_parameter_int(from, "multifd-channels", 16);
-    migrate_set_parameter_int(to, "multifd-channels", 16);
-
     migrate_set_parameter_str(from, "multifd-compression", method);
     migrate_set_parameter_str(to, "multifd-compression", method);
 
-    migrate_set_capability(from, "multifd", true);
-    migrate_set_capability(to, "multifd", true);
-
     /* Start incoming migration from the 1st socket */
     migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
 
diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
index e4a11870f6..74d53eee69 100644
--- a/tests/qtest/migration/framework.h
+++ b/tests/qtest/migration/framework.h
@@ -12,6 +12,7 @@
 #define TEST_FRAMEWORK_H
 
 #include "libqtest.h"
+#include <qapi/qapi-types-migration.h>
 
 #define FILE_TEST_FILENAME "migfile"
 #define FILE_TEST_OFFSET 0x1000
@@ -207,8 +208,9 @@ typedef struct {
 
     /* Postcopy specific fields */
     void *postcopy_data;
-    bool postcopy_preempt;
     PostcopyRecoveryFailStage postcopy_recovery_fail_stage;
+
+    bool caps[MIGRATION_CAPABILITY__MAX];
 } MigrateCommon;
 
 void wait_for_serial(const char *side);
diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
index 982457bed1..b0e70a6367 100644
--- a/tests/qtest/migration/postcopy-tests.c
+++ b/tests/qtest/migration/postcopy-tests.c
@@ -39,7 +39,7 @@ static void test_postcopy_suspend(void)
 static void test_postcopy_preempt(void)
 {
     MigrateCommon args = {
-        .postcopy_preempt = true,
+        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
     };
 
     test_postcopy_common(&args);
@@ -73,7 +73,7 @@ static void test_postcopy_recovery_fail_reconnect(void)
 static void test_postcopy_preempt_recovery(void)
 {
     MigrateCommon args = {
-        .postcopy_preempt = true,
+        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
     };
 
     test_postcopy_recovery_common(&args);
diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index 162fa69531..e5d8c49dbe 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -107,23 +107,12 @@ static void test_precopy_tcp_plain(void)
     test_precopy_common(&args);
 }
 
-static void *migrate_hook_start_switchover_ack(QTestState *from, QTestState *to)
-{
-
-    migrate_set_capability(from, "return-path", true);
-    migrate_set_capability(to, "return-path", true);
-
-    migrate_set_capability(from, "switchover-ack", true);
-    migrate_set_capability(to, "switchover-ack", true);
-
-    return NULL;
-}
-
 static void test_precopy_tcp_switchover_ack(void)
 {
     MigrateCommon args = {
         .listen_uri = "tcp:127.0.0.1:0",
-        .start_hook = migrate_hook_start_switchover_ack,
+        .caps[MIGRATION_CAPABILITY_RETURN_PATH] = true,
+        .caps[MIGRATION_CAPABILITY_SWITCHOVER_ACK] = true,
         /*
          * Source VM must be running in order to consider the switchover ACK
          * when deciding to do switchover or not.
@@ -392,6 +381,7 @@ static void test_multifd_tcp_uri_none(void)
     MigrateCommon args = {
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         /*
          * Multifd is more complicated than most of the features, it
          * directly takes guest page buffers when sending, make sure
@@ -407,6 +397,7 @@ static void test_multifd_tcp_zero_page_legacy(void)
     MigrateCommon args = {
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd_zero_page_legacy,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         /*
          * Multifd is more complicated than most of the features, it
          * directly takes guest page buffers when sending, make sure
@@ -422,6 +413,7 @@ static void test_multifd_tcp_no_zero_page(void)
     MigrateCommon args = {
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd_no_zero_page,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         /*
          * Multifd is more complicated than most of the features, it
          * directly takes guest page buffers when sending, make sure
@@ -438,6 +430,7 @@ static void test_multifd_tcp_channels_none(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd,
         .live = true,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         .connect_channels = ("[ { 'channel-type': 'main',"
                              "    'addr': { 'transport': 'socket',"
                              "              'type': 'inet',"
diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
index 2cb4a44bcd..30ab79e058 100644
--- a/tests/qtest/migration/tls-tests.c
+++ b/tests/qtest/migration/tls-tests.c
@@ -375,9 +375,9 @@ static void test_postcopy_tls_psk(void)
 static void test_postcopy_preempt_tls_psk(void)
 {
     MigrateCommon args = {
-        .postcopy_preempt = true,
         .start_hook = migrate_hook_start_tls_psk_match,
         .end_hook = migrate_hook_end_tls_psk,
+        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
     };
 
     test_postcopy_common(&args);
@@ -397,9 +397,9 @@ static void test_postcopy_recovery_tls_psk(void)
 static void test_postcopy_preempt_all(void)
 {
     MigrateCommon args = {
-        .postcopy_preempt = true,
         .start_hook = migrate_hook_start_tls_psk_match,
         .end_hook = migrate_hook_end_tls_psk,
+        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
     };
 
     test_postcopy_recovery_common(&args);
@@ -631,6 +631,7 @@ static void test_multifd_tcp_tls_psk_match(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tcp_tls_psk_match,
         .end_hook = migrate_hook_end_tls_psk,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -645,6 +646,7 @@ static void test_multifd_tcp_tls_psk_mismatch(void)
         .start_hook = migrate_hook_start_multifd_tcp_tls_psk_mismatch,
         .end_hook = migrate_hook_end_tls_psk,
         .result = MIG_TEST_FAIL,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -656,6 +658,7 @@ static void test_multifd_tcp_tls_x509_default_host(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_default_host,
         .end_hook = migrate_hook_end_tls_x509,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -666,6 +669,7 @@ static void test_multifd_tcp_tls_x509_override_host(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_override_host,
         .end_hook = migrate_hook_end_tls_x509,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -693,6 +697,7 @@ static void test_multifd_tcp_tls_x509_mismatch_host(void)
         .start_hook = migrate_hook_start_multifd_tls_x509_mismatch_host,
         .end_hook = migrate_hook_end_tls_x509,
         .result = MIG_TEST_FAIL,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -703,6 +708,7 @@ static void test_multifd_tcp_tls_x509_allow_anon_client(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_allow_anon_client,
         .end_hook = migrate_hook_end_tls_x509,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -717,6 +723,7 @@ static void test_multifd_tcp_tls_x509_reject_anon_client(void)
         .start_hook = migrate_hook_start_multifd_tls_x509_reject_anon_client,
         .end_hook = migrate_hook_end_tls_x509,
         .result = MIG_TEST_FAIL,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
--
2.48.1

Re: [PATCH v6 3/4] tests/qtest/migration: consolidate set capabilities

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> From: Prasad Pandit <pjp@fedoraproject.org>
>
> Migration capabilities are set in multiple '.start_hook'
> functions for various tests. Instead, consolidate setting
> capabilities in 'set_migration_capabilities()' function
> which is called from various 'test_*_common()' functions.
> While simplifying the capabilities setting, it helps
> to declutter the qtest sources.
>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
> ---
>  tests/qtest/migration/compression-tests.c | 10 ++--
>  tests/qtest/migration/cpr-tests.c         |  4 +-
>  tests/qtest/migration/file-tests.c        | 44 +++++-------------
>  tests/qtest/migration/framework.c         | 56 +++++++++++++++++------
>  tests/qtest/migration/framework.h         |  4 +-
>  tests/qtest/migration/postcopy-tests.c    |  4 +-
>  tests/qtest/migration/precopy-tests.c     | 19 +++-----
>  tests/qtest/migration/tls-tests.c         | 11 ++++-
>  8 files changed, 80 insertions(+), 72 deletions(-)
>
> v6:
> - Reorder, make this the first qtest patch in this series.
>
> v5:
> - https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t
>
> diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
> index 8b58401b84..4558a7b9ff 100644
> --- a/tests/qtest/migration/compression-tests.c
> +++ b/tests/qtest/migration/compression-tests.c
> @@ -35,6 +35,7 @@ static void test_multifd_tcp_zstd(void)
>  {
>      MigrateCommon args = {
>          .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>          .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
>      };
>      test_precopy_common(&args);
> @@ -56,6 +57,7 @@ static void test_multifd_tcp_qatzip(void)
>  {
>      MigrateCommon args = {
>          .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>          .start_hook = migrate_hook_start_precopy_tcp_multifd_qatzip,
>      };
>      test_precopy_common(&args);
> @@ -74,6 +76,7 @@ static void test_multifd_tcp_qpl(void)
>  {
>      MigrateCommon args = {
>          .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>          .start_hook = migrate_hook_start_precopy_tcp_multifd_qpl,
>      };
>      test_precopy_common(&args);
> @@ -92,6 +95,7 @@ static void test_multifd_tcp_uadk(void)
>  {
>      MigrateCommon args = {
>          .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>          .start_hook = migrate_hook_start_precopy_tcp_multifd_uadk,
>      };
>      test_precopy_common(&args);
> @@ -103,10 +107,6 @@ migrate_hook_start_xbzrle(QTestState *from,
>                            QTestState *to)
>  {
>      migrate_set_parameter_int(from, "xbzrle-cache-size", 33554432);
> -
> -    migrate_set_capability(from, "xbzrle", true);
> -    migrate_set_capability(to, "xbzrle", true);
> -
>      return NULL;
>  }
>  
> @@ -118,6 +118,7 @@ static void test_precopy_unix_xbzrle(void)
>          .listen_uri = uri,
>          .start_hook = migrate_hook_start_xbzrle,
>          .iterations = 2,
> +        .caps[MIGRATION_CAPABILITY_XBZRLE] = true,
>          /*
>           * XBZRLE needs pages to be modified when doing the 2nd+ round
>           * iteration to have real data pushed to the stream.
> @@ -146,6 +147,7 @@ static void test_multifd_tcp_zlib(void)
>  {
>      MigrateCommon args = {
>          .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>          .start_hook = migrate_hook_start_precopy_tcp_multifd_zlib,
>      };
>      test_precopy_common(&args);
> diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
> index 4758841824..a175646b57 100644
> --- a/tests/qtest/migration/cpr-tests.c
> +++ b/tests/qtest/migration/cpr-tests.c
> @@ -24,9 +24,6 @@ static void *migrate_hook_start_mode_reboot(QTestState *from, QTestState *to)
>      migrate_set_parameter_str(from, "mode", "cpr-reboot");
>      migrate_set_parameter_str(to, "mode", "cpr-reboot");
>  
> -    migrate_set_capability(from, "x-ignore-shared", true);
> -    migrate_set_capability(to, "x-ignore-shared", true);
> -
>      return NULL;
>  }
>  
> @@ -39,6 +36,7 @@ static void test_mode_reboot(void)
>          .connect_uri = uri,
>          .listen_uri = "defer",
>          .start_hook = migrate_hook_start_mode_reboot,
> +        .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true,
>      };
>  
>      test_file_common(&args, true);
> diff --git a/tests/qtest/migration/file-tests.c b/tests/qtest/migration/file-tests.c
> index f260e2871d..6253ece687 100644
> --- a/tests/qtest/migration/file-tests.c
> +++ b/tests/qtest/migration/file-tests.c
> @@ -107,15 +107,6 @@ static void test_precopy_file_offset_bad(void)
>      test_file_common(&args, false);
>  }
>  
> -static void *migrate_hook_start_mapped_ram(QTestState *from,
> -                                           QTestState *to)
> -{
> -    migrate_set_capability(from, "mapped-ram", true);
> -    migrate_set_capability(to, "mapped-ram", true);
> -
> -    return NULL;
> -}
> -
>  static void test_precopy_file_mapped_ram_live(void)
>  {
>      g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
> @@ -123,7 +114,7 @@ static void test_precopy_file_mapped_ram_live(void)
>      MigrateCommon args = {
>          .connect_uri = uri,
>          .listen_uri = "defer",
> -        .start_hook = migrate_hook_start_mapped_ram,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
>      };
>  
>      test_file_common(&args, false);
> @@ -136,26 +127,12 @@ static void test_precopy_file_mapped_ram(void)
>      MigrateCommon args = {
>          .connect_uri = uri,
>          .listen_uri = "defer",
> -        .start_hook = migrate_hook_start_mapped_ram,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
>      };
>  
>      test_file_common(&args, true);
>  }
>  
> -static void *migrate_hook_start_multifd_mapped_ram(QTestState *from,
> -                                                   QTestState *to)
> -{
> -    migrate_hook_start_mapped_ram(from, to);
> -
> -    migrate_set_parameter_int(from, "multifd-channels", 4);
> -    migrate_set_parameter_int(to, "multifd-channels", 4);
> -
> -    migrate_set_capability(from, "multifd", true);
> -    migrate_set_capability(to, "multifd", true);
> -
> -    return NULL;
> -}
> -
>  static void test_multifd_file_mapped_ram_live(void)
>  {
>      g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
> @@ -163,7 +140,8 @@ static void test_multifd_file_mapped_ram_live(void)
>      MigrateCommon args = {
>          .connect_uri = uri,
>          .listen_uri = "defer",
> -        .start_hook = migrate_hook_start_multifd_mapped_ram,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
>      };
>  
>      test_file_common(&args, false);
> @@ -176,7 +154,8 @@ static void test_multifd_file_mapped_ram(void)
>      MigrateCommon args = {
>          .connect_uri = uri,
>          .listen_uri = "defer",
> -        .start_hook = migrate_hook_start_multifd_mapped_ram,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
>      };
>  
>      test_file_common(&args, true);
> @@ -185,8 +164,6 @@ static void test_multifd_file_mapped_ram(void)
>  static void *migrate_hook_start_multifd_mapped_ram_dio(QTestState *from,
>                                                         QTestState *to)
>  {
> -    migrate_hook_start_multifd_mapped_ram(from, to);
> -
>      migrate_set_parameter_bool(from, "direct-io", true);
>      migrate_set_parameter_bool(to, "direct-io", true);
>  
> @@ -201,6 +178,8 @@ static void test_multifd_file_mapped_ram_dio(void)
>          .connect_uri = uri,
>          .listen_uri = "defer",
>          .start_hook = migrate_hook_start_multifd_mapped_ram_dio,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>      };
>  
>      if (!probe_o_direct_support(tmpfs)) {
> @@ -246,7 +225,6 @@ static void *migrate_hook_start_multifd_mapped_ram_fdset_dio(QTestState *from,
>      fdset_add_fds(from, file, O_WRONLY, 2, true);
>      fdset_add_fds(to, file, O_RDONLY, 2, true);
>  
> -    migrate_hook_start_multifd_mapped_ram(from, to);
>      migrate_set_parameter_bool(from, "direct-io", true);
>      migrate_set_parameter_bool(to, "direct-io", true);
>  
> @@ -261,8 +239,6 @@ static void *migrate_hook_start_multifd_mapped_ram_fdset(QTestState *from,
>      fdset_add_fds(from, file, O_WRONLY, 2, false);
>      fdset_add_fds(to, file, O_RDONLY, 2, false);
>  
> -    migrate_hook_start_multifd_mapped_ram(from, to);
> -
>      return NULL;
>  }
>  
> @@ -275,6 +251,8 @@ static void test_multifd_file_mapped_ram_fdset(void)
>          .listen_uri = "defer",
>          .start_hook = migrate_hook_start_multifd_mapped_ram_fdset,
>          .end_hook = migrate_hook_end_multifd_mapped_ram_fdset,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>      };
>  
>      test_file_common(&args, true);
> @@ -289,6 +267,8 @@ static void test_multifd_file_mapped_ram_fdset_dio(void)
>          .listen_uri = "defer",
>          .start_hook = migrate_hook_start_multifd_mapped_ram_fdset_dio,
>          .end_hook = migrate_hook_end_multifd_mapped_ram_fdset,
> +        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
>      };
>  
>      if (!probe_o_direct_support(tmpfs)) {
> diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
> index 10e1d04b58..82aaa13e85 100644
> --- a/tests/qtest/migration/framework.c
> +++ b/tests/qtest/migration/framework.c
> @@ -207,6 +207,31 @@ static QList *migrate_start_get_qmp_capabilities(const MigrateStart *args)
>      return capabilities;
>  }
>  
> +static void set_migration_capabilities(QTestState *from,
> +                                        QTestState *to, MigrateCommon *args)
> +{
> +    /*
> +     * MigrationCapability_lookup and MIGRATION_CAPABILITY_ constants
> +     * are from qapi-types-migration.h.
> +     */
> +    for (uint8_t i = 0; i < MIGRATION_CAPABILITY__MAX; i++)
> +    {
> +        if (!args->caps[i]) {
> +            continue;
> +        }
> +        if (from) {
> +            migrate_set_capability(from,
> +                            MigrationCapability_lookup.array[i], true);
> +        }
> +        if (to) {
> +            migrate_set_capability(to,
> +                            MigrationCapability_lookup.array[i], true);
> +        }
> +    }
> +
> +    return;
> +}
> +
>  int migrate_start(QTestState **from, QTestState **to, const char *uri,
>                    MigrateStart *args)
>  {
> @@ -440,17 +465,12 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>          args->postcopy_data = args->start_hook(from, to);
>      }
>  
> -    migrate_set_capability(from, "postcopy-ram", true);
> -    migrate_set_capability(to, "postcopy-ram", true);
> -    migrate_set_capability(to, "postcopy-blocktime", true);
> -
> -    if (args->postcopy_preempt) {
> -        migrate_set_capability(from, "postcopy-preempt", true);
> -        migrate_set_capability(to, "postcopy-preempt", true);
> -    }
> +    /* set postcopy capabilities */
> +    args->caps[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME] = true;
> +    args->caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true;
> +    set_migration_capabilities(from, to, args);
>  
>      migrate_ensure_non_converge(from);
> -
>      migrate_prepare_for_dirty_mem(from);
>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>                               "  'arguments': { "
> @@ -717,6 +737,12 @@ void test_precopy_common(MigrateCommon *args)
>          return;
>      }
>  
> +    set_migration_capabilities(from, to, args);
> +    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
> +        migrate_set_parameter_int(from, "multifd-channels", 16);
> +        migrate_set_parameter_int(to, "multifd-channels", 16);
> +    }
> +
>      if (args->start_hook) {
>          data_hook = args->start_hook(from, to);
>      }
> @@ -888,6 +914,12 @@ void test_file_common(MigrateCommon *args, bool stop_src)
>       */
>      g_assert_false(args->live);
>  
> +    set_migration_capabilities(from, to, args);
> +    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
> +        migrate_set_parameter_int(from, "multifd-channels", 4);
> +        migrate_set_parameter_int(to, "multifd-channels", 4);
> +    }
> +
>      if (g_strrstr(args->connect_uri, "offset=")) {
>          check_offset = true;
>          /*
> @@ -948,15 +980,9 @@ void *migrate_hook_start_precopy_tcp_multifd_common(QTestState *from,
>                                                      QTestState *to,
>                                                      const char *method)
>  {
> -    migrate_set_parameter_int(from, "multifd-channels", 16);
> -    migrate_set_parameter_int(to, "multifd-channels", 16);
> -
>      migrate_set_parameter_str(from, "multifd-compression", method);
>      migrate_set_parameter_str(to, "multifd-compression", method);
>  
> -    migrate_set_capability(from, "multifd", true);
> -    migrate_set_capability(to, "multifd", true);
> -
>      /* Start incoming migration from the 1st socket */
>      migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>  
> diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
> index e4a11870f6..74d53eee69 100644
> --- a/tests/qtest/migration/framework.h
> +++ b/tests/qtest/migration/framework.h
> @@ -12,6 +12,7 @@
>  #define TEST_FRAMEWORK_H
>  
>  #include "libqtest.h"
> +#include <qapi/qapi-types-migration.h>
>  
>  #define FILE_TEST_FILENAME "migfile"
>  #define FILE_TEST_OFFSET 0x1000
> @@ -207,8 +208,9 @@ typedef struct {
>  
>      /* Postcopy specific fields */
>      void *postcopy_data;
> -    bool postcopy_preempt;
>      PostcopyRecoveryFailStage postcopy_recovery_fail_stage;
> +
> +    bool caps[MIGRATION_CAPABILITY__MAX];

I just noticed this is way more suited to be at MigrateStart instead,
because then we can make the set_capabilities as part of migrate_start()
and move the events setting in there as well.

I also think we could just pick a default for multifd channels and avoid
setting it in most places. 4 is probably a good number.

-- >8 --
From 811eca2a333c98df7dd6ffe9af0c9ce81aef8731 Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Mon, 17 Feb 2025 11:55:56 -0300
Subject: [PATCH] fixup! tests/qtest/migration: consolidate set capabilities

---
 tests/qtest/migration/compression-tests.c | 24 +++++++---
 tests/qtest/migration/cpr-tests.c         |  4 +-
 tests/qtest/migration/file-tests.c        | 38 ++++++++++-----
 tests/qtest/migration/framework.c         | 56 ++++++++++++-----------
 tests/qtest/migration/framework.h         |  9 +++-
 tests/qtest/migration/misc-tests.c        |  4 +-
 tests/qtest/migration/postcopy-tests.c    |  8 +++-
 tests/qtest/migration/precopy-tests.c     | 22 ++++++---
 tests/qtest/migration/tls-tests.c         | 30 ++++++++----
 9 files changed, 127 insertions(+), 68 deletions(-)

diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
index 4558a7b9ff..41e79f031b 100644
--- a/tests/qtest/migration/compression-tests.c
+++ b/tests/qtest/migration/compression-tests.c
@@ -35,7 +35,9 @@ static void test_multifd_tcp_zstd(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
     };
     test_precopy_common(&args);
@@ -57,7 +59,9 @@ static void test_multifd_tcp_qatzip(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         .start_hook = migrate_hook_start_precopy_tcp_multifd_qatzip,
     };
     test_precopy_common(&args);
@@ -76,7 +80,9 @@ static void test_multifd_tcp_qpl(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         .start_hook = migrate_hook_start_precopy_tcp_multifd_qpl,
     };
     test_precopy_common(&args);
@@ -95,7 +101,9 @@ static void test_multifd_tcp_uadk(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         .start_hook = migrate_hook_start_precopy_tcp_multifd_uadk,
     };
     test_precopy_common(&args);
@@ -118,7 +126,9 @@ static void test_precopy_unix_xbzrle(void)
         .listen_uri = uri,
         .start_hook = migrate_hook_start_xbzrle,
         .iterations = 2,
-        .caps[MIGRATION_CAPABILITY_XBZRLE] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_XBZRLE] = true,
+        },
         /*
          * XBZRLE needs pages to be modified when doing the 2nd+ round
          * iteration to have real data pushed to the stream.
@@ -147,7 +157,9 @@ static void test_multifd_tcp_zlib(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         .start_hook = migrate_hook_start_precopy_tcp_multifd_zlib,
     };
     test_precopy_common(&args);
diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
index a175646b57..5536e14610 100644
--- a/tests/qtest/migration/cpr-tests.c
+++ b/tests/qtest/migration/cpr-tests.c
@@ -36,7 +36,9 @@ static void test_mode_reboot(void)
         .connect_uri = uri,
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_mode_reboot,
-        .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true,
+        },
     };
 
     test_file_common(&args, true);
diff --git a/tests/qtest/migration/file-tests.c b/tests/qtest/migration/file-tests.c
index 6253ece687..4d78ce0855 100644
--- a/tests/qtest/migration/file-tests.c
+++ b/tests/qtest/migration/file-tests.c
@@ -114,7 +114,9 @@ static void test_precopy_file_mapped_ram_live(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        },
     };
 
     test_file_common(&args, false);
@@ -127,7 +129,9 @@ static void test_precopy_file_mapped_ram(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        },
     };
 
     test_file_common(&args, true);
@@ -140,8 +144,10 @@ static void test_multifd_file_mapped_ram_live(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        },
     };
 
     test_file_common(&args, false);
@@ -154,8 +160,10 @@ static void test_multifd_file_mapped_ram(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+        },
     };
 
     test_file_common(&args, true);
@@ -178,8 +186,10 @@ static void test_multifd_file_mapped_ram_dio(void)
         .connect_uri = uri,
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_mapped_ram_dio,
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
 
     if (!probe_o_direct_support(tmpfs)) {
@@ -251,8 +261,10 @@ static void test_multifd_file_mapped_ram_fdset(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_mapped_ram_fdset,
         .end_hook = migrate_hook_end_multifd_mapped_ram_fdset,
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
 
     test_file_common(&args, true);
@@ -267,8 +279,10 @@ static void test_multifd_file_mapped_ram_fdset_dio(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_mapped_ram_fdset_dio,
         .end_hook = migrate_hook_end_multifd_mapped_ram_fdset,
-        .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
 
     if (!probe_o_direct_support(tmpfs)) {
diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
index 82aaa13e85..be6c245843 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -30,6 +30,7 @@
 #define QEMU_VM_FILE_MAGIC 0x5145564d
 #define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
 #define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
+#define MULTIFD_TEST_CHANNELS 4
 
 unsigned start_address;
 unsigned end_address;
@@ -207,8 +208,8 @@ static QList *migrate_start_get_qmp_capabilities(const MigrateStart *args)
     return capabilities;
 }
 
-static void set_migration_capabilities(QTestState *from,
-                                        QTestState *to, MigrateCommon *args)
+static void migrate_start_set_capabilities(QTestState *from, QTestState *to,
+                                           MigrateStart *args)
 {
     /*
      * MigrationCapability_lookup and MIGRATION_CAPABILITY_ constants
@@ -229,6 +230,27 @@ static void set_migration_capabilities(QTestState *from,
         }
     }
 
+    /*
+     * Always enable migration events.  Libvirt always uses it, let's try
+     * to mimic as closer as that.
+     */
+    migrate_set_capability(from, "events", true);
+    if (!args->defer_target_connect) {
+        migrate_set_capability(to, "events", true);
+    }
+
+    /*
+     * Default number of channels should be fine for most
+     * tests. Individual tests can override by calling
+     * migrate_set_parameter() directly.
+     */
+    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
+        migrate_set_parameter_int(from, "multifd-channels",
+                                  MULTIFD_TEST_CHANNELS);
+        migrate_set_parameter_int(to, "multifd-channels",
+                                  MULTIFD_TEST_CHANNELS);
+    }
+
     return;
 }
 
@@ -404,14 +426,7 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
         unlink(shmem_path);
     }
 
-    /*
-     * Always enable migration events.  Libvirt always uses it, let's try
-     * to mimic as closer as that.
-     */
-    migrate_set_capability(*from, "events", true);
-    if (!args->defer_target_connect) {
-        migrate_set_capability(*to, "events", true);
-    }
+    migrate_start_set_capabilities(*from, *to, args);
 
     return 0;
 }
@@ -457,6 +472,10 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 {
     QTestState *from, *to;
 
+    /* set postcopy capabilities */
+    args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME] = true;
+    args->start.caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true;
+
     if (migrate_start(&from, &to, "defer", &args->start)) {
         return -1;
     }
@@ -465,11 +484,6 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
         args->postcopy_data = args->start_hook(from, to);
     }
 
-    /* set postcopy capabilities */
-    args->caps[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME] = true;
-    args->caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true;
-    set_migration_capabilities(from, to, args);
-
     migrate_ensure_non_converge(from);
     migrate_prepare_for_dirty_mem(from);
     qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
@@ -737,12 +751,6 @@ void test_precopy_common(MigrateCommon *args)
         return;
     }
 
-    set_migration_capabilities(from, to, args);
-    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
-        migrate_set_parameter_int(from, "multifd-channels", 16);
-        migrate_set_parameter_int(to, "multifd-channels", 16);
-    }
-
     if (args->start_hook) {
         data_hook = args->start_hook(from, to);
     }
@@ -914,12 +922,6 @@ void test_file_common(MigrateCommon *args, bool stop_src)
      */
     g_assert_false(args->live);
 
-    set_migration_capabilities(from, to, args);
-    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
-        migrate_set_parameter_int(from, "multifd-channels", 4);
-        migrate_set_parameter_int(to, "multifd-channels", 4);
-    }
-
     if (g_strrstr(args->connect_uri, "offset=")) {
         check_offset = true;
         /*
diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
index 74d53eee69..01e425e64e 100644
--- a/tests/qtest/migration/framework.h
+++ b/tests/qtest/migration/framework.h
@@ -121,6 +121,13 @@ typedef struct {
 
     /* Do not connect to target monitor and qtest sockets in qtest_init */
     bool defer_target_connect;
+
+    /*
+     * Migration capabilities to be set in both source and
+     * destination. For unilateral capabilities, use
+     * migration_set_capabilities().
+     */
+    bool caps[MIGRATION_CAPABILITY__MAX];
 } MigrateStart;
 
 typedef enum PostcopyRecoveryFailStage {
@@ -209,8 +216,6 @@ typedef struct {
     /* Postcopy specific fields */
     void *postcopy_data;
     PostcopyRecoveryFailStage postcopy_recovery_fail_stage;
-
-    bool caps[MIGRATION_CAPABILITY__MAX];
 } MigrateCommon;
 
 void wait_for_serial(const char *side);
diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 2e612d9e38..54995256d8 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -98,6 +98,7 @@ static void test_ignore_shared(void)
     QTestState *from, *to;
     MigrateStart args = {
         .use_shmem = true,
+        .caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED] = true,
     };
 
     if (migrate_start(&from, &to, uri, &args)) {
@@ -107,9 +108,6 @@ static void test_ignore_shared(void)
     migrate_ensure_non_converge(from);
     migrate_prepare_for_dirty_mem(from);
 
-    migrate_set_capability(from, "x-ignore-shared", true);
-    migrate_set_capability(to, "x-ignore-shared", true);
-
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
index b0e70a6367..483e3ff99f 100644
--- a/tests/qtest/migration/postcopy-tests.c
+++ b/tests/qtest/migration/postcopy-tests.c
@@ -39,7 +39,9 @@ static void test_postcopy_suspend(void)
 static void test_postcopy_preempt(void)
 {
     MigrateCommon args = {
-        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        },
     };
 
     test_postcopy_common(&args);
@@ -73,7 +75,9 @@ static void test_postcopy_recovery_fail_reconnect(void)
 static void test_postcopy_preempt_recovery(void)
 {
     MigrateCommon args = {
-        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        },
     };
 
     test_postcopy_recovery_common(&args);
diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index a0399b78d6..f8404793b8 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -112,8 +112,10 @@ static void test_precopy_tcp_switchover_ack(void)
 {
     MigrateCommon args = {
         .listen_uri = "tcp:127.0.0.1:0",
-        .caps[MIGRATION_CAPABILITY_RETURN_PATH] = true,
-        .caps[MIGRATION_CAPABILITY_SWITCHOVER_ACK] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_RETURN_PATH] = true,
+            .caps[MIGRATION_CAPABILITY_SWITCHOVER_ACK] = true,
+        },
         /*
          * Source VM must be running in order to consider the switchover ACK
          * when deciding to do switchover or not.
@@ -382,7 +384,9 @@ static void test_multifd_tcp_uri_none(void)
     MigrateCommon args = {
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         /*
          * Multifd is more complicated than most of the features, it
          * directly takes guest page buffers when sending, make sure
@@ -398,7 +402,9 @@ static void test_multifd_tcp_zero_page_legacy(void)
     MigrateCommon args = {
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd_zero_page_legacy,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         /*
          * Multifd is more complicated than most of the features, it
          * directly takes guest page buffers when sending, make sure
@@ -414,7 +420,9 @@ static void test_multifd_tcp_no_zero_page(void)
     MigrateCommon args = {
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd_no_zero_page,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         /*
          * Multifd is more complicated than most of the features, it
          * directly takes guest page buffers when sending, make sure
@@ -431,7 +439,9 @@ static void test_multifd_tcp_channels_none(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_precopy_tcp_multifd,
         .live = true,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
         .connect_channels = ("[ { 'channel-type': 'main',"
                              "    'addr': { 'transport': 'socket',"
                              "              'type': 'inet',"
diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
index 30ab79e058..72f44defbb 100644
--- a/tests/qtest/migration/tls-tests.c
+++ b/tests/qtest/migration/tls-tests.c
@@ -377,7 +377,9 @@ static void test_postcopy_preempt_tls_psk(void)
     MigrateCommon args = {
         .start_hook = migrate_hook_start_tls_psk_match,
         .end_hook = migrate_hook_end_tls_psk,
-        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        },
     };
 
     test_postcopy_common(&args);
@@ -399,7 +401,9 @@ static void test_postcopy_preempt_all(void)
     MigrateCommon args = {
         .start_hook = migrate_hook_start_tls_psk_match,
         .end_hook = migrate_hook_end_tls_psk,
-        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+        },
     };
 
     test_postcopy_recovery_common(&args);
@@ -631,7 +635,9 @@ static void test_multifd_tcp_tls_psk_match(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tcp_tls_psk_match,
         .end_hook = migrate_hook_end_tls_psk,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
     test_precopy_common(&args);
 }
@@ -641,12 +647,12 @@ static void test_multifd_tcp_tls_psk_mismatch(void)
     MigrateCommon args = {
         .start = {
             .hide_stderr = true,
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         },
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tcp_tls_psk_mismatch,
         .end_hook = migrate_hook_end_tls_psk,
         .result = MIG_TEST_FAIL,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -658,7 +664,9 @@ static void test_multifd_tcp_tls_x509_default_host(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_default_host,
         .end_hook = migrate_hook_end_tls_x509,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
     test_precopy_common(&args);
 }
@@ -669,7 +677,9 @@ static void test_multifd_tcp_tls_x509_override_host(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_override_host,
         .end_hook = migrate_hook_end_tls_x509,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
     test_precopy_common(&args);
 }
@@ -692,12 +702,12 @@ static void test_multifd_tcp_tls_x509_mismatch_host(void)
     MigrateCommon args = {
         .start = {
             .hide_stderr = true,
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         },
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_mismatch_host,
         .end_hook = migrate_hook_end_tls_x509,
         .result = MIG_TEST_FAIL,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
@@ -708,7 +718,9 @@ static void test_multifd_tcp_tls_x509_allow_anon_client(void)
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_allow_anon_client,
         .end_hook = migrate_hook_end_tls_x509,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start = {
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        },
     };
     test_precopy_common(&args);
 }
@@ -718,12 +730,12 @@ static void test_multifd_tcp_tls_x509_reject_anon_client(void)
     MigrateCommon args = {
         .start = {
             .hide_stderr = true,
+            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
         },
         .listen_uri = "defer",
         .start_hook = migrate_hook_start_multifd_tls_x509_reject_anon_client,
         .end_hook = migrate_hook_end_tls_x509,
         .result = MIG_TEST_FAIL,
-        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
     };
     test_precopy_common(&args);
 }
-- 
2.35.3

Re: [PATCH v6 3/4] tests/qtest/migration: consolidate set capabilities

[Prasad Pandit]

On Mon, 17 Feb 2025 at 20:47, Fabiano Rosas <farosas@suse.de> wrote:
> I just noticed this is way more suited to be at MigrateStart instead,
> because then we can make the set_capabilities as part of migrate_start()
> and move the events setting in there as well.
>
> I also think we could just pick a default for multifd channels and avoid
> setting it in most places. 4 is probably a good number.

* Okay, sounds reasonable. I was thinking of setting multifd channels
to 8 as default, but didn't want to deviate from current code so kept
all values of 4/8/16.

Thank you.
---
  - Prasad

[PATCH v6 4/4] tests/qtest/migration: add postcopy tests with multifd

[Prasad Pandit]

From: Prasad Pandit <pjp@fedoraproject.org>

Add new qtests to run postcopy migration with multifd
channels enabled.

Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
---
 tests/qtest/migration/compression-tests.c | 13 ++++++++
 tests/qtest/migration/framework.c         |  4 +++
 tests/qtest/migration/postcopy-tests.c    | 23 +++++++++++++
 tests/qtest/migration/precopy-tests.c     | 19 +++++++++++
 tests/qtest/migration/tls-tests.c         | 40 +++++++++++++++++++++++
 5 files changed, 99 insertions(+)

v6:
- Reorder, make this the second patch in this series.

v5:
- https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t

diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
index 4558a7b9ff..d4d6b3c4de 100644
--- a/tests/qtest/migration/compression-tests.c
+++ b/tests/qtest/migration/compression-tests.c
@@ -40,6 +40,17 @@ static void test_multifd_tcp_zstd(void)
     };
     test_precopy_common(&args);
 }
+
+static void test_multifd_postcopy_tcp_zstd(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true,
+        .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
+    };
+
+    test_precopy_common(&args);
+}
 #endif /* CONFIG_ZSTD */
 
 #ifdef CONFIG_QATZIP
@@ -172,6 +183,8 @@ void migration_test_add_compression(MigrationTestEnv *env)
 #ifdef CONFIG_ZSTD
     migration_test_add("/migration/multifd/tcp/plain/zstd",
                        test_multifd_tcp_zstd);
+    migration_test_add("/migration/multifd+postcopy/tcp/plain/zstd",
+                       test_multifd_postcopy_tcp_zstd);
 #endif
 
 #ifdef CONFIG_QATZIP
diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
index 82aaa13e85..2396405b51 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -469,6 +469,10 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     args->caps[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME] = true;
     args->caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true;
     set_migration_capabilities(from, to, args);
+    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
+        migrate_set_parameter_int(from, "multifd-channels", 8);
+        migrate_set_parameter_int(to, "multifd-channels", 8);
+    }
 
     migrate_ensure_non_converge(from);
     migrate_prepare_for_dirty_mem(from);
diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
index b0e70a6367..32fe7b0324 100644
--- a/tests/qtest/migration/postcopy-tests.c
+++ b/tests/qtest/migration/postcopy-tests.c
@@ -90,6 +90,25 @@ static void migration_test_add_postcopy_smoke(MigrationTestEnv *env)
     }
 }
 
+static void test_multifd_postcopy(void)
+{
+    MigrateCommon args = {
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+    };
+
+    test_postcopy_common(&args);
+}
+
+static void test_multifd_postcopy_preempt(void)
+{
+    MigrateCommon args = {
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
+    };
+
+    test_postcopy_common(&args);
+}
+
 void migration_test_add_postcopy(MigrationTestEnv *env)
 {
     migration_test_add_postcopy_smoke(env);
@@ -110,6 +129,10 @@ void migration_test_add_postcopy(MigrationTestEnv *env)
             "/migration/postcopy/recovery/double-failures/reconnect",
             test_postcopy_recovery_fail_reconnect);
 
+        migration_test_add("/migration/multifd+postcopy/plain",
+                           test_multifd_postcopy);
+        migration_test_add("/migration/multifd+postcopy/preempt/plain",
+                           test_multifd_postcopy_preempt);
         if (env->is_x86) {
             migration_test_add("/migration/postcopy/suspend",
                                test_postcopy_suspend);
diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index e5d8c49dbe..2126cb8e2c 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -33,6 +33,7 @@
 #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
 
 static char *tmpfs;
+static bool postcopy_ram = false;
 
 static void test_precopy_unix_plain(void)
 {
@@ -465,6 +466,11 @@ static void test_multifd_tcp_cancel(void)
     migrate_ensure_non_converge(from);
     migrate_prepare_for_dirty_mem(from);
 
+    if (postcopy_ram) {
+        migrate_set_capability(from, "postcopy-ram", true);
+        migrate_set_capability(to, "postcopy-ram", true);
+    }
+
     migrate_set_parameter_int(from, "multifd-channels", 16);
     migrate_set_parameter_int(to, "multifd-channels", 16);
 
@@ -506,6 +512,10 @@ static void test_multifd_tcp_cancel(void)
         return;
     }
 
+    if (postcopy_ram) {
+        migrate_set_capability(to2, "postcopy-ram", true);
+    }
+
     migrate_set_parameter_int(to2, "multifd-channels", 16);
 
     migrate_set_capability(to2, "multifd", true);
@@ -529,6 +539,13 @@ static void test_multifd_tcp_cancel(void)
     migrate_end(from, to2, true);
 }
 
+static void test_multifd_postcopy_tcp_cancel(void)
+{
+    postcopy_ram = true;
+    test_multifd_tcp_cancel();
+    postcopy_ram = false;
+}
+
 static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
 {
     qtest_qmp_assert_success(who,
@@ -1001,6 +1018,8 @@ void migration_test_add_precopy(MigrationTestEnv *env)
                        test_multifd_tcp_zero_page_legacy);
     migration_test_add("/migration/multifd/tcp/plain/zero-page/none",
                        test_multifd_tcp_no_zero_page);
+    migration_test_add("migration/multifd+postcopy/tcp/plain/cancel",
+                       test_multifd_postcopy_tcp_cancel);
     if (g_str_equal(env->arch, "x86_64")
         && env->has_kvm && env->has_dirty_ring) {
 
diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
index 30ab79e058..ce57f0cb5d 100644
--- a/tests/qtest/migration/tls-tests.c
+++ b/tests/qtest/migration/tls-tests.c
@@ -393,6 +393,17 @@ static void test_postcopy_recovery_tls_psk(void)
     test_postcopy_recovery_common(&args);
 }
 
+static void test_multifd_postcopy_recovery_tls_psk(void)
+{
+    MigrateCommon args = {
+        .start_hook = migrate_hook_start_tls_psk_match,
+        .end_hook = migrate_hook_end_tls_psk,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+    };
+
+    test_postcopy_recovery_common(&args);
+}
+
 /* This contains preempt+recovery+tls test altogether */
 static void test_postcopy_preempt_all(void)
 {
@@ -405,6 +416,17 @@ static void test_postcopy_preempt_all(void)
     test_postcopy_recovery_common(&args);
 }
 
+static void test_multifd_postcopy_preempt_recovery_tls_psk(void)
+{
+    MigrateCommon args = {
+        .start_hook = migrate_hook_start_tls_psk_match,
+        .end_hook = migrate_hook_end_tls_psk,
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+    };
+
+    test_postcopy_recovery_common(&args);
+}
+
 static void test_precopy_unix_tls_psk(void)
 {
     g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
@@ -651,6 +673,18 @@ static void test_multifd_tcp_tls_psk_mismatch(void)
     test_precopy_common(&args);
 }
 
+static void test_multifd_postcopy_tcp_tls_psk_match(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
+        .start_hook = migrate_hook_start_multifd_tcp_tls_psk_match,
+        .end_hook = migrate_hook_end_tls_psk,
+    };
+
+    test_precopy_common(&args);
+}
+
 #ifdef CONFIG_TASN1
 static void test_multifd_tcp_tls_x509_default_host(void)
 {
@@ -762,6 +796,10 @@ void migration_test_add_tls(MigrationTestEnv *env)
                            test_postcopy_preempt_tls_psk);
         migration_test_add("/migration/postcopy/preempt/recovery/tls/psk",
                            test_postcopy_preempt_all);
+        migration_test_add("/migration/multifd+postcopy/recovery/tls/psk",
+                           test_multifd_postcopy_recovery_tls_psk);
+        migration_test_add("/migration/multifd+postcopy/preempt/recovery/tls/psk",
+                           test_multifd_postcopy_preempt_recovery_tls_psk);
     }
 #ifdef CONFIG_TASN1
     migration_test_add("/migration/precopy/unix/tls/x509/default-host",
@@ -793,6 +831,8 @@ void migration_test_add_tls(MigrationTestEnv *env)
                        test_multifd_tcp_tls_psk_match);
     migration_test_add("/migration/multifd/tcp/tls/psk/mismatch",
                        test_multifd_tcp_tls_psk_mismatch);
+    migration_test_add("/migration/multifd+postcopy/tcp/tls/psk/match",
+                       test_multifd_postcopy_tcp_tls_psk_match);
 #ifdef CONFIG_TASN1
     migration_test_add("/migration/multifd/tcp/tls/x509/default-host",
                        test_multifd_tcp_tls_x509_default_host);
--
2.48.1

Re: [PATCH v6 4/4] tests/qtest/migration: add postcopy tests with multifd

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> From: Prasad Pandit <pjp@fedoraproject.org>
>
> Add new qtests to run postcopy migration with multifd
> channels enabled.
>
> Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
> ---
>  tests/qtest/migration/compression-tests.c | 13 ++++++++
>  tests/qtest/migration/framework.c         |  4 +++
>  tests/qtest/migration/postcopy-tests.c    | 23 +++++++++++++
>  tests/qtest/migration/precopy-tests.c     | 19 +++++++++++
>  tests/qtest/migration/tls-tests.c         | 40 +++++++++++++++++++++++
>  5 files changed, 99 insertions(+)
>
> v6:
> - Reorder, make this the second patch in this series.
>
> v5:
> - https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t
>
> diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
> index 4558a7b9ff..d4d6b3c4de 100644
> --- a/tests/qtest/migration/compression-tests.c
> +++ b/tests/qtest/migration/compression-tests.c
> @@ -40,6 +40,17 @@ static void test_multifd_tcp_zstd(void)
>      };
>      test_precopy_common(&args);
>  }
> +
> +static void test_multifd_postcopy_tcp_zstd(void)
> +{
> +    MigrateCommon args = {
> +        .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true,
> +        .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
> +    };
> +
> +    test_precopy_common(&args);
> +}
>  #endif /* CONFIG_ZSTD */
>  
>  #ifdef CONFIG_QATZIP
> @@ -172,6 +183,8 @@ void migration_test_add_compression(MigrationTestEnv *env)
>  #ifdef CONFIG_ZSTD
>      migration_test_add("/migration/multifd/tcp/plain/zstd",
>                         test_multifd_tcp_zstd);
> +    migration_test_add("/migration/multifd+postcopy/tcp/plain/zstd",
> +                       test_multifd_postcopy_tcp_zstd);
>  #endif
>  
>  #ifdef CONFIG_QATZIP
> diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
> index 82aaa13e85..2396405b51 100644
> --- a/tests/qtest/migration/framework.c
> +++ b/tests/qtest/migration/framework.c
> @@ -469,6 +469,10 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      args->caps[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME] = true;
>      args->caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] = true;
>      set_migration_capabilities(from, to, args);
> +    if (args->caps[MIGRATION_CAPABILITY_MULTIFD]) {
> +        migrate_set_parameter_int(from, "multifd-channels", 8);
> +        migrate_set_parameter_int(to, "multifd-channels", 8);
> +    }
>  
>      migrate_ensure_non_converge(from);
>      migrate_prepare_for_dirty_mem(from);
> diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
> index b0e70a6367..32fe7b0324 100644
> --- a/tests/qtest/migration/postcopy-tests.c
> +++ b/tests/qtest/migration/postcopy-tests.c
> @@ -90,6 +90,25 @@ static void migration_test_add_postcopy_smoke(MigrationTestEnv *env)
>      }
>  }
>  
> +static void test_multifd_postcopy(void)
> +{
> +    MigrateCommon args = {
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +    };
> +
> +    test_postcopy_common(&args);
> +}
> +
> +static void test_multifd_postcopy_preempt(void)
> +{
> +    MigrateCommon args = {
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +        .caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT] = true,
> +    };
> +
> +    test_postcopy_common(&args);
> +}
> +
>  void migration_test_add_postcopy(MigrationTestEnv *env)
>  {
>      migration_test_add_postcopy_smoke(env);
> @@ -110,6 +129,10 @@ void migration_test_add_postcopy(MigrationTestEnv *env)
>              "/migration/postcopy/recovery/double-failures/reconnect",
>              test_postcopy_recovery_fail_reconnect);
>  
> +        migration_test_add("/migration/multifd+postcopy/plain",
> +                           test_multifd_postcopy);
> +        migration_test_add("/migration/multifd+postcopy/preempt/plain",
> +                           test_multifd_postcopy_preempt);

For postcopy-tests.c I'd use /migration/postcopy/multifd so we can run
them all via command-line. These are also the only ones actually doing
postcopy migration. We need to distinguish multifd+postcopy proper from
merely postcopy-ram=true.

>          if (env->is_x86) {
>              migration_test_add("/migration/postcopy/suspend",
>                                 test_postcopy_suspend);
> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> index e5d8c49dbe..2126cb8e2c 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -33,6 +33,7 @@
>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>  
>  static char *tmpfs;
> +static bool postcopy_ram = false;
>  
>  static void test_precopy_unix_plain(void)
>  {
> @@ -465,6 +466,11 @@ static void test_multifd_tcp_cancel(void)
>      migrate_ensure_non_converge(from);
>      migrate_prepare_for_dirty_mem(from);
>  
> +    if (postcopy_ram) {
> +        migrate_set_capability(from, "postcopy-ram", true);
> +        migrate_set_capability(to, "postcopy-ram", true);
> +    }
> +
>      migrate_set_parameter_int(from, "multifd-channels", 16);
>      migrate_set_parameter_int(to, "multifd-channels", 16);
>  
> @@ -506,6 +512,10 @@ static void test_multifd_tcp_cancel(void)
>          return;
>      }
>  
> +    if (postcopy_ram) {
> +        migrate_set_capability(to2, "postcopy-ram", true);
> +    }
> +
>      migrate_set_parameter_int(to2, "multifd-channels", 16);
>  
>      migrate_set_capability(to2, "multifd", true);
> @@ -529,6 +539,13 @@ static void test_multifd_tcp_cancel(void)
>      migrate_end(from, to2, true);
>  }
>  
> +static void test_multifd_postcopy_tcp_cancel(void)
> +{
> +    postcopy_ram = true;
> +    test_multifd_tcp_cancel();
> +    postcopy_ram = false;

You could pass this in, there's just one other caller.

> +}
> +
>  static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
>  {
>      qtest_qmp_assert_success(who,
> @@ -1001,6 +1018,8 @@ void migration_test_add_precopy(MigrationTestEnv *env)
>                         test_multifd_tcp_zero_page_legacy);
>      migration_test_add("/migration/multifd/tcp/plain/zero-page/none",
>                         test_multifd_tcp_no_zero_page);
> +    migration_test_add("migration/multifd+postcopy/tcp/plain/cancel",
> +                       test_multifd_postcopy_tcp_cancel);
>      if (g_str_equal(env->arch, "x86_64")
>          && env->has_kvm && env->has_dirty_ring) {
>  
> diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
> index 30ab79e058..ce57f0cb5d 100644
> --- a/tests/qtest/migration/tls-tests.c
> +++ b/tests/qtest/migration/tls-tests.c
> @@ -393,6 +393,17 @@ static void test_postcopy_recovery_tls_psk(void)
>      test_postcopy_recovery_common(&args);
>  }
>  
> +static void test_multifd_postcopy_recovery_tls_psk(void)
> +{
> +    MigrateCommon args = {
> +        .start_hook = migrate_hook_start_tls_psk_match,
> +        .end_hook = migrate_hook_end_tls_psk,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +    };
> +
> +    test_postcopy_recovery_common(&args);
> +}
> +
>  /* This contains preempt+recovery+tls test altogether */
>  static void test_postcopy_preempt_all(void)
>  {
> @@ -405,6 +416,17 @@ static void test_postcopy_preempt_all(void)
>      test_postcopy_recovery_common(&args);
>  }
>  
> +static void test_multifd_postcopy_preempt_recovery_tls_psk(void)
> +{
> +    MigrateCommon args = {
> +        .start_hook = migrate_hook_start_tls_psk_match,
> +        .end_hook = migrate_hook_end_tls_psk,
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +    };
> +
> +    test_postcopy_recovery_common(&args);
> +}
> +
>  static void test_precopy_unix_tls_psk(void)
>  {
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -651,6 +673,18 @@ static void test_multifd_tcp_tls_psk_mismatch(void)
>      test_precopy_common(&args);
>  }
>  
> +static void test_multifd_postcopy_tcp_tls_psk_match(void)
> +{
> +    MigrateCommon args = {
> +        .listen_uri = "defer",
> +        .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> +        .start_hook = migrate_hook_start_multifd_tcp_tls_psk_match,
> +        .end_hook = migrate_hook_end_tls_psk,
> +    };
> +
> +    test_precopy_common(&args);
> +}
> +
>  #ifdef CONFIG_TASN1
>  static void test_multifd_tcp_tls_x509_default_host(void)
>  {
> @@ -762,6 +796,10 @@ void migration_test_add_tls(MigrationTestEnv *env)
>                             test_postcopy_preempt_tls_psk);
>          migration_test_add("/migration/postcopy/preempt/recovery/tls/psk",
>                             test_postcopy_preempt_all);
> +        migration_test_add("/migration/multifd+postcopy/recovery/tls/psk",
> +                           test_multifd_postcopy_recovery_tls_psk);
> +        migration_test_add("/migration/multifd+postcopy/preempt/recovery/tls/psk",
> +                           test_multifd_postcopy_preempt_recovery_tls_psk);
>      }
>  #ifdef CONFIG_TASN1
>      migration_test_add("/migration/precopy/unix/tls/x509/default-host",
> @@ -793,6 +831,8 @@ void migration_test_add_tls(MigrationTestEnv *env)
>                         test_multifd_tcp_tls_psk_match);
>      migration_test_add("/migration/multifd/tcp/tls/psk/mismatch",
>                         test_multifd_tcp_tls_psk_mismatch);
> +    migration_test_add("/migration/multifd+postcopy/tcp/tls/psk/match",
> +                       test_multifd_postcopy_tcp_tls_psk_match);
>  #ifdef CONFIG_TASN1
>      migration_test_add("/migration/multifd/tcp/tls/x509/default-host",
>                         test_multifd_tcp_tls_x509_default_host);
> --
> 2.48.1

Re: [PATCH v6 4/4] tests/qtest/migration: add postcopy tests with multifd

[Prasad Pandit]

On Mon, 17 Feb 2025 at 21:03, Fabiano Rosas <farosas@suse.de> wrote:
> > @@ -110,6 +129,10 @@ void migration_test_add_postcopy(MigrationTestEnv *env)
> >              "/migration/postcopy/recovery/double-failures/reconnect",
> >              test_postcopy_recovery_fail_reconnect);
> >
> > +        migration_test_add("/migration/multifd+postcopy/plain",
> > +                           test_multifd_postcopy);
> > +        migration_test_add("/migration/multifd+postcopy/preempt/plain",
> > +                           test_multifd_postcopy_preempt);
>
> For postcopy-tests.c I'd use /migration/postcopy/multifd so we can run
> them all via command-line. These are also the only ones actually doing
> postcopy migration. We need to distinguish multifd+postcopy proper from
> merely postcopy-ram=true.

* ie. repalce 'multifd+postcopy'  with '../postcopy/multifd/' only in
postcopy-tests.c? And keep other instances unchanged?

...
> >
> > +static void test_multifd_postcopy_tcp_cancel(void)
> > +{
> > +    postcopy_ram = true;
> > +    test_multifd_tcp_cancel();
> > +    postcopy_ram = false;
>
> You could pass this in, there's just one other caller.

* Sorry, what do you mean here?

Thank you.
---
  - Prasad

Re: [PATCH v6 4/4] tests/qtest/migration: add postcopy tests with multifd

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> On Mon, 17 Feb 2025 at 21:03, Fabiano Rosas <farosas@suse.de> wrote:
>> > @@ -110,6 +129,10 @@ void migration_test_add_postcopy(MigrationTestEnv *env)
>> >              "/migration/postcopy/recovery/double-failures/reconnect",
>> >              test_postcopy_recovery_fail_reconnect);
>> >
>> > +        migration_test_add("/migration/multifd+postcopy/plain",
>> > +                           test_multifd_postcopy);
>> > +        migration_test_add("/migration/multifd+postcopy/preempt/plain",
>> > +                           test_multifd_postcopy_preempt);
>>
>> For postcopy-tests.c I'd use /migration/postcopy/multifd so we can run
>> them all via command-line. These are also the only ones actually doing
>> postcopy migration. We need to distinguish multifd+postcopy proper from
>> merely postcopy-ram=true.
>
> * ie. repalce 'multifd+postcopy'  with '../postcopy/multifd/' only in
> postcopy-tests.c? And keep other instances unchanged?
>

Yes. Or do someting else for the others, I don't have a preference. But
we need to have this be different in a way that people expecting to test
postcopy can do that along with the rest of the postcopy tests and we
also make it more clear that postcopy is not actually being tested in
the precopy tests.

These would actually do postcopy:
postcopy/multifd
multifd/postcopy

These just enable the postcopy cap:
precopy/postcopy-ram/
multifd/postcopy-ram/

The exact names could change...

> ...
>> >
>> > +static void test_multifd_postcopy_tcp_cancel(void)
>> > +{
>> > +    postcopy_ram = true;
>> > +    test_multifd_tcp_cancel();
>> > +    postcopy_ram = false;
>>
>> You could pass this in, there's just one other caller.
>
> * Sorry, what do you mean here?

To make postcopy_ram be passed in as an argument to
test_multifd_tcp_cancel(). Having globals tend to get in the way of
refactoring stuff later. We already had issues with tmpfs being global
all over the place.

>
> Thank you.
> ---
>   - Prasad

Re: [PATCH v6 4/4] tests/qtest/migration: add postcopy tests with multifd

[Prasad Pandit]

Hello Fabiano,

On Tue, 18 Feb 2025 at 19:58, Fabiano Rosas <farosas@suse.de> wrote:
> >> > +static void test_multifd_postcopy_tcp_cancel(void)
> >> > +{
> >> > +    postcopy_ram = true;
> >> > +    test_multifd_tcp_cancel();
> >> > +    postcopy_ram = false;
> >>
> >> You could pass this in, there's just one other caller.
>
> To make postcopy_ram be passed in as an argument to
> test_multifd_tcp_cancel(). Having globals tend to get in the way of
> refactoring stuff later. We already had issues with tmpfs being global
> all over the place.

* This looks tricky to do. test_multifd_tcp_cancel() is called via
migration_test_add(), which expects a function pointer of type => void
(*fn)(void). Changing 'migration_test_add' signature would entail
adding a parameter to all functions called by it.

Thank you.
---
  - Prasad

Re: [PATCH v6 4/4] tests/qtest/migration: add postcopy tests with multifd

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> Hello Fabiano,
>
> On Tue, 18 Feb 2025 at 19:58, Fabiano Rosas <farosas@suse.de> wrote:
>> >> > +static void test_multifd_postcopy_tcp_cancel(void)
>> >> > +{
>> >> > +    postcopy_ram = true;
>> >> > +    test_multifd_tcp_cancel();
>> >> > +    postcopy_ram = false;
>> >>
>> >> You could pass this in, there's just one other caller.
>>
>> To make postcopy_ram be passed in as an argument to
>> test_multifd_tcp_cancel(). Having globals tend to get in the way of
>> refactoring stuff later. We already had issues with tmpfs being global
>> all over the place.
>
> * This looks tricky to do. test_multifd_tcp_cancel() is called via
> migration_test_add(), which expects a function pointer of type => void
> (*fn)(void). Changing 'migration_test_add' signature would entail
> adding a parameter to all functions called by it.
>

Indeed. Leave it then. If I think of something I'll let you know.

> Thank you.
> ---
>   - Prasad