On 12/4/2023 3:49 PM, Peter Xu wrote:
> 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.sist...@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, suspend not seen() should not be called for suspend
> test at all, I guess, because we know it won't.
Calling migrate_wait_for_dirty_mem proves that a source write is propagated
to the dest, even for the suspended case. We fully expect that, but a good
test verifies our expectations. That is done in the first loop of
migrate_wait_for_dirty_mem. After that, we must check for the suspended
state, because the second loop will not terminate. Here is a more explicit
version:
static void migrate_wait_for_dirty_mem(QTestState *from,
QTestState *to)
{
uint64_t watch_address = start_address + MAGIC_OFFSET_BASE;
uint64_t marker_address = start_address + MAGIC_OFFSET;
uint8_t watch_byte;
/*
* Wait for the MAGIC_MARKER to get transferred, as an
* indicator that a migration pass has made some known
* amount of progress.
*/
do {
usleep(1000 * 10);
} while (qtest_readq(to, marker_address) != MAGIC_MARKER);
+ /* If suspended, src only iterates once, and watch_byte may never change */
+ if (src_state.suspend_me) {
+ return;
+ }
>> }
>>
>>
>> @@ -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!
Agreed.
> 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).
Yes, it played that role. I will delete all the existing calls to
wait_for_suspended,
and add them after wait_for_serial("src_serial") in test_precopy_common and
migrate_postcopy_prepare.
- Steve
>> 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,
>
