From: Peter Xu <peterx@redhat.com>
To: qemu-devel@nongnu.org
Cc: "Avihai Horon" <avihaih@nvidia.com>,
"Cédric Le Goater" <clg@redhat.com>,
peterx@redhat.com, "Fabiano Rosas" <farosas@suse.de>,
"Alex Williamson" <alex.williamson@redhat.com>
Subject: [PATCH v3 8/8] migration: Protect updates to current_migration with a mutex
Date: Thu, 24 Oct 2024 17:30:56 -0400 [thread overview]
Message-ID: <20241024213056.1395400-9-peterx@redhat.com> (raw)
In-Reply-To: <20241024213056.1395400-1-peterx@redhat.com>
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
next prev parent reply other threads:[~2024-10-24 21:32 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-24 21:30 [PATCH v3 0/8] Migration: Make misc.h helpers available for whole VM lifecycle Peter Xu
2024-10-24 21:30 ` [PATCH v3 1/8] migration: Take migration object refcount earlier for threads Peter Xu
2024-10-25 8:36 ` Cédric Le Goater
2024-10-29 18:48 ` Fabiano Rosas
2024-10-24 21:30 ` [PATCH v3 2/8] migration: Unexport dirty_bitmap_mig_init() Peter Xu
2024-10-29 18:48 ` Fabiano Rosas
2024-10-24 21:30 ` [PATCH v3 3/8] migration: Unexport ram_mig_init() Peter Xu
2024-10-25 8:36 ` Cédric Le Goater
2024-10-29 18:49 ` Fabiano Rosas
2024-10-24 21:30 ` [PATCH v3 4/8] migration: Drop migration_is_setup_or_active() Peter Xu
2024-10-25 8:59 ` Cédric Le Goater
2024-10-29 19:04 ` Fabiano Rosas
2024-10-24 21:30 ` [PATCH v3 5/8] migration: Drop migration_is_idle() Peter Xu
2024-10-25 12:39 ` Cédric Le Goater
2024-10-29 19:07 ` Fabiano Rosas
2024-10-24 21:30 ` [PATCH v3 6/8] migration: Drop migration_is_device() Peter Xu
2024-10-24 21:30 ` [PATCH v3 7/8] migration: Unexport migration_is_active() Peter Xu
2024-10-28 7:43 ` Avihai Horon
2024-10-28 15:45 ` Peter Xu
2024-10-28 16:41 ` Avihai Horon
2024-10-28 16:58 ` Peter Xu
2024-10-28 17:20 ` Avihai Horon
2024-10-28 19:06 ` Peter Xu
2024-10-30 14:39 ` Avihai Horon
2024-10-30 14:43 ` Peter Xu
2024-10-24 21:30 ` Peter Xu [this message]
2024-10-25 12:50 ` [PATCH v3 8/8] migration: Protect updates to current_migration with a mutex Cédric Le Goater
2024-10-28 15:50 ` Peter Xu
2024-11-04 15:26 ` Cédric Le Goater
2024-10-29 20:22 ` [PATCH v3 0/8] Migration: Make misc.h helpers available for whole VM lifecycle Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241024213056.1395400-9-peterx@redhat.com \
--to=peterx@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=avihaih@nvidia.com \
--cc=clg@redhat.com \
--cc=farosas@suse.de \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).