[PATCH v3 0/8] Migration: Make misc.h helpers available for whole VM lifecycle

[PATCH v3 0/8] Migration: Make misc.h helpers available for whole VM lifecycle

[Peter Xu]

Based-on: <20241024165627.1372621-1-peterx@redhat.com>
CI:       https://gitlab.com/peterx/qemu/-/pipelines/1511349805

This is a follow up of below patch from Avihai as a replacement:

https://lore.kernel.org/qemu-devel/20241020130108.27148-3-avihaih@nvidia.com/

This is v3 of the series, and it happened again: changelog doesn't make
sense to compare to v2 because it's rewritten.  Meanwhile, now this series
is based on the other TYPE_SINGLETON series I posted just now:

https://lore.kernel.org/r/20241024165627.1372621-1-peterx@redhat.com

It turns out I found more things to cleanup, as the versions spin up.

Patch 1:     I found that object_ref() in migration thread is actually also
             racy..  this should fix it.

Patch 2-7:   It turns out I decided to clean things up first, then it'll
             make my last patch easier on adding the mutex protection for
             the current_migration reference

Patch 8:     The fix for NULL-dereference / race for the exported
             functions.  VFIO can hit it only because VFIO's specialty
             in using migration helpers in vmstate handlers, I guess.  I
             found most functions always safe because even if they're used
             outside migration/ they're most likely still invoked with
             migration thread context.  So I found only two functions that
             really need protections, exactly what VFIO is using.

Comments welcomed, thanks.

Peter Xu (8):
  migration: Take migration object refcount earlier for threads
  migration: Unexport dirty_bitmap_mig_init()
  migration: Unexport ram_mig_init()
  migration: Drop migration_is_setup_or_active()
  migration: Drop migration_is_idle()
  migration: Drop migration_is_device()
  migration: Unexport migration_is_active()
  migration: Protect updates to current_migration with a mutex

 include/migration/misc.h |  11 +---
 migration/migration.h    |   8 +++
 migration/ram.h          |   1 +
 hw/vfio/common.c         |   6 +-
 hw/virtio/virtio-mem.c   |   2 +-
 migration/migration.c    | 118 +++++++++++++++++++--------------------
 migration/ram.c          |   7 +--
 net/vhost-vdpa.c         |   3 +-
 system/dirtylimit.c      |   3 +-
 system/qdev-monitor.c    |   4 +-
 10 files changed, 81 insertions(+), 82 deletions(-)

-- 
2.45.0

[PATCH v3 1/8] migration: Take migration object refcount earlier for threads

[Peter Xu]

Both migration thread or background snapshot thread will take a refcount of
the migration object at the entrace of the thread function.

That makes sense, because it protects the object from being freed by the
main thread in migration_shutdown() later, but it might still race with it
if the thread is scheduled too late.  Consider the case right after
pthread_create() happened, VM shuts down with the object released, but
right after that the migration thread finally got created, referencing
MigrationState* in the opaque pointer which is already freed.

The only 100% safe way to make sure it won't get freed is taking the
refcount right before the thread is created, meanwhile when BQL is held.

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

diff --git a/migration/migration.c b/migration/migration.c
index 74812ca785..e82ffa8cf3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3491,7 +3491,6 @@ static void *migration_thread(void *opaque)
 
     rcu_register_thread();
 
-    object_ref(OBJECT(s));
     update_iteration_initial_status(s);
 
     if (!multifd_send_setup()) {
@@ -3629,7 +3628,6 @@ static void *bg_migration_thread(void *opaque)
     int ret;
 
     rcu_register_thread();
-    object_ref(OBJECT(s));
 
     migration_rate_set(RATE_LIMIT_DISABLED);
 
@@ -3841,6 +3839,14 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         }
     }
 
