[PATCH v2 0/3] migration: Don't serialize devices in qemu_savevm_state_iterate()

[PATCH v2 0/3] migration: Don't serialize devices in qemu_savevm_state_iterate()

[Avihai Horon]

Hi,

This small series is v2 of the single patch I previously sent [1].

It removes device serialization in qemu_savevm_state_iterate() and does
some VFIO migration touch ups. More info provided in the commit
messages.

Thanks.

Changes from V1 -> V2:
* Remove device serialization in qemu_savevm_state_iterate() always,
  regardless of switchover-ack.
* Refactor vfio_save_iterate() return value.
* Add a note about migration rate limiting in vfio_save_iterate().

[1] https://lore.kernel.org/qemu-devel/20240222155627.14563-1-avihaih@nvidia.com/

Avihai Horon (3):
  migration: Don't serialize devices in qemu_savevm_state_iterate()
  vfio/migration: Refactor vfio_save_state() return value
  vfio/migration: Add a note about migration rate limiting

 hw/vfio/migration.c | 12 +++++++-----
 migration/savevm.c  | 15 ++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.26.3

[PATCH v2 1/3] migration: Don't serialize devices in qemu_savevm_state_iterate()

[Avihai Horon]

Commit 90697be8896c ("live migration: Serialize vmstate saving in stage
2") introduced device serialization in qemu_savevm_state_iterate(). The
rationale behind it was to first complete migration of slower changing
block devices and only then migrate the RAM, to avoid sending fast
changing RAM pages over and over.

This commit was added a long time ago, and while it was useful back
then, it is not the case anymore:
1. Block migration is deprecated, see commit 66db46ca83b8 ("migration:
   Deprecate block migration").
2. Today there are other iterative devices besides RAM and block, such
   as VFIO, which are registered for migration after RAM. With current
   serialization behavior, a fast changing device can block other
   devices from sending their data, which may prevent migration from
   converging in some cases.

The issue described in item 2 was observed in several VFIO migration
scenarios with switchover-ack capability enabled, where some workload on
the VM prevented RAM from ever reaching a hard zero, thus blocking VFIO
initial pre-copy data from being sent. Hence, destination could not ack
switchover and migration could not converge.

Fix that by not serializing iterative devices in
qemu_savevm_state_iterate().

Note that this still doesn't fully prevent device starvation. As
correctly pointed out by Peter [1], a fast changing device might
constantly consume all allocated bandwidth and block the following
devices. However, this scenario is more likely to happen only if
max-bandwidth is low.

[1] https://lore.kernel.org/qemu-devel/Zd6iw9dBhW6wKNxx@x1n/

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/savevm.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index d612c8a9020..d76d82e7596 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1389,7 +1389,8 @@ int qemu_savevm_state_resume_prepare(MigrationState *s)
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
 {
     SaveStateEntry *se;
-    int ret = 1;
+    bool all_finished = true;
+    int ret;
 
     trace_savevm_state_iterate();
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
@@ -1430,16 +1431,12 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
                          "%d(%s): %d",
                          se->section_id, se->idstr, ret);
             qemu_file_set_error(f, ret);
-        }
-        if (ret <= 0) {
-            /* Do not proceed to the next vmstate before this one reported
-               completion of the current stage. This serializes the migration
-               and reduces the probability that a faster changing state is
-               synchronized over and over again. */
-            break;
+            return ret;
+        } else if (!ret) {
+            all_finished = false;
         }
     }
-    return ret;
+    return all_finished;
 }
 
 static bool should_send_vmdesc(void)
-- 
2.26.3

[PATCH v2 2/3] vfio/migration: Refactor vfio_save_state() return value

[Avihai Horon]

Currently, vfio_save_state() returns 1 regardless of whether there is
more data to send or not. This was done to prevent a fast changing VFIO
device from potentially blocking other devices from sending their data,
as qemu_savevm_state_iterate() serialized devices.

