All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/4] Allow to enable multifd and postcopy migration together
@ 2025-01-27 12:08 Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 1/4] migration/multifd: move macros to multifd header Prasad Pandit
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Prasad Pandit @ 2025-01-27 12:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: peterx, farosas, berrange, Prasad Pandit

From: Prasad Pandit <pjp@fedoraproject.org>

Hello,

* 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: refactor ram_save_target_page functions
  migration: enable multifd and postcopy together
  tests/qtest/migration: add postcopy tests with multifd

 migration/migration.c                     | 106 ++++++++++++++--------
 migration/multifd-nocomp.c                |   3 +-
 migration/multifd.c                       |   5 -
 migration/multifd.h                       |   5 +
 migration/options.c                       |   5 -
 migration/ram.c                           |  69 ++++----------
 tests/qtest/migration/compression-tests.c |  13 +++
 tests/qtest/migration/framework.c         |  13 +++
 tests/qtest/migration/framework.h         |   4 +
 tests/qtest/migration/postcopy-tests.c    |  23 +++++
 tests/qtest/migration/precopy-tests.c     |  19 ++++
 tests/qtest/migration/tls-tests.c         |  40 ++++++++
 12 files changed, 203 insertions(+), 102 deletions(-)

--
2.48.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4 1/4] migration/multifd: move macros to multifd header
  2025-01-27 12:08 [PATCH v4 0/4] Allow to enable multifd and postcopy migration together Prasad Pandit
@ 2025-01-27 12:08 ` Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 2/4] migration: refactor ram_save_target_page functions Prasad Pandit
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Prasad Pandit @ 2025-01-27 12:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: peterx, farosas, berrange, 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(-)

v3: No change
- https://lore.kernel.org/qemu-devel/20250121131032.1611245-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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 2/4] migration: refactor ram_save_target_page functions
  2025-01-27 12:08 [PATCH v4 0/4] Allow to enable multifd and postcopy migration together Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 1/4] migration/multifd: move macros to multifd header Prasad Pandit
@ 2025-01-27 12:08 ` Prasad Pandit
  2025-01-27 18:46   ` Fabiano Rosas
  2025-01-27 12:08 ` [PATCH v4 3/4] migration: enable multifd and postcopy together Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd Prasad Pandit
  3 siblings, 1 reply; 11+ messages in thread
From: Prasad Pandit @ 2025-01-27 12:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: peterx, farosas, berrange, Prasad Pandit

From: Prasad Pandit <pjp@fedoraproject.org>

Refactor ram_save_target_page legacy and multifd
functions into one. Other than simplifying it,
it frees 'migration_ops' object from usage, so it
is expunged.

Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
---
 migration/ram.c | 67 +++++++++++++------------------------------------
 1 file changed, 17 insertions(+), 50 deletions(-)

v3: No change
- https://lore.kernel.org/qemu-devel/20250121131032.1611245-1-ppandit@redhat.com/T/#t

diff --git a/migration/ram.c b/migration/ram.c
index ce28328141..f2326788de 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -446,13 +446,6 @@ void ram_transferred_add(uint64_t bytes)
     }
 }
 
-struct MigrationOps {
-    int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
-};
-typedef struct MigrationOps MigrationOps;
-
-MigrationOps *migration_ops;
-
 static int ram_save_host_page_urgent(PageSearchStatus *pss);
 
 /* NOTE: page is the PFN not real ram_addr_t. */
@@ -1958,55 +1951,36 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len,
 }
 
 /**
- * ram_save_target_page_legacy: save one target page
- *
- * Returns the number of pages written
+ * ram_save_target_page: save one target page to the precopy thread
+ * OR to multifd workers.
  *
  * @rs: current RAM state
  * @pss: data about the page we want to send
  */
-static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
+static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
 {
     ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
     int res;
 
+    if (!migrate_multifd()
+        || migrate_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
+        if (save_zero_page(rs, pss, offset)) {
+            return 1;
+        }
+    }
+
+    if (migrate_multifd()) {
+        RAMBlock *block = pss->block;
+        return ram_save_multifd_page(block, offset);
+    }
+
     if (control_save_page(pss, offset, &res)) {
         return res;
     }
 
-    if (save_zero_page(rs, pss, offset)) {
-        return 1;
-    }
-
     return ram_save_page(rs, pss);
 }
 
