Re: [PATCH 3/8] migration: Add precopy initial data loaded ACK functionality

[Avihai Horon]

On 10/05/2023 11:54, Juan Quintela wrote:
External email: Use caution opening links or attachments


Avihai Horon <avih...@nvidia.com> wrote:
Add the core functionality of precopy initial data, which allows the
destination to ACK that initial data has been loaded and the source to
wait for this ACK before completing the migration.

A new return path command MIG_RP_MSG_INITIAL_DATA_LOADED_ACK is added.
It is sent by the destination after precopy initial data is loaded to
ACK to the source that precopy initial data has been loaded.

In addition, two new SaveVMHandlers handlers are added:
1. is_initial_data_active which indicates whether precopy initial data
    is used for this migration user (i.e., SaveStateEntry).
2. initial_data_loaded which indicates whether precopy initial data has
    been loaded by this migration user.

Signed-off-by: Avihai Horon <avih...@nvidia.com>

@@ -1401,6 +1412,8 @@ void migrate_init(MigrationState *s)
      s->vm_was_running = false;
      s->iteration_initial_bytes = 0;
      s->threshold_size = 0;
+
+    s->initial_data_loaded_acked = false;
In general, you let a blank line for the stuff you add, when all the
previous fields don't do that.  Can you remove it.

Sure, I will remove them.


@@ -2704,6 +2725,20 @@ static void migration_update_counters(MigrationState *s,
                                bandwidth, s->threshold_size);
  }

+static bool initial_data_loaded_acked(MigrationState *s)
+{
+    if (!migrate_precopy_initial_data()) {
+        return true;
+    }
+
+    /* No reason to wait for precopy initial data loaded ACK if VM is stopped 
*/
+    if (!runstate_is_running()) {
+        return true;
+    }
Thinking loud here.

What happens if we start a migration.  Guest is running.
We enable precopy_initial_data().

And then we stop the guest.

Are we going to receive data that expect this return false?  Or it is
handled somewhere else?

Not sure I fully understand what you mean here, could you elaborate?

@@ -2719,6 +2754,7 @@ static MigIterateState 
migration_iteration_run(MigrationState *s)
  {
      uint64_t must_precopy, can_postcopy;
      bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
+    bool initial_data_loaded = initial_data_loaded_acked(s);

      qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy);
      uint64_t pending_size = must_precopy + can_postcopy;
@@ -2731,7 +2767,8 @@ static MigIterateState 
migration_iteration_run(MigrationState *s)
          trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy);
      }

-    if (!pending_size || pending_size < s->threshold_size) {
+    if ((!pending_size || pending_size < s->threshold_size) &&
+        initial_data_loaded) {
          trace_migration_thread_low_pending(pending_size);
          migration_completion(s);
          return MIG_ITERATE_BREAK;
For this specific variable, I think I am going to add something more
general that this can piggy back.

For the migration tests I need exactly this functionality.  I want
migration to run until the test decided that it is a good idea to
finish.  I.e. For testing xbzrle I need at least three iterations, to
test auto_converge I need a minimum of 13 iterations.  And I am going to
do exactly what you have done here.

Will came back to you after I think something.

Oh, cool. So I will keep this as is in the meantime.


@@ -2739,7 +2776,7 @@ static MigIterateState 
migration_iteration_run(MigrationState *s)

      /* Still a significant amount to transfer */
      if (!in_postcopy && must_precopy <= s->threshold_size &&
-        qatomic_read(&s->start_postcopy)) {
+        initial_data_loaded && qatomic_read(&s->start_postcopy)) {
          if (postcopy_start(s)) {
              error_report("%s: postcopy failed to start", __func__);
          }
diff --git a/migration/savevm.c b/migration/savevm.c
index 2740defdf0..7a94deda3b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2504,6 +2504,39 @@ static int loadvm_process_command(QEMUFile *f)
      return 0;
  }

+static int qemu_loadvm_initial_data_loaded_ack(MigrationIncomingState *mis)
+{
+    SaveStateEntry *se;
+    int ret;
+
+    if (!mis->initial_data_enabled || mis->initial_data_loaded_ack_sent) {
+        return 0;
+    }
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        if (!se->ops || !se->ops->initial_data_loaded) {
+            continue;
+        }
+
+        if (!se->ops->is_initial_data_active ||
+            !se->ops->is_initial_data_active(se->opaque)) {
+            continue;
+        }
If you don't have any other use for is_initial_data_active() I think you
can integrate the functionality with initial_data_loaded().

If it is not active just return 1?

Yes it's possible.
However, if we change this to a notification mechanism as Peter 
suggested then I think both is_initial_data_active() and 
initial_data_loaded() can be dropped.

Thanks.


+
+        if (!se->ops->initial_data_loaded(se->opaque)) {
+            return 0;
+        }
+    }
+
+    ret = migrate_send_rp_initial_data_loaded_ack(mis);
+    if (!ret) {
+        mis->initial_data_loaded_ack_sent = true;
+        trace_loadvm_initial_data_loaded_acked();
+    }
+
+    return ret;
+}
Later, Juan