+    /*
+     * Take a refcount to make sure the migration object won't get freed by
+     * the main thread already in migration_shutdown().
+     *
+     * The refcount will be released at the end of the thread function.
+     */
+    object_ref(OBJECT(s));
+
     if (migrate_background_snapshot()) {
         qemu_thread_create(&s->thread, MIGRATION_THREAD_SNAPSHOT,
                 bg_migration_thread, s, QEMU_THREAD_JOINABLE);
-- 
2.45.0

[PATCH v3 2/8] migration: Unexport dirty_bitmap_mig_init()

[Peter Xu]

It's only used within migration/, so it shouldn't be exported.

Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 3 ---
 migration/migration.h    | 4 ++++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index bfadc5613b..df57be6b5e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -108,7 +108,4 @@ bool migration_incoming_postcopy_advised(void);
 /* True if background snapshot is active */
 bool migration_in_bg_snapshot(void);
 
-/* migration/block-dirty-bitmap.c */
-void dirty_bitmap_mig_init(void);
-
 #endif
diff --git a/migration/migration.h b/migration/migration.h
index 7dc59c5e8d..0956e9274b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -552,4 +552,8 @@ int migration_rp_wait(MigrationState *s);
 void migration_rp_kick(MigrationState *s);
 
 void migration_bitmap_sync_precopy(bool last_stage);
+
+/* migration/block-dirty-bitmap.c */
+void dirty_bitmap_mig_init(void);
+
 #endif
-- 
2.45.0

[PATCH v3 3/8] migration: Unexport ram_mig_init()

[Peter Xu]

It's only used within migration/.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 1 -
 migration/ram.h          | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index df57be6b5e..e8490e3af5 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -39,7 +39,6 @@ void precopy_add_notifier(NotifierWithReturn *n);
 void precopy_remove_notifier(NotifierWithReturn *n);
 int precopy_notify(PrecopyNotifyReason reason, Error **errp);
 
-void ram_mig_init(void);
 void qemu_guest_free_page_hint(void *addr, size_t len);
 bool migrate_ram_is_ignored(RAMBlock *block);
 
diff --git a/migration/ram.h b/migration/ram.h
index bc0318b834..0d1981f888 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -44,6 +44,7 @@ extern XBZRLECacheStats xbzrle_counters;
     INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
+void ram_mig_init(void);
 int xbzrle_cache_resize(uint64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
-- 
2.45.0

[PATCH v3 4/8] migration: Drop migration_is_setup_or_active()

[Peter Xu]

This helper is mostly the same as migration_is_running(), except that one
has COLO reported as true, the other has CANCELLING reported as true.

Per my past years experience on the state changes, none of them should
matter.

To make it slightly safer, report both COLO || CANCELLING to be true in
migration_is_running(), then drop the other one.  We kept the 1st only
because the name is simpler, and clear enough.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  4 ++--
 hw/vfio/common.c         |  2 +-
 migration/migration.c    | 35 +++--------------------------------
 migration/ram.c          |  5 ++---
 net/vhost-vdpa.c         |  3 +--
 5 files changed, 9 insertions(+), 40 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index e8490e3af5..86ef160f19 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -52,11 +52,12 @@ void dump_vmstate_json_to_file(FILE *out_fp);
 /* migration/migration.c */
 void migration_object_init(void);
 void migration_shutdown(void);
+
 bool migration_is_idle(void);
 bool migration_is_active(void);
 bool migration_is_device(void);
+bool migration_is_running(void);
 bool migration_thread_is_self(void);
-bool migration_is_setup_or_active(void);
 
 typedef enum MigrationEventType {
     MIG_EVENT_PRECOPY_SETUP,
@@ -95,7 +96,6 @@ void migration_add_notifier_mode(NotifierWithReturn *notify,
                                  MigrationNotifyFunc func, MigMode mode);
 
 void migration_remove_notifier(NotifierWithReturn *notify);
-bool migration_is_running(void);
 void migration_file_set_error(int ret, Error *err);
 
 /* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 36d0cf6585..dcef44fe55 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -149,7 +149,7 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
 
 static void vfio_set_migration_error(int ret)
 {
-    if (migration_is_setup_or_active()) {
+    if (migration_is_running()) {
         migration_file_set_error(ret, NULL);
     }
 }
diff --git a/migration/migration.c b/migration/migration.c
index e82ffa8cf3..3365195def 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1116,33 +1116,6 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
     migrate_send_rp_message(mis, MIG_RP_MSG_RESUME_ACK, sizeof(buf), &buf);
 }
 
-/*
- * Return true if we're already in the middle of a migration
- * (i.e. any of the active or setup states)
- */
-bool migration_is_setup_or_active(void)
-{
-    MigrationState *s = current_migration;
-
-    switch (s->state) {
-    case MIGRATION_STATUS_ACTIVE:
-    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-    case MIGRATION_STATUS_POSTCOPY_PAUSED:
-    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
-    case MIGRATION_STATUS_POSTCOPY_RECOVER:
-    case MIGRATION_STATUS_SETUP:
-    case MIGRATION_STATUS_PRE_SWITCHOVER:
-    case MIGRATION_STATUS_DEVICE:
-    case MIGRATION_STATUS_WAIT_UNPLUG:
-    case MIGRATION_STATUS_COLO:
-        return true;
-
-    default:
-        return false;
-
-    }
-}
-
 bool migration_is_running(void)
 {
     MigrationState *s = current_migration;
@@ -1158,11 +1131,10 @@ bool migration_is_running(void)
     case MIGRATION_STATUS_DEVICE:
     case MIGRATION_STATUS_WAIT_UNPLUG:
     case MIGRATION_STATUS_CANCELLING:
+    case MIGRATION_STATUS_COLO:
         return true;
-
     default:
         return false;
-
     }
 }
 
@@ -1661,8 +1633,7 @@ bool migration_incoming_postcopy_advised(void)
 
 bool migration_in_bg_snapshot(void)
 {
-    return migrate_background_snapshot() &&
-           migration_is_setup_or_active();
+    return migrate_background_snapshot() && migration_is_running();
 }
 
 bool migration_is_idle(void)
@@ -2335,7 +2306,7 @@ static void *source_return_path_thread(void *opaque)
     trace_source_return_path_thread_entry();
     rcu_register_thread();
 
-    while (migration_is_setup_or_active()) {
+    while (migration_is_running()) {
         trace_source_return_path_thread_loop_top();
 
         header_type = qemu_get_be16(rp);
diff --git a/migration/ram.c b/migration/ram.c
index d284f63854..5646a0b882 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2860,7 +2860,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
     size_t used_len, start, npages;
 
     /* This function is currently expected to be used during live migration */
-    if (!migration_is_setup_or_active()) {
+    if (!migration_is_running()) {
         return;
     }
 
@@ -3208,8 +3208,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     }
 
 out:
-    if (ret >= 0
-        && migration_is_setup_or_active()) {
+    if (ret >= 0 && migration_is_running()) {
         if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
             !migrate_mapped_ram()) {
             ret = multifd_ram_flush_and_sync();
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 46b02c50be..231b45246c 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -375,8 +375,7 @@ static int vhost_vdpa_net_data_start(NetClientState *nc)
 
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
-    if (s->always_svq ||
-        migration_is_setup_or_active()) {
+    if (s->always_svq || migration_is_running()) {
         v->shadow_vqs_enabled = true;
     } else {
         v->shadow_vqs_enabled = false;
-- 
2.45.0

[PATCH v3 5/8] migration: Drop migration_is_idle()

[Peter Xu]

Now with the current migration_is_running(), it will report exactly the
opposite of what will be reported by migration_is_idle().

Drop migration_is_idle(), instead use "!migration_is_running()" which
should be identical on functionality.

In reality, most of the idle check is inverted, so it's even easier to
write with "migrate_is_running()" check.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  1 -
 hw/virtio/virtio-mem.c   |  2 +-
 migration/migration.c    | 21 +--------------------
 migration/ram.c          |  2 +-
 system/qdev-monitor.c    |  4 ++--
 5 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 86ef160f19..804eb23c06 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
 void migration_object_init(void);
 void migration_shutdown(void);
 
-bool migration_is_idle(void);
 bool migration_is_active(void);
 bool migration_is_device(void);
 bool migration_is_running(void);
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index ae1e81d7ba..80ada89551 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -188,7 +188,7 @@ static bool virtio_mem_is_busy(void)
      * after plugging them) until we're running on the destination (as we didn't
      * migrate these blocks when they were unplugged).
      */
-    return migration_in_incoming_postcopy() || !migration_is_idle();
+    return migration_in_incoming_postcopy() || migration_is_running();
 }
 
 typedef int (*virtio_mem_range_cb)(VirtIOMEM *vmem, void *arg,
diff --git a/migration/migration.c b/migration/migration.c
index 3365195def..f86c709699 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1636,25 +1636,6 @@ bool migration_in_bg_snapshot(void)
     return migrate_background_snapshot() && migration_is_running();
 }
 
-bool migration_is_idle(void)
-{
-    MigrationState *s = current_migration;
-
-    if (!s) {
-        return true;
-    }
-
-    switch (s->state) {
-    case MIGRATION_STATUS_NONE:
-    case MIGRATION_STATUS_CANCELLED:
-    case MIGRATION_STATUS_COMPLETED:
-    case MIGRATION_STATUS_FAILED:
-        return true;
-    default:
-        return false;
-    }
-}
-
 bool migration_is_active(void)
 {
     MigrationState *s = current_migration;
@@ -1733,7 +1714,7 @@ static bool is_busy(Error **reasonp, Error **errp)
     ERRP_GUARD();
 
     /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
-    if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
+    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
         error_propagate_prepend(errp, *reasonp,
                                 "disallowing migration blocker "
                                 "(migration/snapshot in progress) for: ");
diff --git a/migration/ram.c b/migration/ram.c
index 5646a0b882..504b48d584 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4498,7 +4498,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
         return;
     }
 
-    if (!migration_is_idle()) {
+    if (migration_is_running()) {
         /*
          * Precopy code on the source cannot deal with the size of RAM blocks
          * changing at random points in time - especially after sending the
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 1310f35c9f..83fa684475 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -686,7 +686,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
-    if (!migration_is_idle()) {
+    if (migration_is_running()) {
         error_setg(errp, "device_add not allowed while migrating");
         return NULL;
     }
@@ -935,7 +935,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (!migration_is_idle() && !dev->allow_unplug_during_migration) {
+    if (migration_is_running() && !dev->allow_unplug_during_migration) {
         error_setg(errp, "device_del not allowed while migrating");
         return;
     }
-- 
2.45.0

[PATCH v3 6/8] migration: Drop migration_is_device()

[Peter Xu]

This only checks the DEVICE stage, which is a very special stage, and only
checked together with ACTIVE+POSTCOPY_ACTIVE.

It's debatable why this needs a separate helper just to be exported.
Theoretically speaking, DEVICE stage is also part of ACTIVE stage, where
we're reaching the switchover phase but waiting for some external
operation, during which VM stopped but migration is definitely active.

Report DEVICE as active too, then we can drop migration_is_device().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  1 -
 hw/vfio/common.c         |  2 +-
 migration/migration.c    | 19 +++++++++----------
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 804eb23c06..ad1e25826a 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -54,7 +54,6 @@ void migration_object_init(void);
 void migration_shutdown(void);
 
 bool migration_is_active(void);
-bool migration_is_device(void);
 bool migration_is_running(void);
 bool migration_thread_is_self(void);
 
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index dcef44fe55..cc72282c71 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
 {
     VFIODevice *vbasedev;
 
-    if (!migration_is_active() && !migration_is_device()) {
+    if (!migration_is_active()) {
         return false;
     }
 
diff --git a/migration/migration.c b/migration/migration.c
index f86c709699..127b01734d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1638,17 +1638,16 @@ bool migration_in_bg_snapshot(void)
 
 bool migration_is_active(void)
 {
-    MigrationState *s = current_migration;
-
-    return (s->state == MIGRATION_STATUS_ACTIVE ||
-            s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
-}
+    MigrationStatus state = current_migration->state;
 
-bool migration_is_device(void)
-{
-    MigrationState *s = current_migration;
-
-    return s->state == MIGRATION_STATUS_DEVICE;
+    switch (state) {
+    case MIGRATION_STATUS_ACTIVE:
+    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
+    case MIGRATION_STATUS_DEVICE:
+        return true;
+    default:
+        return false;
+    }
 }
 
 bool migration_thread_is_self(void)
-- 
2.45.0

[PATCH v3 7/8] migration: Unexport migration_is_active()

[Peter Xu]

We have two outside users of this API, so it's exported.

Is it really necessary?  Does it matter whether it must be
ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.

The external user is trying to detect whether migration is running or not,
as simple as that.

To make the migration_is*() APIs even shorter, let's use
migration_is_running() for outside worlds.

Internally there're actually three places that literally needs
migration_is_active() rather than running().  Keep that an internal helper.

After this patch, we finally only export one helper that allows external
world to try detect migration status, which is migration_is_running().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 1 -
 migration/migration.h    | 1 +
 hw/vfio/common.c         | 4 ++--
 system/dirtylimit.c      | 3 +--
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index ad1e25826a..c0e23fdac9 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
 void migration_object_init(void);
 void migration_shutdown(void);
 
-bool migration_is_active(void);
 bool migration_is_running(void);
 bool migration_thread_is_self(void);
 
diff --git a/migration/migration.h b/migration/migration.h
index 0956e9274b..9fa26ab06a 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
 
 int migrate_init(MigrationState *s, Error **errp);
 bool migration_is_blocked(Error **errp);
+bool migration_is_active(void);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
 bool migration_postcopy_is_alive(MigrationStatus state);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index cc72282c71..7eb99ebd4d 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
 {
     VFIODevice *vbasedev;
 
-    if (!migration_is_active()) {
+    if (!migration_is_running()) {
         return false;
     }
 
@@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
 {
     VFIODevice *vbasedev;
 
-    if (!migration_is_active()) {
+    if (!migration_is_running()) {
         return false;
     }
 
diff --git a/system/dirtylimit.c b/system/dirtylimit.c
index ab20da34bb..d7a855c603 100644
--- a/system/dirtylimit.c
+++ b/system/dirtylimit.c
@@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
     int i = 0;
     int64_t period = DIRTYLIMIT_CALC_TIME_MS;
 
-    if (migrate_dirty_limit() &&
-        migration_is_active()) {
+    if (migrate_dirty_limit() && migration_is_running()) {
         period = migrate_vcpu_dirty_limit_period();
     }
 
-- 
2.45.0

[PATCH v3 8/8] migration: Protect updates to current_migration with a mutex

[Peter Xu]

Introduce migration_mutex, protecting concurrent updates to
current_migration.

In reality, most of the exported migration functions are safe to access
migration objects on capabilities, etc., e.g. many of the code is invoked
within migration thread via different hooks (e.g., precopy notifier,
vmstate handler hooks, etc.).

So literally the mutex so far only makes sure below two APIs that are prone
to accessing freed current_migration:

        migration_is_running()
        migration_file_set_error()

Then we'll need to take the mutex too when init/free the migration object.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h |  3 +++
 migration/migration.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/migration/migration.h b/migration/migration.h
index 9fa26ab06a..05edcf0c49 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -473,6 +473,9 @@ struct MigrationState {
     bool rdma_migration;
 };
 
+extern QemuMutex migration_mutex;
+#define  QEMU_MIGRATION_LOCK_GUARD()  QEMU_LOCK_GUARD(&migration_mutex)
+
 void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
                        MigrationStatus new_state);
 
diff --git a/migration/migration.c b/migration/migration.c
index 127b01734d..ef044968df 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -97,6 +97,14 @@ enum mig_rp_message_type {
    migrations at once.  For now we don't need to add
    dynamic creation of migration */
 
+/*
+ * Protects current_migration below.  Must be hold when using migration
+ * exported functions unless the caller knows it won't get freed.  For
+ * example, when in the context of migration_thread() it's safe to access
+ * current_migration without the mutex, because the thread holds one extra
+ * refcount of the object, so it literally pins the object in-memory.
+ */
+QemuMutex migration_mutex;
 static MigrationState *current_migration;
 static MigrationIncomingState *current_incoming;
 
@@ -110,6 +118,17 @@ static void migrate_fd_cancel(MigrationState *s);
 static bool close_return_path_on_source(MigrationState *s);
 static void migration_completion_end(MigrationState *s);
 
+/*
+ * This is explicitly done without migration_object_init(), because it may
+ * start to use this lock already when instance_init() of the object.  The
+ * mutex is alive for the whole lifecycle of QEMU, so it's always usable
+ * and never destroyed.
+ */
+static void __attribute__((constructor)) migration_mutex_init(void)
+{
+    qemu_mutex_init(&migration_mutex);
+}
+
 static void migration_downtime_start(MigrationState *s)
 {
     trace_vmstate_downtime_checkpoint("src-downtime-start");
@@ -336,6 +355,14 @@ void migration_shutdown(void)
      * stop the migration using this structure
      */
     migration_cancel(NULL);
+    /*
+     * Release the refcount from the main thread.  It means it can be freed
+     * here if migration thread is not running.
+     *
+     * NOTE: we don't need QEMU_MIGRATION_LOCK_GUARD() on this access
+     * because we're sure there's one refcount.  The lock will be taken in
+     * finalize() if it triggers, so we can't take it here anyway.
+     */
     object_unref(OBJECT(current_migration));
 
     /*
@@ -1118,8 +1145,14 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
 
 bool migration_is_running(void)
 {
+    QEMU_MIGRATION_LOCK_GUARD();
+
     MigrationState *s = current_migration;
 
+    if (!s) {
+        return false;
+    }
+
     switch (s->state) {
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
@@ -3029,8 +3062,14 @@ static MigThrError postcopy_pause(MigrationState *s)
 
 void migration_file_set_error(int ret, Error *err)
 {
+    QEMU_MIGRATION_LOCK_GUARD();
+
     MigrationState *s = current_migration;
 
+    if (!s) {
+        return;
+    }
+
     WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
         if (s->to_dst_file) {
             qemu_file_set_error_obj(s->to_dst_file, ret, err);
@@ -3835,6 +3874,8 @@ static void migration_instance_finalize(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
 
+    QEMU_MIGRATION_LOCK_GUARD();
+
     qemu_mutex_destroy(&ms->error_mutex);
     qemu_mutex_destroy(&ms->qemu_file_lock);
     qemu_sem_destroy(&ms->wait_unplug_sem);
@@ -3858,6 +3899,8 @@ static void migration_instance_init(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
 
+    QEMU_MIGRATION_LOCK_GUARD();
+
     /*
      * There can only be one migration object globally. Keep a record of
      * the pointer in current_migration, which will be reset after the
-- 
2.45.0

Re: [PATCH v3 1/8] migration: Take migration object refcount earlier for threads

[Cédric Le Goater]

On 10/24/24 23:30, Peter Xu wrote:
> Both migration thread or background snapshot thread will take a refcount of
> the migration object at the entrace of the thread function.
> 
> That makes sense, because it protects the object from being freed by the
> main thread in migration_shutdown() later, but it might still race with it
> if the thread is scheduled too late.  Consider the case right after
> pthread_create() happened, VM shuts down with the object released, but
> right after that the migration thread finally got created, referencing
> MigrationState* in the opaque pointer which is already freed.
> 
> The only 100% safe way to make sure it won't get freed is taking the
> refcount right before the thread is created, meanwhile when BQL is held.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/migration.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 74812ca785..e82ffa8cf3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3491,7 +3491,6 @@ static void *migration_thread(void *opaque)
>   
>       rcu_register_thread();
>   
> -    object_ref(OBJECT(s));
>       update_iteration_initial_status(s);
>   
>       if (!multifd_send_setup()) {
> @@ -3629,7 +3628,6 @@ static void *bg_migration_thread(void *opaque)
>       int ret;
>   
>       rcu_register_thread();
> -    object_ref(OBJECT(s));
>   
>       migration_rate_set(RATE_LIMIT_DISABLED);
>   
> @@ -3841,6 +3839,14 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>           }
>       }
>   
> +    /*
> +     * Take a refcount to make sure the migration object won't get freed by
> +     * the main thread already in migration_shutdown().
> +     *
> +     * The refcount will be released at the end of the thread function.
> +     */
> +    object_ref(OBJECT(s));
> +
>       if (migrate_background_snapshot()) {
>           qemu_thread_create(&s->thread, MIGRATION_THREAD_SNAPSHOT,
>                   bg_migration_thread, s, QEMU_THREAD_JOINABLE);

yes. It is safer to take a ref before starting the migration thread.

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v3 3/8] migration: Unexport ram_mig_init()

[Cédric Le Goater]

On 10/24/24 23:30, Peter Xu wrote:
> It's only used within migration/.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   include/migration/misc.h | 1 -
>   migration/ram.h          | 1 +
>   2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index df57be6b5e..e8490e3af5 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -39,7 +39,6 @@ void precopy_add_notifier(NotifierWithReturn *n);
>   void precopy_remove_notifier(NotifierWithReturn *n);
>   int precopy_notify(PrecopyNotifyReason reason, Error **errp);
>   
> -void ram_mig_init(void);
>   void qemu_guest_free_page_hint(void *addr, size_t len);
>   bool migrate_ram_is_ignored(RAMBlock *block);
>   
> diff --git a/migration/ram.h b/migration/ram.h
> index bc0318b834..0d1981f888 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -44,6 +44,7 @@ extern XBZRLECacheStats xbzrle_counters;
>       INTERNAL_RAMBLOCK_FOREACH(block)                   \
>           if (!qemu_ram_is_migratable(block)) {} else
>   
> +void ram_mig_init(void);
>   int xbzrle_cache_resize(uint64_t new_size, Error **errp);
>   uint64_t ram_bytes_remaining(void);
>   uint64_t ram_bytes_total(void);

Re: [PATCH v3 4/8] migration: Drop migration_is_setup_or_active()

[Cédric Le Goater]

On 10/24/24 23:30, Peter Xu wrote:
> This helper is mostly the same as migration_is_running(), except that one
> has COLO reported as true, the other has CANCELLING reported as true.
> 
> Per my past years experience on the state changes, none of them should
> matter.
> 
> To make it slightly safer, report both COLO || CANCELLING to be true in
> migration_is_running(), then drop the other one.  We kept the 1st only
> because the name is simpler, and clear enough.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

It is preferable to reduce the API of the core migration subsystem.

Looks safe for vfio.

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   include/migration/misc.h |  4 ++--
>   hw/vfio/common.c         |  2 +-
>   migration/migration.c    | 35 +++--------------------------------
>   migration/ram.c          |  5 ++---
>   net/vhost-vdpa.c         |  3 +--
>   5 files changed, 9 insertions(+), 40 deletions(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index e8490e3af5..86ef160f19 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -52,11 +52,12 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>   /* migration/migration.c */
>   void migration_object_init(void);
>   void migration_shutdown(void);
> +
>   bool migration_is_idle(void);
>   bool migration_is_active(void);
>   bool migration_is_device(void);
> +bool migration_is_running(void);
>   bool migration_thread_is_self(void);
> -bool migration_is_setup_or_active(void);
>   
>   typedef enum MigrationEventType {
>       MIG_EVENT_PRECOPY_SETUP,
> @@ -95,7 +96,6 @@ void migration_add_notifier_mode(NotifierWithReturn *notify,
>                                    MigrationNotifyFunc func, MigMode mode);
>   
>   void migration_remove_notifier(NotifierWithReturn *notify);
> -bool migration_is_running(void);
>   void migration_file_set_error(int ret, Error *err);
>   
>   /* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 36d0cf6585..dcef44fe55 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -149,7 +149,7 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
>   
>   static void vfio_set_migration_error(int ret)
>   {
> -    if (migration_is_setup_or_active()) {
> +    if (migration_is_running()) {
>           migration_file_set_error(ret, NULL);
>       }
>   }
> diff --git a/migration/migration.c b/migration/migration.c
> index e82ffa8cf3..3365195def 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1116,33 +1116,6 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
>       migrate_send_rp_message(mis, MIG_RP_MSG_RESUME_ACK, sizeof(buf), &buf);
>   }
>   
> -/*
> - * Return true if we're already in the middle of a migration
> - * (i.e. any of the active or setup states)
> - */
> -bool migration_is_setup_or_active(void)
> -{
> -    MigrationState *s = current_migration;
> -
> -    switch (s->state) {
> -    case MIGRATION_STATUS_ACTIVE:
> -    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> -    case MIGRATION_STATUS_POSTCOPY_PAUSED:
> -    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> -    case MIGRATION_STATUS_POSTCOPY_RECOVER:
> -    case MIGRATION_STATUS_SETUP:
> -    case MIGRATION_STATUS_PRE_SWITCHOVER:
> -    case MIGRATION_STATUS_DEVICE:
> -    case MIGRATION_STATUS_WAIT_UNPLUG:
> -    case MIGRATION_STATUS_COLO:
> -        return true;
> -
> -    default:
> -        return false;
> -
> -    }
> -}
> -
>   bool migration_is_running(void)
>   {
>       MigrationState *s = current_migration;
> @@ -1158,11 +1131,10 @@ bool migration_is_running(void)
>       case MIGRATION_STATUS_DEVICE:
>       case MIGRATION_STATUS_WAIT_UNPLUG:
>       case MIGRATION_STATUS_CANCELLING:
> +    case MIGRATION_STATUS_COLO:
>           return true;
> -
>       default:
>           return false;
> -
>       }
>   }
>   
> @@ -1661,8 +1633,7 @@ bool migration_incoming_postcopy_advised(void)
>   
>   bool migration_in_bg_snapshot(void)
>   {
> -    return migrate_background_snapshot() &&
> -           migration_is_setup_or_active();
> +    return migrate_background_snapshot() && migration_is_running();
>   }
>   
>   bool migration_is_idle(void)
> @@ -2335,7 +2306,7 @@ static void *source_return_path_thread(void *opaque)
>       trace_source_return_path_thread_entry();
>       rcu_register_thread();
>   
> -    while (migration_is_setup_or_active()) {
> +    while (migration_is_running()) {
>           trace_source_return_path_thread_loop_top();
>   
>           header_type = qemu_get_be16(rp);
> diff --git a/migration/ram.c b/migration/ram.c
> index d284f63854..5646a0b882 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2860,7 +2860,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
>       size_t used_len, start, npages;
>   
>       /* This function is currently expected to be used during live migration */
> -    if (!migration_is_setup_or_active()) {
> +    if (!migration_is_running()) {
>           return;
>       }
>   
> @@ -3208,8 +3208,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>       }
>   
>   out:
> -    if (ret >= 0
> -        && migration_is_setup_or_active()) {
> +    if (ret >= 0 && migration_is_running()) {
>           if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
>               !migrate_mapped_ram()) {
>               ret = multifd_ram_flush_and_sync();
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 46b02c50be..231b45246c 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -375,8 +375,7 @@ static int vhost_vdpa_net_data_start(NetClientState *nc)
>   
>       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>   
> -    if (s->always_svq ||
> -        migration_is_setup_or_active()) {
> +    if (s->always_svq || migration_is_running()) {
>           v->shadow_vqs_enabled = true;
>       } else {
>           v->shadow_vqs_enabled = false;

Re: [PATCH v3 5/8] migration: Drop migration_is_idle()

[Cédric Le Goater]

On 10/24/24 23:30, Peter Xu wrote:
> Now with the current migration_is_running(), it will report exactly the
> opposite of what will be reported by migration_is_idle().
> 
> Drop migration_is_idle(), instead use "!migration_is_running()" which
> should be identical on functionality.
> 
> In reality, most of the idle check is inverted, so it's even easier to
> write with "migrate_is_running()" check.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   include/migration/misc.h |  1 -
>   hw/virtio/virtio-mem.c   |  2 +-
>   migration/migration.c    | 21 +--------------------
>   migration/ram.c          |  2 +-
>   system/qdev-monitor.c    |  4 ++--
>   5 files changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 86ef160f19..804eb23c06 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>   void migration_object_init(void);
>   void migration_shutdown(void);
>   
> -bool migration_is_idle(void);
>   bool migration_is_active(void);
>   bool migration_is_device(void);
>   bool migration_is_running(void);
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index ae1e81d7ba..80ada89551 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -188,7 +188,7 @@ static bool virtio_mem_is_busy(void)
>        * after plugging them) until we're running on the destination (as we didn't
>        * migrate these blocks when they were unplugged).
>        */
> -    return migration_in_incoming_postcopy() || !migration_is_idle();
> +    return migration_in_incoming_postcopy() || migration_is_running();
>   }
>   
>   typedef int (*virtio_mem_range_cb)(VirtIOMEM *vmem, void *arg,
> diff --git a/migration/migration.c b/migration/migration.c
> index 3365195def..f86c709699 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1636,25 +1636,6 @@ bool migration_in_bg_snapshot(void)
>       return migrate_background_snapshot() && migration_is_running();
>   }
>   
> -bool migration_is_idle(void)
> -{
> -    MigrationState *s = current_migration;
> -
> -    if (!s) {
> -        return true;
> -    }
> -
> -    switch (s->state) {
> -    case MIGRATION_STATUS_NONE:
> -    case MIGRATION_STATUS_CANCELLED:
> -    case MIGRATION_STATUS_COMPLETED:
> -    case MIGRATION_STATUS_FAILED:
> -        return true;
> -    default:
> -        return false;
> -    }
> -}
> -
>   bool migration_is_active(void)
>   {
>       MigrationState *s = current_migration;
> @@ -1733,7 +1714,7 @@ static bool is_busy(Error **reasonp, Error **errp)
>       ERRP_GUARD();
>   
>       /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> -    if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
> +    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
>           error_propagate_prepend(errp, *reasonp,
>                                   "disallowing migration blocker "
>                                   "(migration/snapshot in progress) for: ");
> diff --git a/migration/ram.c b/migration/ram.c
> index 5646a0b882..504b48d584 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4498,7 +4498,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>           return;
>       }
>   
> -    if (!migration_is_idle()) {
> +    if (migration_is_running()) {
>           /*
>            * Precopy code on the source cannot deal with the size of RAM blocks
>            * changing at random points in time - especially after sending the
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 1310f35c9f..83fa684475 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -686,7 +686,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>           return NULL;
>       }
>   
> -    if (!migration_is_idle()) {
> +    if (migration_is_running()) {
>           error_setg(errp, "device_add not allowed while migrating");
>           return NULL;
>       }
> @@ -935,7 +935,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    if (!migration_is_idle() && !dev->allow_unplug_during_migration) {
> +    if (migration_is_running() && !dev->allow_unplug_during_migration) {
>           error_setg(errp, "device_del not allowed while migrating");
>           return;
>       }

Re: [PATCH v3 8/8] migration: Protect updates to current_migration with a mutex

[Cédric Le Goater]

On 10/24/24 23:30, Peter Xu wrote:
> Introduce migration_mutex, protecting concurrent updates to
> current_migration.
> 
> In reality, most of the exported migration functions are safe to access
> migration objects on capabilities, etc., e.g. many of the code is invoked
> within migration thread via different hooks (e.g., precopy notifier,
> vmstate handler hooks, etc.).
> 
> So literally the mutex so far only makes sure below two APIs that are prone
> to accessing freed current_migration:
> 
>          migration_is_running()
>          migration_file_set_error()
> 
> Then we'll need to take the mutex too when init/free the migration object.

we should also drop :

   static void vfio_set_migration_error(int ret)
   {
       if (migration_is_running()) {
           migration_file_set_error(ret, NULL);
       }
   }

and use directly migration_file_set_error().

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/migration.h |  3 +++
>   migration/migration.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 9fa26ab06a..05edcf0c49 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -473,6 +473,9 @@ struct MigrationState {
>       bool rdma_migration;
>   };
>   
> +extern QemuMutex migration_mutex;
> +#define  QEMU_MIGRATION_LOCK_GUARD()  QEMU_LOCK_GUARD(&migration_mutex)
> +

Why are these definitions exported ?

Thanks,

C.



>   void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
>                          MigrationStatus new_state);
>   
> diff --git a/migration/migration.c b/migration/migration.c
> index 127b01734d..ef044968df 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -97,6 +97,14 @@ enum mig_rp_message_type {
>      migrations at once.  For now we don't need to add
>      dynamic creation of migration */
>   
> +/*
> + * Protects current_migration below.  Must be hold when using migration
> + * exported functions unless the caller knows it won't get freed.  For
> + * example, when in the context of migration_thread() it's safe to access
> + * current_migration without the mutex, because the thread holds one extra
> + * refcount of the object, so it literally pins the object in-memory.
> + */
> +QemuMutex migration_mutex;
>   static MigrationState *current_migration;
>   static MigrationIncomingState *current_incoming;
>   
> @@ -110,6 +118,17 @@ static void migrate_fd_cancel(MigrationState *s);
>   static bool close_return_path_on_source(MigrationState *s);
>   static void migration_completion_end(MigrationState *s);
>   
> +/*
> + * This is explicitly done without migration_object_init(), because it may
> + * start to use this lock already when instance_init() of the object.  The
> + * mutex is alive for the whole lifecycle of QEMU, so it's always usable
> + * and never destroyed.
> + */
> +static void __attribute__((constructor)) migration_mutex_init(void)
> +{
> +    qemu_mutex_init(&migration_mutex);
> +}
> +
>   static void migration_downtime_start(MigrationState *s)
>   {
>       trace_vmstate_downtime_checkpoint("src-downtime-start");
> @@ -336,6 +355,14 @@ void migration_shutdown(void)
>        * stop the migration using this structure
>        */
>       migration_cancel(NULL);
> +    /*
> +     * Release the refcount from the main thread.  It means it can be freed
> +     * here if migration thread is not running.
> +     *
> +     * NOTE: we don't need QEMU_MIGRATION_LOCK_GUARD() on this access
> +     * because we're sure there's one refcount.  The lock will be taken in
> +     * finalize() if it triggers, so we can't take it here anyway.
> +     */
>       object_unref(OBJECT(current_migration));
>   
>       /*
> @@ -1118,8 +1145,14 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
>   
>   bool migration_is_running(void)
>   {
> +    QEMU_MIGRATION_LOCK_GUARD();
> +
>       MigrationState *s = current_migration;
>   
> +    if (!s) {
> +        return false;
> +    }
> +
>       switch (s->state) {
>       case MIGRATION_STATUS_ACTIVE:
>       case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> @@ -3029,8 +3062,14 @@ static MigThrError postcopy_pause(MigrationState *s)
>   
>   void migration_file_set_error(int ret, Error *err)
>   {
> +    QEMU_MIGRATION_LOCK_GUARD();
> +
>       MigrationState *s = current_migration;
>   
> +    if (!s) {
> +        return;
> +    }
> +
>       WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
>           if (s->to_dst_file) {
>               qemu_file_set_error_obj(s->to_dst_file, ret, err);
> @@ -3835,6 +3874,8 @@ static void migration_instance_finalize(Object *obj)
>   {
>       MigrationState *ms = MIGRATION_OBJ(obj);
>   
> +    QEMU_MIGRATION_LOCK_GUARD();
> +
>       qemu_mutex_destroy(&ms->error_mutex);
>       qemu_mutex_destroy(&ms->qemu_file_lock);
>       qemu_sem_destroy(&ms->wait_unplug_sem);
> @@ -3858,6 +3899,8 @@ static void migration_instance_init(Object *obj)
>   {
>       MigrationState *ms = MIGRATION_OBJ(obj);
>   
> +    QEMU_MIGRATION_LOCK_GUARD();
> +
>       /*
>        * There can only be one migration object globally. Keep a record of
>        * the pointer in current_migration, which will be reset after the

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Avihai Horon]

On 25/10/2024 0:30, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> We have two outside users of this API, so it's exported.
>
> Is it really necessary?  Does it matter whether it must be
> ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.

Actually for VFIO it does matter, because we don't want VFIO to do DPT 
log_sync in SETUP stage when DPT might not have been started yet.
See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration 
SETUP state").

Thanks.

>
> The external user is trying to detect whether migration is running or not,
> as simple as that.
>
> To make the migration_is*() APIs even shorter, let's use
> migration_is_running() for outside worlds.
>
> Internally there're actually three places that literally needs
> migration_is_active() rather than running().  Keep that an internal helper.
>
> After this patch, we finally only export one helper that allows external
> world to try detect migration status, which is migration_is_running().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/migration/misc.h | 1 -
>   migration/migration.h    | 1 +
>   hw/vfio/common.c         | 4 ++--
>   system/dirtylimit.c      | 3 +--
>   4 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index ad1e25826a..c0e23fdac9 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>   void migration_object_init(void);
>   void migration_shutdown(void);
>
> -bool migration_is_active(void);
>   bool migration_is_running(void);
>   bool migration_thread_is_self(void);
>
> diff --git a/migration/migration.h b/migration/migration.h
> index 0956e9274b..9fa26ab06a 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>
>   int migrate_init(MigrationState *s, Error **errp);
>   bool migration_is_blocked(Error **errp);
> +bool migration_is_active(void);
>   /* True if outgoing migration has entered postcopy phase */
>   bool migration_in_postcopy(void);
>   bool migration_postcopy_is_alive(MigrationStatus state);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index cc72282c71..7eb99ebd4d 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
>   {
>       VFIODevice *vbasedev;
>
> -    if (!migration_is_active()) {
> +    if (!migration_is_running()) {
>           return false;
>       }
>
> @@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
>   {
>       VFIODevice *vbasedev;
>
> -    if (!migration_is_active()) {
> +    if (!migration_is_running()) {
>           return false;
>       }
>
> diff --git a/system/dirtylimit.c b/system/dirtylimit.c
> index ab20da34bb..d7a855c603 100644
> --- a/system/dirtylimit.c
> +++ b/system/dirtylimit.c
> @@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
>       int i = 0;
>       int64_t period = DIRTYLIMIT_CALC_TIME_MS;
>
> -    if (migrate_dirty_limit() &&
> -        migration_is_active()) {
> +    if (migrate_dirty_limit() && migration_is_running()) {
>           period = migrate_vcpu_dirty_limit_period();
>       }
>
> --
> 2.45.0
>

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Peter Xu]

On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
> 
> On 25/10/2024 0:30, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > We have two outside users of this API, so it's exported.
> > 
> > Is it really necessary?  Does it matter whether it must be
> > ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
> 
> Actually for VFIO it does matter, because we don't want VFIO to do DPT
> log_sync in SETUP stage when DPT might not have been started yet.
> See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
> SETUP state").

This seems to be a known issue for migration in general, rather than VFIO
specific.  Hyman has a patch for it, not yet reviewed..

https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com

That corresponds to your comment here:

    Redundant -- all RAM is marked dirty in migration SETUP state and is
    transferred only after migration is set to ACTIVE state, so doing
    log_sync during migration SETUP is pointless.

So I wonder whether it's only VFIO that should skip it, or log_sync()
simply shouldn't be called at all during SETUP, because of its redundancy.

The other thing you mentioned here:

    Can fail -- there is a time window, between setting migration state to
    SETUP and starting dirty tracking by RAM save_live_setup handler, during
    which dirty tracking is still not started. Any VFIO log_sync call that
    is issued during this time window will fail. For example, this error can
    be triggered by migrating a VM when a GUI is active, which constantly
    calls log_sync.

This is VFIO specific.  Why this can fail even if global tracking is
started already?  I didn't yet get why a GUI being active in guest could
affect log_sync() from working.. after vfio_listener_log_global_start()
properly setup everything.

Thanks,

> 
> Thanks.
> 
> > 
> > The external user is trying to detect whether migration is running or not,
> > as simple as that.
> > 
> > To make the migration_is*() APIs even shorter, let's use
> > migration_is_running() for outside worlds.
> > 
> > Internally there're actually three places that literally needs
> > migration_is_active() rather than running().  Keep that an internal helper.
> > 
> > After this patch, we finally only export one helper that allows external
> > world to try detect migration status, which is migration_is_running().
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   include/migration/misc.h | 1 -
> >   migration/migration.h    | 1 +
> >   hw/vfio/common.c         | 4 ++--
> >   system/dirtylimit.c      | 3 +--
> >   4 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > index ad1e25826a..c0e23fdac9 100644
> > --- a/include/migration/misc.h
> > +++ b/include/migration/misc.h
> > @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
> >   void migration_object_init(void);
> >   void migration_shutdown(void);
> > 
> > -bool migration_is_active(void);
> >   bool migration_is_running(void);
> >   bool migration_thread_is_self(void);
> > 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 0956e9274b..9fa26ab06a 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
> > 
> >   int migrate_init(MigrationState *s, Error **errp);
> >   bool migration_is_blocked(Error **errp);
> > +bool migration_is_active(void);
> >   /* True if outgoing migration has entered postcopy phase */
> >   bool migration_in_postcopy(void);
> >   bool migration_postcopy_is_alive(MigrationStatus state);
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index cc72282c71..7eb99ebd4d 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
> >   {
> >       VFIODevice *vbasedev;
> > 
> > -    if (!migration_is_active()) {
> > +    if (!migration_is_running()) {
> >           return false;
> >       }
> > 
> > @@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
> >   {
> >       VFIODevice *vbasedev;
> > 
> > -    if (!migration_is_active()) {
> > +    if (!migration_is_running()) {
> >           return false;
> >       }
> > 
> > diff --git a/system/dirtylimit.c b/system/dirtylimit.c
> > index ab20da34bb..d7a855c603 100644
> > --- a/system/dirtylimit.c
> > +++ b/system/dirtylimit.c
> > @@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
> >       int i = 0;
> >       int64_t period = DIRTYLIMIT_CALC_TIME_MS;
> > 
> > -    if (migrate_dirty_limit() &&
> > -        migration_is_active()) {
> > +    if (migrate_dirty_limit() && migration_is_running()) {
> >           period = migrate_vcpu_dirty_limit_period();
> >       }
> > 
> > --
> > 2.45.0
> > 
> 

-- 
Peter Xu

Re: [PATCH v3 8/8] migration: Protect updates to current_migration with a mutex

[Peter Xu]

On Fri, Oct 25, 2024 at 02:50:36PM +0200, Cédric Le Goater wrote:
> On 10/24/24 23:30, Peter Xu wrote:
> > Introduce migration_mutex, protecting concurrent updates to
> > current_migration.
> > 
> > In reality, most of the exported migration functions are safe to access
> > migration objects on capabilities, etc., e.g. many of the code is invoked
> > within migration thread via different hooks (e.g., precopy notifier,
> > vmstate handler hooks, etc.).
> > 
> > So literally the mutex so far only makes sure below two APIs that are prone
> > to accessing freed current_migration:
> > 
> >          migration_is_running()
> >          migration_file_set_error()
> > 
> > Then we'll need to take the mutex too when init/free the migration object.
> 
> we should also drop :
> 
>   static void vfio_set_migration_error(int ret)
>   {
>       if (migration_is_running()) {
>           migration_file_set_error(ret, NULL);
>       }
>   }
> 
> and use directly migration_file_set_error().

We'll need to export migration_is_running() anyway, though.  So maybe we
can do that as a VFIO's follow up?

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   migration/migration.h |  3 +++
> >   migration/migration.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 46 insertions(+)
> > 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 9fa26ab06a..05edcf0c49 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -473,6 +473,9 @@ struct MigrationState {
> >       bool rdma_migration;
> >   };
> > +extern QemuMutex migration_mutex;
> > +#define  QEMU_MIGRATION_LOCK_GUARD()  QEMU_LOCK_GUARD(&migration_mutex)
> > +
> 
> Why are these definitions exported ?

This is still only used in migration/ so it's not exported to QEMU.  I was
planning this can be available anywhere in migration/ when a function needs
to be exported.

However I think you're right.. this so far is even only used in
migration.c, so we don't need to export it, at least until some other
migration/*.c will export anything.  Will remove it when repost.

Thanks,

> 
> Thanks,
> 
> C.
> 
> 
> 
> >   void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
> >                          MigrationStatus new_state);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 127b01734d..ef044968df 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -97,6 +97,14 @@ enum mig_rp_message_type {
> >      migrations at once.  For now we don't need to add
> >      dynamic creation of migration */
> > +/*
> > + * Protects current_migration below.  Must be hold when using migration
> > + * exported functions unless the caller knows it won't get freed.  For
> > + * example, when in the context of migration_thread() it's safe to access
> > + * current_migration without the mutex, because the thread holds one extra
> > + * refcount of the object, so it literally pins the object in-memory.
> > + */
> > +QemuMutex migration_mutex;
> >   static MigrationState *current_migration;
> >   static MigrationIncomingState *current_incoming;
> > @@ -110,6 +118,17 @@ static void migrate_fd_cancel(MigrationState *s);
> >   static bool close_return_path_on_source(MigrationState *s);
> >   static void migration_completion_end(MigrationState *s);
> > +/*
> > + * This is explicitly done without migration_object_init(), because it may
> > + * start to use this lock already when instance_init() of the object.  The
> > + * mutex is alive for the whole lifecycle of QEMU, so it's always usable
> > + * and never destroyed.
> > + */
> > +static void __attribute__((constructor)) migration_mutex_init(void)
> > +{
> > +    qemu_mutex_init(&migration_mutex);
> > +}
> > +
> >   static void migration_downtime_start(MigrationState *s)
> >   {
> >       trace_vmstate_downtime_checkpoint("src-downtime-start");
> > @@ -336,6 +355,14 @@ void migration_shutdown(void)
> >        * stop the migration using this structure
> >        */
> >       migration_cancel(NULL);
> > +    /*
> > +     * Release the refcount from the main thread.  It means it can be freed
> > +     * here if migration thread is not running.
> > +     *
> > +     * NOTE: we don't need QEMU_MIGRATION_LOCK_GUARD() on this access
> > +     * because we're sure there's one refcount.  The lock will be taken in
> > +     * finalize() if it triggers, so we can't take it here anyway.
> > +     */
> >       object_unref(OBJECT(current_migration));
> >       /*
> > @@ -1118,8 +1145,14 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
> >   bool migration_is_running(void)
> >   {
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       MigrationState *s = current_migration;
> > +    if (!s) {
> > +        return false;
> > +    }
> > +
> >       switch (s->state) {
> >       case MIGRATION_STATUS_ACTIVE:
> >       case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > @@ -3029,8 +3062,14 @@ static MigThrError postcopy_pause(MigrationState *s)
> >   void migration_file_set_error(int ret, Error *err)
> >   {
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       MigrationState *s = current_migration;
> > +    if (!s) {
> > +        return;
> > +    }
> > +
> >       WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
> >           if (s->to_dst_file) {
> >               qemu_file_set_error_obj(s->to_dst_file, ret, err);
> > @@ -3835,6 +3874,8 @@ static void migration_instance_finalize(Object *obj)
> >   {
> >       MigrationState *ms = MIGRATION_OBJ(obj);
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       qemu_mutex_destroy(&ms->error_mutex);
> >       qemu_mutex_destroy(&ms->qemu_file_lock);
> >       qemu_sem_destroy(&ms->wait_unplug_sem);
> > @@ -3858,6 +3899,8 @@ static void migration_instance_init(Object *obj)
> >   {
> >       MigrationState *ms = MIGRATION_OBJ(obj);
> > +    QEMU_MIGRATION_LOCK_GUARD();
> > +
> >       /*
> >        * There can only be one migration object globally. Keep a record of
> >        * the pointer in current_migration, which will be reset after the
> 

-- 
Peter Xu

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Avihai Horon]

On 28/10/2024 17:45, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
>> On 25/10/2024 0:30, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> We have two outside users of this API, so it's exported.
>>>
>>> Is it really necessary?  Does it matter whether it must be
>>> ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
>> Actually for VFIO it does matter, because we don't want VFIO to do DPT
>> log_sync in SETUP stage when DPT might not have been started yet.
>> See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
>> SETUP state").
> This seems to be a known issue for migration in general, rather than VFIO
> specific.  Hyman has a patch for it, not yet reviewed..
>
> https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
>
> That corresponds to your comment here:
>
>      Redundant -- all RAM is marked dirty in migration SETUP state and is
>      transferred only after migration is set to ACTIVE state, so doing
>      log_sync during migration SETUP is pointless.
>
> So I wonder whether it's only VFIO that should skip it, or log_sync()
> simply shouldn't be called at all during SETUP, because of its redundancy.

Not sure why this sync was there in the first place, but if its only 
purpose was to sync dirty pages then yes, I guess it be dropped.

>
> The other thing you mentioned here:
>
>      Can fail -- there is a time window, between setting migration state to
>      SETUP and starting dirty tracking by RAM save_live_setup handler, during
>      which dirty tracking is still not started. Any VFIO log_sync call that
>      is issued during this time window will fail. For example, this error can
>      be triggered by migrating a VM when a GUI is active, which constantly
>      calls log_sync.
>
> This is VFIO specific.  Why this can fail even if global tracking is
> started already?

It can fail if global tracking is *not* started yet.
As mentioned in the commit message, there is a time window where 
migration is in SETUP state but global tracking is not started yet.

Thanks.

> I didn't yet get why a GUI being active in guest could
> affect log_sync() from working.. after vfio_listener_log_global_start()
> properly setup everything.
>
> Thanks,
>
>> Thanks.
>>
>>> The external user is trying to detect whether migration is running or not,
>>> as simple as that.
>>>
>>> To make the migration_is*() APIs even shorter, let's use
>>> migration_is_running() for outside worlds.
>>>
>>> Internally there're actually three places that literally needs
>>> migration_is_active() rather than running().  Keep that an internal helper.
>>>
>>> After this patch, we finally only export one helper that allows external
>>> world to try detect migration status, which is migration_is_running().
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>    include/migration/misc.h | 1 -
>>>    migration/migration.h    | 1 +
>>>    hw/vfio/common.c         | 4 ++--
>>>    system/dirtylimit.c      | 3 +--
>>>    4 files changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>> index ad1e25826a..c0e23fdac9 100644
>>> --- a/include/migration/misc.h
>>> +++ b/include/migration/misc.h
>>> @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>>>    void migration_object_init(void);
>>>    void migration_shutdown(void);
>>>
>>> -bool migration_is_active(void);
>>>    bool migration_is_running(void);
>>>    bool migration_thread_is_self(void);
>>>
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index 0956e9274b..9fa26ab06a 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>>>
>>>    int migrate_init(MigrationState *s, Error **errp);
>>>    bool migration_is_blocked(Error **errp);
>>> +bool migration_is_active(void);
>>>    /* True if outgoing migration has entered postcopy phase */
>>>    bool migration_in_postcopy(void);
>>>    bool migration_postcopy_is_alive(MigrationStatus state);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index cc72282c71..7eb99ebd4d 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
>>>    {
>>>        VFIODevice *vbasedev;
>>>
>>> -    if (!migration_is_active()) {
>>> +    if (!migration_is_running()) {
>>>            return false;
>>>        }
>>>
>>> @@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
>>>    {
>>>        VFIODevice *vbasedev;
>>>
>>> -    if (!migration_is_active()) {
>>> +    if (!migration_is_running()) {
>>>            return false;
>>>        }
>>>
>>> diff --git a/system/dirtylimit.c b/system/dirtylimit.c
>>> index ab20da34bb..d7a855c603 100644
>>> --- a/system/dirtylimit.c
>>> +++ b/system/dirtylimit.c
>>> @@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
>>>        int i = 0;
>>>        int64_t period = DIRTYLIMIT_CALC_TIME_MS;
>>>
>>> -    if (migrate_dirty_limit() &&
>>> -        migration_is_active()) {
>>> +    if (migrate_dirty_limit() && migration_is_running()) {
>>>            period = migrate_vcpu_dirty_limit_period();
>>>        }
>>>
>>> --
>>> 2.45.0
>>>
> --
> Peter Xu
>

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Peter Xu]

On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
> 
> On 28/10/2024 17:45, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
> > > On 25/10/2024 0:30, Peter Xu wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > We have two outside users of this API, so it's exported.
> > > > 
> > > > Is it really necessary?  Does it matter whether it must be
> > > > ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
> > > Actually for VFIO it does matter, because we don't want VFIO to do DPT
> > > log_sync in SETUP stage when DPT might not have been started yet.
> > > See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
> > > SETUP state").
> > This seems to be a known issue for migration in general, rather than VFIO
> > specific.  Hyman has a patch for it, not yet reviewed..
> > 
> > https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
> > 
> > That corresponds to your comment here:
> > 
> >      Redundant -- all RAM is marked dirty in migration SETUP state and is
> >      transferred only after migration is set to ACTIVE state, so doing
> >      log_sync during migration SETUP is pointless.
> > 
> > So I wonder whether it's only VFIO that should skip it, or log_sync()
> > simply shouldn't be called at all during SETUP, because of its redundancy.
> 
> Not sure why this sync was there in the first place, but if its only purpose
> was to sync dirty pages then yes, I guess it be dropped.
> 
> > 
> > The other thing you mentioned here:
> > 
> >      Can fail -- there is a time window, between setting migration state to
> >      SETUP and starting dirty tracking by RAM save_live_setup handler, during
> >      which dirty tracking is still not started. Any VFIO log_sync call that
> >      is issued during this time window will fail. For example, this error can
> >      be triggered by migrating a VM when a GUI is active, which constantly
> >      calls log_sync.
> > 
> > This is VFIO specific.  Why this can fail even if global tracking is
> > started already?
> 
> It can fail if global tracking is *not* started yet.
> As mentioned in the commit message, there is a time window where migration
> is in SETUP state but global tracking is not started yet.

Hmm, I'm totally confused now..

The only thing that can kickoff the sync during SETUP, AFAICT, is:

            ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
            if (!ret) {
                goto out_unlock;
            }
            migration_bitmap_sync_precopy(false);   <------------- here

I need to confess this may not be the right place to invoke it in ram.c (I
think we probably should move it out at some point.. into generic migration
code).  However I don't yet see why log_start() is not called first in your
case before log_sync().

Could you elaborate?

> 
> Thanks.
> 
> > I didn't yet get why a GUI being active in guest could
> > affect log_sync() from working.. after vfio_listener_log_global_start()
> > properly setup everything.
> > 
> > Thanks,
> > 
> > > Thanks.
> > > 
> > > > The external user is trying to detect whether migration is running or not,
> > > > as simple as that.
> > > > 
> > > > To make the migration_is*() APIs even shorter, let's use
> > > > migration_is_running() for outside worlds.
> > > > 
> > > > Internally there're actually three places that literally needs
> > > > migration_is_active() rather than running().  Keep that an internal helper.
> > > > 
> > > > After this patch, we finally only export one helper that allows external
> > > > world to try detect migration status, which is migration_is_running().
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >    include/migration/misc.h | 1 -
> > > >    migration/migration.h    | 1 +
> > > >    hw/vfio/common.c         | 4 ++--
> > > >    system/dirtylimit.c      | 3 +--
> > > >    4 files changed, 4 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > > > index ad1e25826a..c0e23fdac9 100644
> > > > --- a/include/migration/misc.h
> > > > +++ b/include/migration/misc.h
> > > > @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
> > > >    void migration_object_init(void);
> > > >    void migration_shutdown(void);
> > > > 
> > > > -bool migration_is_active(void);
> > > >    bool migration_is_running(void);
> > > >    bool migration_thread_is_self(void);
> > > > 
> > > > diff --git a/migration/migration.h b/migration/migration.h
> > > > index 0956e9274b..9fa26ab06a 100644
> > > > --- a/migration/migration.h
> > > > +++ b/migration/migration.h
> > > > @@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
> > > > 
> > > >    int migrate_init(MigrationState *s, Error **errp);
> > > >    bool migration_is_blocked(Error **errp);
> > > > +bool migration_is_active(void);
> > > >    /* True if outgoing migration has entered postcopy phase */
> > > >    bool migration_in_postcopy(void);
> > > >    bool migration_postcopy_is_alive(MigrationStatus state);
> > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > index cc72282c71..7eb99ebd4d 100644
> > > > --- a/hw/vfio/common.c
> > > > +++ b/hw/vfio/common.c
> > > > @@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
> > > >    {
> > > >        VFIODevice *vbasedev;
> > > > 
> > > > -    if (!migration_is_active()) {
> > > > +    if (!migration_is_running()) {
> > > >            return false;
> > > >        }
> > > > 
> > > > @@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
> > > >    {
> > > >        VFIODevice *vbasedev;
> > > > 
> > > > -    if (!migration_is_active()) {
> > > > +    if (!migration_is_running()) {
> > > >            return false;
> > > >        }
> > > > 
> > > > diff --git a/system/dirtylimit.c b/system/dirtylimit.c
> > > > index ab20da34bb..d7a855c603 100644
> > > > --- a/system/dirtylimit.c
> > > > +++ b/system/dirtylimit.c
> > > > @@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
> > > >        int i = 0;
> > > >        int64_t period = DIRTYLIMIT_CALC_TIME_MS;
> > > > 
> > > > -    if (migrate_dirty_limit() &&
> > > > -        migration_is_active()) {
> > > > +    if (migrate_dirty_limit() && migration_is_running()) {
> > > >            period = migrate_vcpu_dirty_limit_period();
> > > >        }
> > > > 
> > > > --
> > > > 2.45.0
> > > > 
> > --
> > Peter Xu
> > 
> 

-- 
Peter Xu

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Avihai Horon]

On 28/10/2024 18:58, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
>> On 28/10/2024 17:45, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
>>>> On 25/10/2024 0:30, Peter Xu wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> We have two outside users of this API, so it's exported.
>>>>>
>>>>> Is it really necessary?  Does it matter whether it must be
>>>>> ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
>>>> Actually for VFIO it does matter, because we don't want VFIO to do DPT
>>>> log_sync in SETUP stage when DPT might not have been started yet.
>>>> See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
>>>> SETUP state").
>>> This seems to be a known issue for migration in general, rather than VFIO
>>> specific.  Hyman has a patch for it, not yet reviewed..
>>>
>>> https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
>>>
>>> That corresponds to your comment here:
>>>
>>>       Redundant -- all RAM is marked dirty in migration SETUP state and is
>>>       transferred only after migration is set to ACTIVE state, so doing
>>>       log_sync during migration SETUP is pointless.
>>>
>>> So I wonder whether it's only VFIO that should skip it, or log_sync()
>>> simply shouldn't be called at all during SETUP, because of its redundancy.
>> Not sure why this sync was there in the first place, but if its only purpose
>> was to sync dirty pages then yes, I guess it be dropped.
>>
>>> The other thing you mentioned here:
>>>
>>>       Can fail -- there is a time window, between setting migration state to
>>>       SETUP and starting dirty tracking by RAM save_live_setup handler, during
>>>       which dirty tracking is still not started. Any VFIO log_sync call that
>>>       is issued during this time window will fail. For example, this error can
>>>       be triggered by migrating a VM when a GUI is active, which constantly
>>>       calls log_sync.
>>>
>>> This is VFIO specific.  Why this can fail even if global tracking is
>>> started already?
>> It can fail if global tracking is *not* started yet.
>> As mentioned in the commit message, there is a time window where migration
>> is in SETUP state but global tracking is not started yet.
> Hmm, I'm totally confused now..
>
> The only thing that can kickoff the sync during SETUP, AFAICT, is:
>
>              ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
>              if (!ret) {
>                  goto out_unlock;
>              }
>              migration_bitmap_sync_precopy(false);   <------------- here
>
> I need to confess this may not be the right place to invoke it in ram.c (I
> think we probably should move it out at some point.. into generic migration
> code).  However I don't yet see why log_start() is not called first in your
> case before log_sync().
>
> Could you elaborate?

Indeed, in the above code log_start is called before log_sync.

I was referring to the case where some other code path triggers log_sync.
E.g., if you open a VNC to the guest then it constantly calls log_sync 
to refresh the graphics. In that case, one of these log_syncs can happen 
between "migration status is set to SETUP" and "global tracking is started".

Thanks.

>> Thanks.
>>
>>> I didn't yet get why a GUI being active in guest could
>>> affect log_sync() from working.. after vfio_listener_log_global_start()
>>> properly setup everything.
>>>
>>> Thanks,
>>>
>>>> Thanks.
>>>>
>>>>> The external user is trying to detect whether migration is running or not,
>>>>> as simple as that.
>>>>>
>>>>> To make the migration_is*() APIs even shorter, let's use
>>>>> migration_is_running() for outside worlds.
>>>>>
>>>>> Internally there're actually three places that literally needs
>>>>> migration_is_active() rather than running().  Keep that an internal helper.
>>>>>
>>>>> After this patch, we finally only export one helper that allows external
>>>>> world to try detect migration status, which is migration_is_running().
>>>>>
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>> ---
>>>>>     include/migration/misc.h | 1 -
>>>>>     migration/migration.h    | 1 +
>>>>>     hw/vfio/common.c         | 4 ++--
>>>>>     system/dirtylimit.c      | 3 +--
>>>>>     4 files changed, 4 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>>>> index ad1e25826a..c0e23fdac9 100644
>>>>> --- a/include/migration/misc.h
>>>>> +++ b/include/migration/misc.h
>>>>> @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>>>>>     void migration_object_init(void);
>>>>>     void migration_shutdown(void);
>>>>>
>>>>> -bool migration_is_active(void);
>>>>>     bool migration_is_running(void);
>>>>>     bool migration_thread_is_self(void);
>>>>>
>>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>>> index 0956e9274b..9fa26ab06a 100644
>>>>> --- a/migration/migration.h
>>>>> +++ b/migration/migration.h
>>>>> @@ -492,6 +492,7 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>>>>>
>>>>>     int migrate_init(MigrationState *s, Error **errp);
>>>>>     bool migration_is_blocked(Error **errp);
>>>>> +bool migration_is_active(void);
>>>>>     /* True if outgoing migration has entered postcopy phase */
>>>>>     bool migration_in_postcopy(void);
>>>>>     bool migration_postcopy_is_alive(MigrationStatus state);
>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>>> index cc72282c71..7eb99ebd4d 100644
>>>>> --- a/hw/vfio/common.c
>>>>> +++ b/hw/vfio/common.c
>>>>> @@ -174,7 +174,7 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
>>>>>     {
>>>>>         VFIODevice *vbasedev;
>>>>>
>>>>> -    if (!migration_is_active()) {
>>>>> +    if (!migration_is_running()) {
>>>>>             return false;
>>>>>         }
>>>>>
>>>>> @@ -219,7 +219,7 @@ vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
>>>>>     {
>>>>>         VFIODevice *vbasedev;
>>>>>
>>>>> -    if (!migration_is_active()) {
>>>>> +    if (!migration_is_running()) {
>>>>>             return false;
>>>>>         }
>>>>>
>>>>> diff --git a/system/dirtylimit.c b/system/dirtylimit.c
>>>>> index ab20da34bb..d7a855c603 100644
>>>>> --- a/system/dirtylimit.c
>>>>> +++ b/system/dirtylimit.c
>>>>> @@ -80,8 +80,7 @@ static void vcpu_dirty_rate_stat_collect(void)
>>>>>         int i = 0;
>>>>>         int64_t period = DIRTYLIMIT_CALC_TIME_MS;
>>>>>
>>>>> -    if (migrate_dirty_limit() &&
>>>>> -        migration_is_active()) {
>>>>> +    if (migrate_dirty_limit() && migration_is_running()) {
>>>>>             period = migrate_vcpu_dirty_limit_period();
>>>>>         }
>>>>>
>>>>> --
>>>>> 2.45.0
>>>>>
>>> --
>>> Peter Xu
>>>
> --
> Peter Xu
>

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Peter Xu]

On Mon, Oct 28, 2024 at 07:20:27PM +0200, Avihai Horon wrote:
> 
> On 28/10/2024 18:58, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
> > > On 28/10/2024 17:45, Peter Xu wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
> > > > > On 25/10/2024 0:30, Peter Xu wrote:
> > > > > > External email: Use caution opening links or attachments
> > > > > > 
> > > > > > 
> > > > > > We have two outside users of this API, so it's exported.
> > > > > > 
> > > > > > Is it really necessary?  Does it matter whether it must be
> > > > > > ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
> > > > > Actually for VFIO it does matter, because we don't want VFIO to do DPT
> > > > > log_sync in SETUP stage when DPT might not have been started yet.
> > > > > See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
> > > > > SETUP state").
> > > > This seems to be a known issue for migration in general, rather than VFIO
> > > > specific.  Hyman has a patch for it, not yet reviewed..
> > > > 
> > > > https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
> > > > 
> > > > That corresponds to your comment here:
> > > > 
> > > >       Redundant -- all RAM is marked dirty in migration SETUP state and is
> > > >       transferred only after migration is set to ACTIVE state, so doing
> > > >       log_sync during migration SETUP is pointless.
> > > > 
> > > > So I wonder whether it's only VFIO that should skip it, or log_sync()
> > > > simply shouldn't be called at all during SETUP, because of its redundancy.
> > > Not sure why this sync was there in the first place, but if its only purpose
> > > was to sync dirty pages then yes, I guess it be dropped.
> > > 
> > > > The other thing you mentioned here:
> > > > 
> > > >       Can fail -- there is a time window, between setting migration state to
> > > >       SETUP and starting dirty tracking by RAM save_live_setup handler, during
> > > >       which dirty tracking is still not started. Any VFIO log_sync call that
> > > >       is issued during this time window will fail. For example, this error can
> > > >       be triggered by migrating a VM when a GUI is active, which constantly
> > > >       calls log_sync.
> > > > 
> > > > This is VFIO specific.  Why this can fail even if global tracking is
> > > > started already?
> > > It can fail if global tracking is *not* started yet.
> > > As mentioned in the commit message, there is a time window where migration
> > > is in SETUP state but global tracking is not started yet.
> > Hmm, I'm totally confused now..
> > 
> > The only thing that can kickoff the sync during SETUP, AFAICT, is:
> > 
> >              ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> >              if (!ret) {
> >                  goto out_unlock;
> >              }
> >              migration_bitmap_sync_precopy(false);   <------------- here
> > 
> > I need to confess this may not be the right place to invoke it in ram.c (I
> > think we probably should move it out at some point.. into generic migration
> > code).  However I don't yet see why log_start() is not called first in your
> > case before log_sync().
> > 
> > Could you elaborate?
> 
> Indeed, in the above code log_start is called before log_sync.
> 
> I was referring to the case where some other code path triggers log_sync.
> E.g., if you open a VNC to the guest then it constantly calls log_sync to
> refresh the graphics. In that case, one of these log_syncs can happen
> between "migration status is set to SETUP" and "global tracking is started".

I see.  That's unfortunate..

Though this is also the case where it shouldn't be VFIO's problem alone.
See some other users of log_sync():

vhost_sync_dirty_bitmap():
    if (!dev->log_enabled || !dev->started) {
        return 0;
    }

kvm_slot_get_dirty_log():
    if (ret == -ENOENT) {
        /* kernel does not have dirty bitmap in this slot */
        ret = 0;
    }

And I didn't further look.

In short, IMHO looks like VFIO still shouldn't be special on differeciating
and make migration export the SETUP phase just for this..  as VFIO has
log_start() like all the rest, so VFIO can also know whether tracking is
enabled at all, then it can silently no-op the log_sync() like all the rest
of the users.

If you agree, I'd prefer we keep this patch - it'll be nice we only ever
expose migration_is_running() for migration status checks, without exposing
SETUP only for this VFIO use case even if it could have followed what other
modules are doing.

If you would like to propose a patch for VFIO, I'd be happy to include your
patch before this patch (just in case this patch could land some day) to
make sure VFIO works as before.  Since I don't have VFIO HW to test, it'll
be challenging for me to propose and test such patch otherwise.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 1/8] migration: Take migration object refcount earlier for threads

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Both migration thread or background snapshot thread will take a refcount of
> the migration object at the entrace of the thread function.
>
> That makes sense, because it protects the object from being freed by the
> main thread in migration_shutdown() later, but it might still race with it
> if the thread is scheduled too late.  Consider the case right after
> pthread_create() happened, VM shuts down with the object released, but
> right after that the migration thread finally got created, referencing
> MigrationState* in the opaque pointer which is already freed.
>
> The only 100% safe way to make sure it won't get freed is taking the
> refcount right before the thread is created, meanwhile when BQL is held.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [PATCH v3 2/8] migration: Unexport dirty_bitmap_mig_init()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> It's only used within migration/, so it shouldn't be exported.
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h | 3 ---
>  migration/migration.h    | 4 ++++
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index bfadc5613b..df57be6b5e 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -108,7 +108,4 @@ bool migration_incoming_postcopy_advised(void);
>  /* True if background snapshot is active */
>  bool migration_in_bg_snapshot(void);
>  
> -/* migration/block-dirty-bitmap.c */
> -void dirty_bitmap_mig_init(void);
> -
>  #endif
> diff --git a/migration/migration.h b/migration/migration.h
> index 7dc59c5e8d..0956e9274b 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -552,4 +552,8 @@ int migration_rp_wait(MigrationState *s);
>  void migration_rp_kick(MigrationState *s);
>  
>  void migration_bitmap_sync_precopy(bool last_stage);
> +
> +/* migration/block-dirty-bitmap.c */
> +void dirty_bitmap_mig_init(void);
> +
>  #endif

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

Re: [PATCH v3 3/8] migration: Unexport ram_mig_init()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> It's only used within migration/.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h | 1 -
>  migration/ram.h          | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index df57be6b5e..e8490e3af5 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -39,7 +39,6 @@ void precopy_add_notifier(NotifierWithReturn *n);
>  void precopy_remove_notifier(NotifierWithReturn *n);
>  int precopy_notify(PrecopyNotifyReason reason, Error **errp);
>  
> -void ram_mig_init(void);
>  void qemu_guest_free_page_hint(void *addr, size_t len);
>  bool migrate_ram_is_ignored(RAMBlock *block);
>  
> diff --git a/migration/ram.h b/migration/ram.h
> index bc0318b834..0d1981f888 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -44,6 +44,7 @@ extern XBZRLECacheStats xbzrle_counters;
>      INTERNAL_RAMBLOCK_FOREACH(block)                   \
>          if (!qemu_ram_is_migratable(block)) {} else
>  
> +void ram_mig_init(void);
>  int xbzrle_cache_resize(uint64_t new_size, Error **errp);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_total(void);

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

Re: [PATCH v3 4/8] migration: Drop migration_is_setup_or_active()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> This helper is mostly the same as migration_is_running(), except that one
> has COLO reported as true, the other has CANCELLING reported as true.
>
> Per my past years experience on the state changes, none of them should
> matter.
>
> To make it slightly safer, report both COLO || CANCELLING to be true in
> migration_is_running(), then drop the other one.  We kept the 1st only
> because the name is simpler, and clear enough.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [PATCH v3 5/8] migration: Drop migration_is_idle()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Now with the current migration_is_running(), it will report exactly the
> opposite of what will be reported by migration_is_idle().
>
> Drop migration_is_idle(), instead use "!migration_is_running()" which
> should be identical on functionality.
>
> In reality, most of the idle check is inverted, so it's even easier to
> write with "migrate_is_running()" check.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h |  1 -
>  hw/virtio/virtio-mem.c   |  2 +-
>  migration/migration.c    | 21 +--------------------
>  migration/ram.c          |  2 +-
>  system/qdev-monitor.c    |  4 ++--
>  5 files changed, 5 insertions(+), 25 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 86ef160f19..804eb23c06 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -53,7 +53,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>  void migration_object_init(void);
>  void migration_shutdown(void);
>  
> -bool migration_is_idle(void);
>  bool migration_is_active(void);
>  bool migration_is_device(void);
>  bool migration_is_running(void);
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index ae1e81d7ba..80ada89551 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -188,7 +188,7 @@ static bool virtio_mem_is_busy(void)
>       * after plugging them) until we're running on the destination (as we didn't
>       * migrate these blocks when they were unplugged).
>       */
> -    return migration_in_incoming_postcopy() || !migration_is_idle();
> +    return migration_in_incoming_postcopy() || migration_is_running();
>  }
>  
>  typedef int (*virtio_mem_range_cb)(VirtIOMEM *vmem, void *arg,
> diff --git a/migration/migration.c b/migration/migration.c
> index 3365195def..f86c709699 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1636,25 +1636,6 @@ bool migration_in_bg_snapshot(void)
>      return migrate_background_snapshot() && migration_is_running();
>  }
>  
> -bool migration_is_idle(void)
> -{
> -    MigrationState *s = current_migration;
> -
> -    if (!s) {
> -        return true;
> -    }
> -
> -    switch (s->state) {
> -    case MIGRATION_STATUS_NONE:
> -    case MIGRATION_STATUS_CANCELLED:
> -    case MIGRATION_STATUS_COMPLETED:
> -    case MIGRATION_STATUS_FAILED:
> -        return true;
> -    default:
> -        return false;
> -    }
> -}
> -
>  bool migration_is_active(void)
>  {
>      MigrationState *s = current_migration;
> @@ -1733,7 +1714,7 @@ static bool is_busy(Error **reasonp, Error **errp)
>      ERRP_GUARD();
>  
>      /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> -    if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
> +    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
>          error_propagate_prepend(errp, *reasonp,
>                                  "disallowing migration blocker "
>                                  "(migration/snapshot in progress) for: ");
> diff --git a/migration/ram.c b/migration/ram.c
> index 5646a0b882..504b48d584 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4498,7 +4498,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>          return;
>      }
>  
> -    if (!migration_is_idle()) {
> +    if (migration_is_running()) {
>          /*
>           * Precopy code on the source cannot deal with the size of RAM blocks
>           * changing at random points in time - especially after sending the
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 1310f35c9f..83fa684475 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -686,7 +686,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>          return NULL;
>      }
>  
> -    if (!migration_is_idle()) {
> +    if (migration_is_running()) {
>          error_setg(errp, "device_add not allowed while migrating");
>          return NULL;
>      }
> @@ -935,7 +935,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    if (!migration_is_idle() && !dev->allow_unplug_during_migration) {
> +    if (migration_is_running() && !dev->allow_unplug_during_migration) {
>          error_setg(errp, "device_del not allowed while migrating");
>          return;
>      }

Good.

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

Re: [PATCH v3 0/8] Migration: Make misc.h helpers available for whole VM lifecycle

[Peter Xu]

On Thu, Oct 24, 2024 at 05:30:48PM -0400, Peter Xu wrote:
> Based-on: <20241024165627.1372621-1-peterx@redhat.com>
> CI:       https://gitlab.com/peterx/qemu/-/pipelines/1511349805
> 
> This is a follow up of below patch from Avihai as a replacement:
> 
> https://lore.kernel.org/qemu-devel/20241020130108.27148-3-avihaih@nvidia.com/
> 
> This is v3 of the series, and it happened again: changelog doesn't make
> sense to compare to v2 because it's rewritten.  Meanwhile, now this series
> is based on the other TYPE_SINGLETON series I posted just now:
> 
> https://lore.kernel.org/r/20241024165627.1372621-1-peterx@redhat.com
> 
> It turns out I found more things to cleanup, as the versions spin up.
> 
> Patch 1:     I found that object_ref() in migration thread is actually also
>              racy..  this should fix it.
> 
> Patch 2-7:   It turns out I decided to clean things up first, then it'll
>              make my last patch easier on adding the mutex protection for
>              the current_migration reference
> 
> Patch 8:     The fix for NULL-dereference / race for the exported
>              functions.  VFIO can hit it only because VFIO's specialty
>              in using migration helpers in vmstate handlers, I guess.  I
>              found most functions always safe because even if they're used
>              outside migration/ they're most likely still invoked with
>              migration thread context.  So I found only two functions that
>              really need protections, exactly what VFIO is using.

I queued patch 1-5.

Ideally 6+7 can be a single patch, then we don't need to move DEVICE into
migration_is_active() at all.  Also that might break VFIO as Avihai pointed
out on VGA sync.  Avihai, I'd still appreciate if you could consider look
at vfio to behave like what kvm/vhost/.. is doing, by accepting log_sync()
before log_start().

Patch 8 still rely on singleton series, which will become rfc to be
reposted, so probably no rush on that.

> 
> Comments welcomed, thanks.
> 
> Peter Xu (8):
>   migration: Take migration object refcount earlier for threads
>   migration: Unexport dirty_bitmap_mig_init()
>   migration: Unexport ram_mig_init()
>   migration: Drop migration_is_setup_or_active()
>   migration: Drop migration_is_idle()
>   migration: Drop migration_is_device()
>   migration: Unexport migration_is_active()
>   migration: Protect updates to current_migration with a mutex
> 
>  include/migration/misc.h |  11 +---
>  migration/migration.h    |   8 +++
>  migration/ram.h          |   1 +
>  hw/vfio/common.c         |   6 +-
>  hw/virtio/virtio-mem.c   |   2 +-
>  migration/migration.c    | 118 +++++++++++++++++++--------------------
>  migration/ram.c          |   7 +--
>  net/vhost-vdpa.c         |   3 +-
>  system/dirtylimit.c      |   3 +-
>  system/qdev-monitor.c    |   4 +-
>  10 files changed, 81 insertions(+), 82 deletions(-)
> 
> -- 
> 2.45.0
> 

-- 
Peter Xu

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Avihai Horon]

On 28/10/2024 21:06, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Oct 28, 2024 at 07:20:27PM +0200, Avihai Horon wrote:
>> On 28/10/2024 18:58, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
>>>> On 28/10/2024 17:45, Peter Xu wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
>>>>>> On 25/10/2024 0:30, Peter Xu wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> We have two outside users of this API, so it's exported.
>>>>>>>
>>>>>>> Is it really necessary?  Does it matter whether it must be
>>>>>>> ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
>>>>>> Actually for VFIO it does matter, because we don't want VFIO to do DPT
>>>>>> log_sync in SETUP stage when DPT might not have been started yet.
>>>>>> See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
>>>>>> SETUP state").
>>>>> This seems to be a known issue for migration in general, rather than VFIO
>>>>> specific.  Hyman has a patch for it, not yet reviewed..
>>>>>
>>>>> https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
>>>>>
>>>>> That corresponds to your comment here:
>>>>>
>>>>>        Redundant -- all RAM is marked dirty in migration SETUP state and is
>>>>>        transferred only after migration is set to ACTIVE state, so doing
>>>>>        log_sync during migration SETUP is pointless.
>>>>>
>>>>> So I wonder whether it's only VFIO that should skip it, or log_sync()
>>>>> simply shouldn't be called at all during SETUP, because of its redundancy.
>>>> Not sure why this sync was there in the first place, but if its only purpose
>>>> was to sync dirty pages then yes, I guess it be dropped.
>>>>
>>>>> The other thing you mentioned here:
>>>>>
>>>>>        Can fail -- there is a time window, between setting migration state to
>>>>>        SETUP and starting dirty tracking by RAM save_live_setup handler, during
>>>>>        which dirty tracking is still not started. Any VFIO log_sync call that
>>>>>        is issued during this time window will fail. For example, this error can
>>>>>        be triggered by migrating a VM when a GUI is active, which constantly
>>>>>        calls log_sync.
>>>>>
>>>>> This is VFIO specific.  Why this can fail even if global tracking is
>>>>> started already?
>>>> It can fail if global tracking is *not* started yet.
>>>> As mentioned in the commit message, there is a time window where migration
>>>> is in SETUP state but global tracking is not started yet.
>>> Hmm, I'm totally confused now..
>>>
>>> The only thing that can kickoff the sync during SETUP, AFAICT, is:
>>>
>>>               ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
>>>               if (!ret) {
>>>                   goto out_unlock;
>>>               }
>>>               migration_bitmap_sync_precopy(false);   <------------- here
>>>
>>> I need to confess this may not be the right place to invoke it in ram.c (I
>>> think we probably should move it out at some point.. into generic migration
>>> code).  However I don't yet see why log_start() is not called first in your
>>> case before log_sync().
>>>
>>> Could you elaborate?
>> Indeed, in the above code log_start is called before log_sync.
>>
>> I was referring to the case where some other code path triggers log_sync.
>> E.g., if you open a VNC to the guest then it constantly calls log_sync to
>> refresh the graphics. In that case, one of these log_syncs can happen
>> between "migration status is set to SETUP" and "global tracking is started".
> I see.  That's unfortunate..
>
> Though this is also the case where it shouldn't be VFIO's problem alone.
> See some other users of log_sync():
>
> vhost_sync_dirty_bitmap():
>      if (!dev->log_enabled || !dev->started) {
>          return 0;
>      }
>
> kvm_slot_get_dirty_log():
>      if (ret == -ENOENT) {
>          /* kernel does not have dirty bitmap in this slot */
>          ret = 0;
>      }
>
> And I didn't further look.
>
> In short, IMHO looks like VFIO still shouldn't be special on differeciating
> and make migration export the SETUP phase just for this..  as VFIO has
> log_start() like all the rest, so VFIO can also know whether tracking is
> enabled at all, then it can silently no-op the log_sync() like all the rest
> of the users.
>
> If you agree, I'd prefer we keep this patch - it'll be nice we only ever
> expose migration_is_running() for migration status checks, without exposing
> SETUP only for this VFIO use case even if it could have followed what other
> modules are doing.

Yes, I agree this could be a nice cleanup.

>
> If you would like to propose a patch for VFIO, I'd be happy to include your
> patch before this patch (just in case this patch could land some day) to
> make sure VFIO works as before.  Since I don't have VFIO HW to test, it'll
> be challenging for me to propose and test such patch otherwise.

I can do that, though it may be a bit involved because VFIO has multiple 
dirty tracking mechanisms (legacy, device DPT and IOMMU DPT). Plus, I 
don't have HW that supports IOMMU DPT at hand for testing.
I assume this is not an urgent cleanup, right?

Thanks.

Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 6104 bytes --]

On Wed, Oct 30, 2024, 10:39 a.m. Avihai Horon <avihaih@nvidia.com> wrote:

>
> On 28/10/2024 21:06, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, Oct 28, 2024 at 07:20:27PM +0200, Avihai Horon wrote:
> >> On 28/10/2024 18:58, Peter Xu wrote:
> >>> External email: Use caution opening links or attachments
> >>>
> >>>
> >>> On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
> >>>> On 28/10/2024 17:45, Peter Xu wrote:
> >>>>> External email: Use caution opening links or attachments
> >>>>>
> >>>>>
> >>>>> On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
> >>>>>> On 25/10/2024 0:30, Peter Xu wrote:
> >>>>>>> External email: Use caution opening links or attachments
> >>>>>>>
> >>>>>>>
> >>>>>>> We have two outside users of this API, so it's exported.
> >>>>>>>
> >>>>>>> Is it really necessary?  Does it matter whether it must be
> >>>>>>> ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
> >>>>>> Actually for VFIO it does matter, because we don't want VFIO to do
> DPT
> >>>>>> log_sync in SETUP stage when DPT might not have been started yet.
> >>>>>> See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during
> migration
> >>>>>> SETUP state").
> >>>>> This seems to be a known issue for migration in general, rather than
> VFIO
> >>>>> specific.  Hyman has a patch for it, not yet reviewed..
> >>>>>
> >>>>> https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
> >>>>>
> >>>>> That corresponds to your comment here:
> >>>>>
> >>>>>        Redundant -- all RAM is marked dirty in migration SETUP state
> and is
> >>>>>        transferred only after migration is set to ACTIVE state, so
> doing
> >>>>>        log_sync during migration SETUP is pointless.
> >>>>>
> >>>>> So I wonder whether it's only VFIO that should skip it, or log_sync()
> >>>>> simply shouldn't be called at all during SETUP, because of its
> redundancy.
> >>>> Not sure why this sync was there in the first place, but if its only
> purpose
> >>>> was to sync dirty pages then yes, I guess it be dropped.
> >>>>
> >>>>> The other thing you mentioned here:
> >>>>>
> >>>>>        Can fail -- there is a time window, between setting migration
> state to
> >>>>>        SETUP and starting dirty tracking by RAM save_live_setup
> handler, during
> >>>>>        which dirty tracking is still not started. Any VFIO log_sync
> call that
> >>>>>        is issued during this time window will fail. For example,
> this error can
> >>>>>        be triggered by migrating a VM when a GUI is active, which
> constantly
> >>>>>        calls log_sync.
> >>>>>
> >>>>> This is VFIO specific.  Why this can fail even if global tracking is
> >>>>> started already?
> >>>> It can fail if global tracking is *not* started yet.
> >>>> As mentioned in the commit message, there is a time window where
> migration
> >>>> is in SETUP state but global tracking is not started yet.
> >>> Hmm, I'm totally confused now..
> >>>
> >>> The only thing that can kickoff the sync during SETUP, AFAICT, is:
> >>>
> >>>               ret =
> memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> >>>               if (!ret) {
> >>>                   goto out_unlock;
> >>>               }
> >>>               migration_bitmap_sync_precopy(false);   <-------------
> here
> >>>
> >>> I need to confess this may not be the right place to invoke it in
> ram.c (I
> >>> think we probably should move it out at some point.. into generic
> migration
> >>> code).  However I don't yet see why log_start() is not called first in
> your
> >>> case before log_sync().
> >>>
> >>> Could you elaborate?
> >> Indeed, in the above code log_start is called before log_sync.
> >>
> >> I was referring to the case where some other code path triggers
> log_sync.
> >> E.g., if you open a VNC to the guest then it constantly calls log_sync
> to
> >> refresh the graphics. In that case, one of these log_syncs can happen
> >> between "migration status is set to SETUP" and "global tracking is
> started".
> > I see.  That's unfortunate..
> >
> > Though this is also the case where it shouldn't be VFIO's problem alone.
> > See some other users of log_sync():
> >
> > vhost_sync_dirty_bitmap():
> >      if (!dev->log_enabled || !dev->started) {
> >          return 0;
> >      }
> >
> > kvm_slot_get_dirty_log():
> >      if (ret == -ENOENT) {
> >          /* kernel does not have dirty bitmap in this slot */
> >          ret = 0;
> >      }
> >
> > And I didn't further look.
> >
> > In short, IMHO looks like VFIO still shouldn't be special on
> differeciating
> > and make migration export the SETUP phase just for this..  as VFIO has
> > log_start() like all the rest, so VFIO can also know whether tracking is
> > enabled at all, then it can silently no-op the log_sync() like all the
> rest
> > of the users.
> >
> > If you agree, I'd prefer we keep this patch - it'll be nice we only ever
> > expose migration_is_running() for migration status checks, without
> exposing
> > SETUP only for this VFIO use case even if it could have followed what
> other
> > modules are doing.
>
> Yes, I agree this could be a nice cleanup.
>
> >
> > If you would like to propose a patch for VFIO, I'd be happy to include
> your
> > patch before this patch (just in case this patch could land some day) to
> > make sure VFIO works as before.  Since I don't have VFIO HW to test,
> it'll
> > be challenging for me to propose and test such patch otherwise.
>
> I can do that, though it may be a bit involved because VFIO has multiple
> dirty tracking mechanisms (legacy, device DPT and IOMMU DPT). Plus, I
> don't have HW that supports IOMMU DPT at hand for testing.
> I assume this is not an urgent cleanup, right?
>

Yes there's no rush.

If you agree with the change, feel free to clean this helper up when you
post the vfio changes.  We can still have one more helper exported before
that.

Thanks a lot.


> Thanks.
>
>

[-- Attachment #2: Type: text/html, Size: 8295 bytes --]

Re: [PATCH v3 8/8] migration: Protect updates to current_migration with a mutex

[Cédric Le Goater]

On 10/28/24 16:50, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 02:50:36PM +0200, Cédric Le Goater wrote:
>> On 10/24/24 23:30, Peter Xu wrote:
>>> Introduce migration_mutex, protecting concurrent updates to
>>> current_migration.
>>>
>>> In reality, most of the exported migration functions are safe to access
>>> migration objects on capabilities, etc., e.g. many of the code is invoked
>>> within migration thread via different hooks (e.g., precopy notifier,
>>> vmstate handler hooks, etc.).
>>>
>>> So literally the mutex so far only makes sure below two APIs that are prone
>>> to accessing freed current_migration:
>>>
>>>           migration_is_running()
>>>           migration_file_set_error()
>>>
>>> Then we'll need to take the mutex too when init/free the migration object.
>>
>> we should also drop :
>>
>>    static void vfio_set_migration_error(int ret)
>>    {
>>        if (migration_is_running()) {
>>            migration_file_set_error(ret, NULL);
>>        }
>>    }
>>
>> and use directly migration_file_set_error().
> 
> We'll need to export migration_is_running() anyway, though.  So maybe we
> can do that as a VFIO's follow up?

sure. Let's add that to the list, along with the 'log_enabled' bool that
you discussed with Avihai in the previous patch :

  [PATCH v3 7/8] migration: Unexport migration_is_active()

These changes would be good to have in QEMU 9.2.

Avihai, I have access to HW supporting HW dirty tracking.

Thanks,

C.