Re: [PATCH V6 11/14] tests/qtest: precopy migration with suspend

[Peter Xu <peterx@redhat.com>]

On Thu, Nov 30, 2023 at 01:37:24PM -0800, Steve Sistare wrote:
> Add a test case to verify that the suspended state is handled correctly
> during live migration precopy.  The test suspends the src, migrates, then
> wakes the dest.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  tests/qtest/migration-helpers.c |  3 ++
>  tests/qtest/migration-helpers.h |  2 ++
>  tests/qtest/migration-test.c    | 64 ++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index fd3b94e..37e8e81 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -32,6 +32,9 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>      if (g_str_equal(name, "STOP")) {
>          state->stop_seen = true;
>          return true;
> +    } else if (g_str_equal(name, "SUSPEND")) {
> +        state->suspend_seen = true;
> +        return true;
>      } else if (g_str_equal(name, "RESUME")) {
>          state->resume_seen = true;
>          return true;
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 3d32699..b478549 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -18,6 +18,8 @@
>  typedef struct QTestMigrationState {
>      bool stop_seen;
>      bool resume_seen;
> +    bool suspend_seen;
> +    bool suspend_me;
>  } QTestMigrationState;
>  
>  bool migrate_watch_for_events(QTestState *who, const char *name,
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index e10d5a4..200f023 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -178,7 +178,7 @@ static void bootfile_delete(void)
>  /*
>   * Wait for some output in the serial output file,
>   * we get an 'A' followed by an endless string of 'B's
> - * but on the destination we won't have the A.
> + * but on the destination we won't have the A (unless we enabled suspend/resume)
>   */
>  static void wait_for_serial(const char *side)
>  {
> @@ -245,6 +245,13 @@ static void wait_for_resume(QTestState *who, QTestMigrationState *state)
>      }
>  }
>  
> +static void wait_for_suspend(QTestState *who, QTestMigrationState *state)
> +{
> +    if (!state->suspend_seen) {
> +        qtest_qmp_eventwait(who, "SUSPEND");
> +    }
> +}
> +
>  /*
>   * It's tricky to use qemu's migration event capability with qtest,
>   * events suddenly appearing confuse the qmp()/hmp() responses.
> @@ -299,7 +306,7 @@ static void wait_for_migration_pass(QTestState *who)
>  {
>      uint64_t pass, prev_pass = 0, changes = 0;
>  
> -    while (changes < 2 && !src_state.stop_seen) {
> +    while (changes < 2 && !src_state.stop_seen && !src_state.suspend_seen) {
>          usleep(1000);
>          pass = get_migration_pass(who);
>          changes += (pass != prev_pass);
> @@ -595,7 +602,8 @@ static void migrate_wait_for_dirty_mem(QTestState *from,
>      watch_byte = qtest_readb(from, watch_address);
>      do {
>          usleep(1000 * 10);
> -    } while (qtest_readb(from, watch_address) == watch_byte);
> +    } while (qtest_readb(from, watch_address) == watch_byte &&
> +             !src_state.suspend_seen);

This is hackish to me.

AFAIU the guest code won't ever dirty anything after printing the initial
'B'.  IOW, migrate_wait_for_dirty_mem() should not be called for suspend
test at all, I guess, because we know it won't.

>  }
>  
>  
> @@ -771,6 +779,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>      dst_state = (QTestMigrationState) { };
>      src_state = (QTestMigrationState) { };
>      bootfile_create(tmpfs, args->suspend_me);
> +    src_state.suspend_me = args->suspend_me;
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          memory_size = "150M";
> @@ -1730,6 +1739,9 @@ static void test_precopy_common(MigrateCommon *args)
>           * change anything.
>           */
>          if (args->result == MIG_TEST_SUCCEED) {
> +            if (src_state.suspend_me) {
> +                wait_for_suspend(from, &src_state);
> +            }
>              qtest_qmp_assert_success(from, "{ 'execute' : 'stop'}");
>              wait_for_stop(from, &src_state);
>              migrate_ensure_converge(from);
> @@ -1777,6 +1789,9 @@ static void test_precopy_common(MigrateCommon *args)
>               */
>              wait_for_migration_complete(from);
>  
> +            if (src_state.suspend_me) {
> +                wait_for_suspend(from, &src_state);
> +            }

Here it's pretty much uneasy to follow too, waiting for SUSPEND to come,
even after migration has already completed!

I suspect it never waits, since suspend_seen is normally always already
set (with the above hack, migrate_wait_for_dirty_mem() plays the role of
waiting for SUSPENDED).

>              wait_for_stop(from, &src_state);
>  
>          } else {

IMHO it'll be cleaner to explicitly wait for SUSPENDED when we know
migration is not yet completed.  Then, we know we're migrating with
SUSPENDED.  In summary, perhaps something squashed like this?

====8<====
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 30d4b32a35..65e6692716 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -605,8 +605,7 @@ static void migrate_wait_for_dirty_mem(QTestState *from,
     watch_byte = qtest_readb(from, watch_address);
     do {
         usleep(1000 * 10);
-    } while (qtest_readb(from, watch_address) == watch_byte &&
-             !src_state.suspend_seen);
+    } while (qtest_readb(from, watch_address) == watch_byte);
 }
 
 
@@ -1805,7 +1804,12 @@ static void test_precopy_common(MigrateCommon *args)
                 wait_for_migration_pass(from);
                 args->iterations--;
             }
-            migrate_wait_for_dirty_mem(from, to);
+
+            if (src_state.suspend_me) {
+                wait_for_suspend(from, &src_state);
+            } else {
+                migrate_wait_for_dirty_mem(from, to);
+            }
 
             migrate_ensure_converge(from);
 
@@ -1814,10 +1818,6 @@ static void test_precopy_common(MigrateCommon *args)
              * hanging forever if migration didn't converge
              */
             wait_for_migration_complete(from);
-
-            if (src_state.suspend_me) {
-                wait_for_suspend(from, &src_state);
-            }
             wait_for_stop(from, &src_state);
 
         } else {
====8<====

I didn't check the postcopy patch, but I'd expect something similar would
be nice.

Thanks,

-- 
Peter Xu