Now that qemu_savevm_state_iterate() no longer serializes devices, there
is no need for that.

Refactor vfio_save_state() to return 0 if more data is available and 1
if no more data is available.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 hw/vfio/migration.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 50140eda872..0af783a5892 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -529,11 +529,7 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
     trace_vfio_save_iterate(vbasedev->name, migration->precopy_init_size,
                             migration->precopy_dirty_size);
 
-    /*
-     * A VFIO device's pre-copy dirty_bytes is not guaranteed to reach zero.
-     * Return 1 so following handlers will not be potentially blocked.
-     */
-    return 1;
+    return !migration->precopy_init_size && !migration->precopy_dirty_size;
 }
 
 static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
-- 
2.26.3

[PATCH v2 3/3] vfio/migration: Add a note about migration rate limiting

[Avihai Horon]

VFIO migration buffer size is currently limited to 1MB. Therefore, there
is no need to check if migration rate exceeded, as in the worst case it
will exceed by only 1MB.

However, if the buffer size is later changed to a bigger value,
vfio_save_iterate() should enforce migration rate (similar to migration
RAM code).

Add a note about this in vfio_save_iterate() to serve as a reminder.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 hw/vfio/migration.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 0af783a5892..f82dcabc49a 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -505,6 +505,12 @@ static bool vfio_is_active_iterate(void *opaque)
     return vfio_device_state_is_precopy(vbasedev);
 }
 
+/*
+ * Note about migration rate limiting: VFIO migration buffer size is currently
+ * limited to 1MB, so there is no need to check if migration rate exceeded (as
+ * in the worst case it will exceed by 1MB). However, if the buffer size is
+ * later changed to a bigger value, migration rate should be enforced here.
+ */
 static int vfio_save_iterate(QEMUFile *f, void *opaque)
 {
     VFIODevice *vbasedev = opaque;
-- 
2.26.3

Re: [PATCH v2 0/3] migration: Don't serialize devices in qemu_savevm_state_iterate()

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> Hi,
>
> This small series is v2 of the single patch I previously sent [1].
>
> It removes device serialization in qemu_savevm_state_iterate() and does
> some VFIO migration touch ups. More info provided in the commit
> messages.
>
> Thanks.
>
> Changes from V1 -> V2:
> * Remove device serialization in qemu_savevm_state_iterate() always,
>   regardless of switchover-ack.
> * Refactor vfio_save_iterate() return value.
> * Add a note about migration rate limiting in vfio_save_iterate().
>
> [1] https://lore.kernel.org/qemu-devel/20240222155627.14563-1-avihaih@nvidia.com/
>
> Avihai Horon (3):
>   migration: Don't serialize devices in qemu_savevm_state_iterate()
>   vfio/migration: Refactor vfio_save_state() return value
>   vfio/migration: Add a note about migration rate limiting
>
>  hw/vfio/migration.c | 12 +++++++-----
>  migration/savevm.c  | 15 ++++++---------
>  2 files changed, 13 insertions(+), 14 deletions(-)

Series:

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

Re: [PATCH v2 0/3] migration: Don't serialize devices in qemu_savevm_state_iterate()

[Peter Xu]

On Mon, Mar 04, 2024 at 12:53:36PM +0200, Avihai Horon wrote:
> Hi,
> 
> This small series is v2 of the single patch I previously sent [1].
> 
> It removes device serialization in qemu_savevm_state_iterate() and does
> some VFIO migration touch ups. More info provided in the commit
> messages.
> 
> Thanks.
> 
> Changes from V1 -> V2:
> * Remove device serialization in qemu_savevm_state_iterate() always,
>   regardless of switchover-ack.
> * Refactor vfio_save_iterate() return value.
> * Add a note about migration rate limiting in vfio_save_iterate().
> 
> [1] https://lore.kernel.org/qemu-devel/20240222155627.14563-1-avihaih@nvidia.com/

Queued, thanks.

-- 
Peter Xu