-/**
- * ram_save_target_page_multifd: send one target page to multifd workers
- *
- * Returns 1 if the page was queued, -1 otherwise.
- *
- * @rs: current RAM state
- * @pss: data about the page we want to send
- */
-static int ram_save_target_page_multifd(RAMState *rs, PageSearchStatus *pss)
-{
-    RAMBlock *block = pss->block;
-    ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
-
-    /*
-     * While using multifd live migration, we still need to handle zero
-     * page checking on the migration main thread.
-     */
-    if (migrate_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
-        if (save_zero_page(rs, pss, offset)) {
-            return 1;
-        }
-    }
-
-    return ram_save_multifd_page(block, offset);
-}
-
 /* Should be called before sending a host page */
 static void pss_host_page_prepare(PageSearchStatus *pss)
 {
@@ -2093,7 +2067,7 @@ static int ram_save_host_page_urgent(PageSearchStatus *pss)
 
         if (page_dirty) {
             /* Be strict to return code; it must be 1, or what else? */
-            if (migration_ops->ram_save_target_page(rs, pss) != 1) {
+            if (ram_save_target_page(rs, pss) != 1) {
                 error_report_once("%s: ram_save_target_page failed", __func__);
                 ret = -1;
                 goto out;
@@ -2162,7 +2136,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss)
             if (preempt_active) {
                 qemu_mutex_unlock(&rs->bitmap_mutex);
             }
-            tmppages = migration_ops->ram_save_target_page(rs, pss);
+            tmppages = ram_save_target_page(rs, pss);
             if (tmppages >= 0) {
                 pages += tmppages;
                 /*
@@ -2360,8 +2334,6 @@ static void ram_save_cleanup(void *opaque)
     xbzrle_cleanup();
     multifd_ram_save_cleanup();
     ram_state_cleanup(rsp);
-    g_free(migration_ops);
-    migration_ops = NULL;
 }
 
 static void ram_state_reset(RAMState *rs)
@@ -3027,13 +2999,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
         return ret;
     }
 
-    migration_ops = g_malloc0(sizeof(MigrationOps));
-
     if (migrate_multifd()) {
         multifd_ram_save_setup();
-        migration_ops->ram_save_target_page = ram_save_target_page_multifd;
-    } else {
-        migration_ops->ram_save_target_page = ram_save_target_page_legacy;
     }
 
     /*
-- 
2.48.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 3/4] migration: enable multifd and postcopy together
  2025-01-27 12:08 [PATCH v4 0/4] Allow to enable multifd and postcopy migration together Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 1/4] migration/multifd: move macros to multifd header Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 2/4] migration: refactor ram_save_target_page functions Prasad Pandit
@ 2025-01-27 12:08 ` Prasad Pandit
  2025-01-27 12:08 ` [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd Prasad Pandit
  3 siblings, 0 replies; 11+ messages in thread
From: Prasad Pandit @ 2025-01-27 12:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: peterx, farosas, berrange, 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
restrained and Postcopy threads on the destination
request/pull data from the source side.

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

v3: No change
- https://lore.kernel.org/qemu-devel/20250121131032.1611245-1-ppandit@redhat.com/T/#t

diff --git a/migration/migration.c b/migration/migration.c
index 2d1da917c7..a280722e9e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -92,6 +92,9 @@ enum mig_rp_message_type {
     MIG_RP_MSG_MAX
 };
 
+/* Migration channel types */
+enum { CH_DEFAULT, 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 */
@@ -929,26 +932,33 @@ void migration_fd_process_incoming(QEMUFile *f)
 /*
  * Returns true when we want to start a new incoming migration process,
  * false otherwise.
+ *
+ * All the required channels must be in place before a new incoming
+ * migration process starts.
+ *  - Multifd enabled:
+ *    The main channel and the multifd channels are required.
+ *  - Multifd/Postcopy disabled:
+ *    The main channel is required.
+ *  - Postcopy enabled:
+ *    We don't want to start a new incoming migration when
+ *    the postcopy channel is created. Because it is created
+ *    towards the end of the precopy migration.
+ *
  */
-static bool migration_should_start_incoming(bool main_channel)
+static bool migration_should_start_incoming(uint8_t channel)
 {
-    /* Multifd doesn't start unless all channels are established */
-    if (migrate_multifd()) {
-        return migration_has_all_channels();
-    }
+    bool ret = false;
+
+    if (channel != CH_POSTCOPY) {
+        MigrationIncomingState *mis = migration_incoming_get_current();
+        ret = mis->from_src_file ? true : false;
 
-    /* Preempt channel only starts when the main channel is created */
-    if (migrate_postcopy_preempt()) {
-        return main_channel;
+        if (ret && migrate_multifd()) {
+            ret = multifd_recv_all_channels_created();
+        }
     }
 
-    /*
-     * 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);
-    return true;
+    return ret;
 }
 
 void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
@@ -956,13 +966,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_DEFAULT;
     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_should_start_incoming(channel)) {
+        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
@@ -973,42 +982,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_DEFAULT;
+            } 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_DEFAULT;
+            } else {
+                error_report("%s: could not identify channel, unknown 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) { // && migrate_postcopy_preempt()
+        channel = CH_POSTCOPY;
     }
 
     if (multifd_recv_setup(errp) != 0) {
         return;
     }
 
-    if (default_channel) {
+    if (channel == CH_DEFAULT) {
         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 (migration_should_start_incoming(channel)) {
         /* If it's a recovery, we're done */
         if (postcopy_try_recover()) {
             return;
@@ -1025,21 +1050,22 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
  */
 bool migration_has_all_channels(void)
 {
+    bool ret = false;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
     if (!mis->from_src_file) {
-        return false;
+        return ret;
     }
 
     if (migrate_multifd()) {
-        return multifd_recv_all_channels_created();
+        ret = multifd_recv_all_channels_created();
     }
 
-    if (migrate_postcopy_preempt()) {
-        return mis->postcopy_qemufile_dst != NULL;
+    if (ret && migrate_postcopy_preempt()) {
+        ret = mis->postcopy_qemufile_dst != NULL;
     }
 
-    return true;
+    return ret;
 }
 
 int migrate_send_rp_switchover_ack(MigrationIncomingState *mis)
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/options.c b/migration/options.c
index b8d5300326..8c878dea49 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -479,11 +479,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 f2326788de..bdba7abe73 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1295,7 +1295,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) {
@@ -1969,7 +1969,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
         }
     }
 
-    if (migrate_multifd()) {
+    if (migrate_multifd() && !migration_in_postcopy()) {
         RAMBlock *block = pss->block;
         return ram_save_multifd_page(block, offset);
     }
-- 
2.48.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd
  2025-01-27 12:08 [PATCH v4 0/4] Allow to enable multifd and postcopy migration together Prasad Pandit
                   ` (2 preceding siblings ...)
  2025-01-27 12:08 ` [PATCH v4 3/4] migration: enable multifd and postcopy together Prasad Pandit
@ 2025-01-27 12:08 ` Prasad Pandit
  2025-01-27 21:13   ` Fabiano Rosas
  3 siblings, 1 reply; 11+ messages in thread
From: Prasad Pandit @ 2025-01-27 12:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: peterx, farosas, berrange, Prasad Pandit

From: Prasad Pandit <pjp@fedoraproject.org>

Add new postcopy tests to run postcopy migration with
multifd channels enabled. Add a boolean fields 'multifd'
and 'postcopy_ram' to the MigrateCommon structure.
It helps to enable multifd channels and postcopy-ram
during migration tests.

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

v3: Add more qtests to cover precopy with 'postcopy-ram' attribute
    and postcopy with multifd channels enabled.
- https://lore.kernel.org/qemu-devel/20250121131032.1611245-1-ppandit@redhat.com/T/#t

diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
index d78f1f11f1..3252ba2f73 100644
--- a/tests/qtest/migration/compression-tests.c
+++ b/tests/qtest/migration/compression-tests.c
@@ -39,6 +39,17 @@ static void test_multifd_tcp_zstd(void)
     };
     test_precopy_common(&args);
 }
+
+static void test_multifd_postcopy_tcp_zstd(void)
+{
+    MigrateCommon args = {
+        .postcopy_ram = true,
+        .listen_uri = "defer",
+        .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
+    };
+
+    test_precopy_common(&args);
+}
 #endif /* CONFIG_ZSTD */
 
 #ifdef CONFIG_QATZIP
@@ -158,6 +169,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 4550cda129..00776f858c 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -427,6 +427,14 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
         migrate_set_capability(to, "postcopy-preempt", true);
     }
 
+    if (args->multifd) {
+        migrate_set_capability(from, "multifd", true);
+        migrate_set_capability(to, "multifd", true);
+
+        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);
@@ -691,6 +699,11 @@ void test_precopy_common(MigrateCommon *args)
         return;
     }
 
+    if (args->postcopy_ram) {
+        migrate_set_capability(from, "postcopy-ram", true);
+        migrate_set_capability(to, "postcopy-ram", true);
+    }
+
     if (args->start_hook) {
         data_hook = args->start_hook(from, to);
     }
diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
index 7991ee56b6..214288ca42 100644
--- a/tests/qtest/migration/framework.h
+++ b/tests/qtest/migration/framework.h
@@ -193,7 +193,11 @@ typedef struct {
      */
     bool live;
 
+    /* set multifd on */
+    bool multifd;
+
     /* Postcopy specific fields */
+    bool postcopy_ram;
     void *postcopy_data;
     bool postcopy_preempt;
     PostcopyRecoveryFailStage postcopy_recovery_fail_stage;
diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
index daf7449f2c..212a5ea600 100644
--- a/tests/qtest/migration/postcopy-tests.c
+++ b/tests/qtest/migration/postcopy-tests.c
@@ -79,6 +79,25 @@ static void test_postcopy_preempt_recovery(void)
     test_postcopy_recovery_common(&args);
 }
 
+static void test_multifd_postcopy(void)
+{
+    MigrateCommon args = {
+        .multifd = true,
+    };
+
+    test_postcopy_common(&args);
+}
+
+static void test_multifd_postcopy_preempt(void)
+{
+    MigrateCommon args = {
+        .multifd = true,
+        .postcopy_preempt = true,
+    };
+
+    test_postcopy_common(&args);
+}
+
 void migration_test_add_postcopy(MigrationTestEnv *env)
 {
     if (env->has_uffd) {
@@ -98,6 +117,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 23599b29ee..b1a4e7bbb1 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)
 {
@@ -472,6 +473,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);
 
@@ -513,6 +519,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);
@@ -536,6 +546,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,
@@ -999,6 +1016,8 @@ void migration_test_add_precopy(MigrationTestEnv *env)
                        test_multifd_tcp_no_zero_page);
     migration_test_add("/migration/multifd/tcp/plain/cancel",
                        test_multifd_tcp_cancel);
+    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 5704a1f992..094dc1d814 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,
+        .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,
+        .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);
@@ -649,6 +671,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 = {
+        .multifd = true,
+        .listen_uri = "defer",
+        .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)
 {
@@ -743,6 +777,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",
@@ -776,6 +814,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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/4] migration: refactor ram_save_target_page functions
  2025-01-27 12:08 ` [PATCH v4 2/4] migration: refactor ram_save_target_page functions Prasad Pandit
@ 2025-01-27 18:46   ` Fabiano Rosas
  0 siblings, 0 replies; 11+ messages in thread
From: Fabiano Rosas @ 2025-01-27 18:46 UTC (permalink / raw)
  To: Prasad Pandit, qemu-devel; +Cc: peterx, berrange, Prasad Pandit

Prasad Pandit <ppandit@redhat.com> writes:

> From: Prasad Pandit <pjp@fedoraproject.org>
>
> Refactor ram_save_target_page legacy and multifd
> functions into one. Other than simplifying it,
> it frees 'migration_ops' object from usage, so it
> is expunged.
>
> Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd
  2025-01-27 12:08 ` [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd Prasad Pandit
@ 2025-01-27 21:13   ` Fabiano Rosas
  2025-01-28  5:30     ` Prasad Pandit
  0 siblings, 1 reply; 11+ messages in thread
From: Fabiano Rosas @ 2025-01-27 21:13 UTC (permalink / raw)
  To: Prasad Pandit, qemu-devel; +Cc: peterx, berrange, Prasad Pandit

Prasad Pandit <ppandit@redhat.com> writes:

> From: Prasad Pandit <pjp@fedoraproject.org>
>
> Add new postcopy tests to run postcopy migration with
> multifd channels enabled. Add a boolean fields 'multifd'
> and 'postcopy_ram' to the MigrateCommon structure.
> It helps to enable multifd channels and postcopy-ram
> during migration tests.
>
> Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
> ---
>  tests/qtest/migration/compression-tests.c | 13 ++++++++
>  tests/qtest/migration/framework.c         | 13 ++++++++
>  tests/qtest/migration/framework.h         |  4 +++
>  tests/qtest/migration/postcopy-tests.c    | 23 +++++++++++++
>  tests/qtest/migration/precopy-tests.c     | 19 +++++++++++
>  tests/qtest/migration/tls-tests.c         | 40 +++++++++++++++++++++++
>  6 files changed, 112 insertions(+)
>
> v3: Add more qtests to cover precopy with 'postcopy-ram' attribute
>     and postcopy with multifd channels enabled.
> - https://lore.kernel.org/qemu-devel/20250121131032.1611245-1-ppandit@redhat.com/T/#t
>
> diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
> index d78f1f11f1..3252ba2f73 100644
> --- a/tests/qtest/migration/compression-tests.c
> +++ b/tests/qtest/migration/compression-tests.c
> @@ -39,6 +39,17 @@ static void test_multifd_tcp_zstd(void)
>      };
>      test_precopy_common(&args);
>  }
> +
> +static void test_multifd_postcopy_tcp_zstd(void)
> +{
> +    MigrateCommon args = {
> +        .postcopy_ram = true,
> +        .listen_uri = "defer",
> +        .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
> +    };
> +
> +    test_precopy_common(&args);
> +}
>  #endif /* CONFIG_ZSTD */
>  
>  #ifdef CONFIG_QATZIP
> @@ -158,6 +169,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 4550cda129..00776f858c 100644
> --- a/tests/qtest/migration/framework.c
> +++ b/tests/qtest/migration/framework.c
> @@ -427,6 +427,14 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>          migrate_set_capability(to, "postcopy-preempt", true);
>      }
>  
> +    if (args->multifd) {
> +        migrate_set_capability(from, "multifd", true);
> +        migrate_set_capability(to, "multifd", true);

This is slightly backwards because currently that's what the hooks are
for. I don't see the need for separate flags for multifd and
postcopy. This also makes the code less maintainable because it creates
two different ways of doing the same thing (hooks vs. args).

I suggest to keep with the hooks. Alternatively, we could add a more
generic args->caps and have every test set the capabilities it wants and
the _common code to iterate over those and set them to true. Something
like this perhaps:

    if (migrate_start(&from, &to, args->listen_uri, &args->start)) {
        return;
    }

    for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
        if (args->caps[i]) {
            migrate_set_capability(from, MigrationCapability_str(args->caps[i]), true);
            migrate_set_capability(to, MigrationCapability_str(args->caps[i]), true);
        }
    }

    if (args->start_hook) {
        data_hook = args->start_hook(from, to);
    }

We could also set the number of channels as a default value. The tests
could overwrite it from the hook if needed.

> +
> +        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);
> @@ -691,6 +699,11 @@ void test_precopy_common(MigrateCommon *args)
>          return;
>      }
>  
> +    if (args->postcopy_ram) {
> +        migrate_set_capability(from, "postcopy-ram", true);
> +        migrate_set_capability(to, "postcopy-ram", true);
> +    }
> +
>      if (args->start_hook) {
>          data_hook = args->start_hook(from, to);
>      }
> diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
> index 7991ee56b6..214288ca42 100644
> --- a/tests/qtest/migration/framework.h
> +++ b/tests/qtest/migration/framework.h
> @@ -193,7 +193,11 @@ typedef struct {
>       */
>      bool live;
>  
> +    /* set multifd on */
> +    bool multifd;
> +
>      /* Postcopy specific fields */
> +    bool postcopy_ram;
>      void *postcopy_data;
>      bool postcopy_preempt;
>      PostcopyRecoveryFailStage postcopy_recovery_fail_stage;
> diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
> index daf7449f2c..212a5ea600 100644
> --- a/tests/qtest/migration/postcopy-tests.c
> +++ b/tests/qtest/migration/postcopy-tests.c
> @@ -79,6 +79,25 @@ static void test_postcopy_preempt_recovery(void)
>      test_postcopy_recovery_common(&args);
>  }
>  
> +static void test_multifd_postcopy(void)
> +{
> +    MigrateCommon args = {
> +        .multifd = true,
> +    };
> +
> +    test_postcopy_common(&args);
> +}
> +
> +static void test_multifd_postcopy_preempt(void)
> +{
> +    MigrateCommon args = {
> +        .multifd = true,
> +        .postcopy_preempt = true,
> +    };
> +
> +    test_postcopy_common(&args);
> +}
> +
>  void migration_test_add_postcopy(MigrationTestEnv *env)
>  {
>      if (env->has_uffd) {
> @@ -98,6 +117,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 23599b29ee..b1a4e7bbb1 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)
>  {
> @@ -472,6 +473,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);
>  
> @@ -513,6 +519,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);
> @@ -536,6 +546,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,
> @@ -999,6 +1016,8 @@ void migration_test_add_precopy(MigrationTestEnv *env)
>                         test_multifd_tcp_no_zero_page);
>      migration_test_add("/migration/multifd/tcp/plain/cancel",
>                         test_multifd_tcp_cancel);
> +    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 5704a1f992..094dc1d814 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,
> +        .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,
> +        .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);
> @@ -649,6 +671,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 = {
> +        .multifd = true,
> +        .listen_uri = "defer",
> +        .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)
>  {
> @@ -743,6 +777,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",
> @@ -776,6 +814,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);


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd
  2025-01-27 21:13   ` Fabiano Rosas
@ 2025-01-28  5:30     ` Prasad Pandit
  2025-01-28 11:17       ` Prasad Pandit
  0 siblings, 1 reply; 11+ messages in thread
From: Prasad Pandit @ 2025-01-28  5:30 UTC (permalink / raw)
  To: Fabiano Rosas; +Cc: qemu-devel, peterx, berrange, Prasad Pandit

Hello Fabiano,

On Tue, 28 Jan 2025 at 02:43, Fabiano Rosas <farosas@suse.de> wrote:
> > +    if (args->multifd) {
> > +        migrate_set_capability(from, "multifd", true);
> > +        migrate_set_capability(to, "multifd", true);
>
> This is slightly backwards because currently that's what the hooks are
> for. I don't see the need for separate flags for multifd and
> postcopy. This also makes the code less maintainable because it creates
> two different ways of doing the same thing (hooks vs. args).

* I did look at the hook functions. In 'postcopy-tests.c' hook
function is not used. Fields are set in the 'MigrateCommon args'
object, which gets passed to migrate_postcopy_prepare() to
enable/disable capability.

* If we look at precopy-tests.c/tls-tests.c/compression-tests.c, the
same hook function 'migrate_hook_start_precopy_tcp_multifd_common'
gets called from them. Setting a capability therein shall affect all
tests which call that function. Defining a new hook function to set a
single field/capability and then calling the existing common hook
function for other attributes is doable, but doing so for multiple
qtests would only increase the number of hook functions, creating
clutter and confusion over time. (thinking aloud)

> Alternatively, we could add a more generic args->caps and have every test set the capabilities it wants and
> the _common code to iterate over those and set them to true. Something
> like this perhaps:
>
>     for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
>         if (args->caps[i]) {
>             migrate_set_capability(from, MigrationCapability_str(args->caps[i]), true);
>             migrate_set_capability(to, MigrationCapability_str(args->caps[i]), true);
>         }
>     }
>
> We could also set the number of channels as a default value. The tests
> could overwrite it from the hook if needed.

* Yes, this seems like a better option, I'll give it a try.  But
should we include it in this patch series OR make it a separate one?
I'm leaning towards the latter, because it is a generic change
affecting all tests, it's not specific to this series.

Thank you.
---
  - Prasad



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd
  2025-01-28  5:30     ` Prasad Pandit
@ 2025-01-28 11:17       ` Prasad Pandit
  2025-01-28 13:50         ` Fabiano Rosas
  0 siblings, 1 reply; 11+ messages in thread
From: Prasad Pandit @ 2025-01-28 11:17 UTC (permalink / raw)
  To: Fabiano Rosas; +Cc: qemu-devel, peterx, berrange, Prasad Pandit

On Tue, 28 Jan 2025 at 11:00, Prasad Pandit <ppandit@redhat.com> wrote:
> >     for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> >         if (args->caps[i]) {
> >             migrate_set_capability(from, MigrationCapability_str(args->caps[i]), true);
> >             migrate_set_capability(to, MigrationCapability_str(args->caps[i]), true);
> >         }
> >     }
> >
> > We could also set the number of channels as a default value. The tests
> > could overwrite it from the hook if needed.
>
> * Yes, this seems like a better option, I'll give it a try.

Please see -> https://notebin.de/?317b9fc90a9a910d#dGKqq4r5pyMYU5SXYLFhd8wrzKRCxCcokTkTRBCUK7w

@Fabiano: does this look okay? If it is, I'll further remove
corresponding boolean fields from MigrateCommon struct etc.

Thank you.
---
  - Prasad



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd
  2025-01-28 11:17       ` Prasad Pandit
@ 2025-01-28 13:50         ` Fabiano Rosas
  2025-01-28 16:45           ` Prasad Pandit
  0 siblings, 1 reply; 11+ messages in thread
From: Fabiano Rosas @ 2025-01-28 13:50 UTC (permalink / raw)
  To: Prasad Pandit; +Cc: qemu-devel, peterx, berrange, Prasad Pandit

Prasad Pandit <ppandit@redhat.com> writes:

> On Tue, 28 Jan 2025 at 11:00, Prasad Pandit <ppandit@redhat.com> wrote:
>> >     for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
>> >         if (args->caps[i]) {
>> >             migrate_set_capability(from, MigrationCapability_str(args->caps[i]), true);
>> >             migrate_set_capability(to, MigrationCapability_str(args->caps[i]), true);
>> >         }
>> >     }
>> >
>> > We could also set the number of channels as a default value. The tests
>> > could overwrite it from the hook if needed.
>>
>> * Yes, this seems like a better option, I'll give it a try.
>
> Please see -> https://notebin.de/?317b9fc90a9a910d#dGKqq4r5pyMYU5SXYLFhd8wrzKRCxCcokTkTRBCUK7w
>
> @Fabiano: does this look okay? If it is, I'll further remove
> corresponding boolean fields from MigrateCommon struct etc.

You could include qapi-types-migration.h and use the actual enum, that
avoids the burden of having to keep the tests in sync with the code.

(I don't think keeping the caps in sync with the current-version tests
would break the compat tests, but please consider that as well)

And a generic helper that calls migrate_set_capability() for any
capabilites set. That solves the capabilities issue for all tests. We
can then move some default parameters setting into that function and
that should already reduce the number of hooks needed.

You can include it in this series or send a separate one, whatever is
easier for you. But we need to base this one on top of it eventually, I
would just send everything at once.

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd
  2025-01-28 13:50         ` Fabiano Rosas
@ 2025-01-28 16:45           ` Prasad Pandit
  0 siblings, 0 replies; 11+ messages in thread
From: Prasad Pandit @ 2025-01-28 16:45 UTC (permalink / raw)
  To: Fabiano Rosas; +Cc: qemu-devel, peterx, berrange, Prasad Pandit

On Tue, 28 Jan 2025 at 19:20, Fabiano Rosas <farosas@suse.de> wrote:
> You could include qapi-types-migration.h and use the actual enum, that
> avoids the burden of having to keep the tests in sync with the code.
>
> (I don't think keeping the caps in sync with the current-version tests
> would break the compat tests, but please consider that as well)
>
> And a generic helper that calls migrate_set_capability() for any
> capabilites set. That solves the capabilities issue for all tests. We
> can then move some default parameters setting into that function and
> that should already reduce the number of hooks needed.

* Okay, will check.

> You can include it in this series or send a separate one, whatever is
> easier for you. But we need to base this one on top of it eventually, I
> would just send everything at once.

* Okay, will send it as part of this series then.

Thank you.
---
  - Prasad



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-01-28 16:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-27 12:08 [PATCH v4 0/4] Allow to enable multifd and postcopy migration together Prasad Pandit
2025-01-27 12:08 ` [PATCH v4 1/4] migration/multifd: move macros to multifd header Prasad Pandit
2025-01-27 12:08 ` [PATCH v4 2/4] migration: refactor ram_save_target_page functions Prasad Pandit
2025-01-27 18:46   ` Fabiano Rosas
2025-01-27 12:08 ` [PATCH v4 3/4] migration: enable multifd and postcopy together Prasad Pandit
2025-01-27 12:08 ` [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd Prasad Pandit
2025-01-27 21:13   ` Fabiano Rosas
2025-01-28  5:30     ` Prasad Pandit
2025-01-28 11:17       ` Prasad Pandit
2025-01-28 13:50         ` Fabiano Rosas
2025-01-28 16:45           ` Prasad Pandit

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.