Re: [PATCH v2 08/17] migration: Add load_finish handler and associated functions

[Maciej S. Szmigiero]

On 5.09.2024 17:13, Avihai Horon wrote:

On 27/08/2024 20:54, Maciej S. Szmigiero wrote:

External email: Use caution opening links or attachments


From: "Maciej S. Szmigiero" <maciej.szmigi...@oracle.com>

load_finish SaveVMHandler allows migration code to poll whether
a device-specific asynchronous device state loading operation had finished.

In order to avoid calling this handler needlessly the device is supposed
to notify the migration code of its possible readiness via a call to
qemu_loadvm_load_finish_ready_broadcast() while holding
qemu_loadvm_load_finish_ready_lock.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
  include/migration/register.h | 21 +++++++++++++++
  migration/migration.c        |  6 +++++
  migration/migration.h        |  3 +++
  migration/savevm.c           | 52 ++++++++++++++++++++++++++++++++++++
  migration/savevm.h           |  4 +++
  5 files changed, 86 insertions(+)

diff --git a/include/migration/register.h b/include/migration/register.h
index 4a578f140713..44d8cf5192ae 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -278,6 +278,27 @@ typedef struct SaveVMHandlers {
      int (*load_state_buffer)(void *opaque, char *data, size_t data_size,
                               Error **errp);

+    /**
+     * @load_finish
+     *
+     * Poll whether all asynchronous device state loading had finished.
+     * Not called on the load failure path.
+     *
+     * Called while holding the qemu_loadvm_load_finish_ready_lock.
+     *
+     * If this method signals "not ready" then it might not be called
+     * again until qemu_loadvm_load_finish_ready_broadcast() is invoked
+     * while holding qemu_loadvm_load_finish_ready_lock.
+     *
+     * @opaque: data pointer passed to register_savevm_live()
+     * @is_finished: whether the loading had finished (output parameter)
+     * @errp: pointer to Error*, to store an error if it happens.
+     *
+     * Returns zero to indicate success and negative for error
+     * It's not an error that the loading still hasn't finished.
+     */
+    int (*load_finish)(void *opaque, bool *is_finished, Error **errp);
+
      /**
       * @load_setup
       *
diff --git a/migration/migration.c b/migration/migration.c
index 3dea06d57732..d61e7b055e07 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -259,6 +259,9 @@ void migration_object_init(void)

      current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;

+    qemu_mutex_init(&current_incoming->load_finish_ready_mutex);
+    qemu_cond_init(&current_incoming->load_finish_ready_cond);
+
      migration_object_check(current_migration, &error_fatal);

      ram_mig_init();
@@ -410,6 +413,9 @@ void migration_incoming_state_destroy(void)
          mis->postcopy_qemufile_dst = NULL;
      }

+    qemu_mutex_destroy(&mis->load_finish_ready_mutex);
+    qemu_cond_destroy(&mis->load_finish_ready_cond);
+
      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
  }

diff --git a/migration/migration.h b/migration/migration.h
index 38aa1402d516..4e2443e6c8ec 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -230,6 +230,9 @@ struct MigrationIncomingState {

      /* Do exit on incoming migration failure */
      bool exit_on_error;
+
+    QemuCond load_finish_ready_cond;
+    QemuMutex load_finish_ready_mutex;
  };

  MigrationIncomingState *migration_incoming_get_current(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 3fde5ca8c26b..33c9200d1e78 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -3022,6 +3022,37 @@ int qemu_loadvm_state(QEMUFile *f)
          return ret;
      }

+    qemu_loadvm_load_finish_ready_lock();
+    while (!ret) { /* Don't call load_finish() handlers on the load failure 
path */
+        bool all_ready = true;

Nit: Maybe rename all_ready to all_finished to be consistent with load_finish() 
terminology? Same for this_ready.

Will rename it accordingly.

+        SaveStateEntry *se = NULL;
+
+        QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+            bool this_ready;
+
+            if (!se->ops || !se->ops->load_finish) {
+                continue;
+            }
+
+            ret = se->ops->load_finish(se->opaque, &this_ready, &local_err);
+            if (ret) {
+                error_report_err(local_err);
+
+                qemu_loadvm_load_finish_ready_unlock();
+                return -EINVAL;
+            } else if (!this_ready) {
+                all_ready = false;
+            }
+        }
+
+        if (all_ready) {
+            break;
+        }
+
+        qemu_cond_wait(&mis->load_finish_ready_cond, 
&mis->load_finish_ready_mutex);
+    }
+    qemu_loadvm_load_finish_ready_unlock();
+
      if (ret == 0) {
          ret = qemu_file_get_error(f);
      }
@@ -3126,6 +3157,27 @@ int qemu_loadvm_load_state_buffer(const char *idstr, 
uint32_t instance_id,
      return 0;
  }

+void qemu_loadvm_load_finish_ready_lock(void)
+{
+    MigrationIncomingState *mis = migration_incoming_get_current();
+
+    qemu_mutex_lock(&mis->load_finish_ready_mutex);
+}
+
+void qemu_loadvm_load_finish_ready_unlock(void)
+{
+    MigrationIncomingState *mis = migration_incoming_get_current();
+
+    qemu_mutex_unlock(&mis->load_finish_ready_mutex);
+}
+
+void qemu_loadvm_load_finish_ready_broadcast(void)
+{
+    MigrationIncomingState *mis = migration_incoming_get_current();
+
+    qemu_cond_broadcast(&mis->load_finish_ready_cond);

Do we need a broadcast? isn't signal enough as we only have one waiter thread?

Currently, there's just one waiter but looking at the relatively small
implementation difference between pthread_cond_signal() and
pthread_cond_broadcast() I'm not sure whether it is worth changing it
it to _signal() and not having a possibility of signalling multiple
waiters upfront.

Thanks.

Thanks,
Maciej