Re: [kvm-unit-tests PATCH 5/7] arch-run: Add a "continuous" migration option for tests

[Thomas Huth <thuth@redhat.com>]

On 26/02/2024 10.38, Nicholas Piggin wrote:
> The cooperative migration protocol is very good to control precise
> pre and post conditions for a migration event. However in some cases
> its intrusiveness to the test program, can mask problems and make
> analysis more difficult.
> 
> For example to stress test migration vs concurrent complicated
> memory access, including TLB refill, ram dirtying, etc., then the
> tight spin at getchar() and resumption of the workload after
> migration is unhelpful.
> 
> This adds a continuous migration mode that directs the harness to
> perform migrations continually. This is added to the migration
> selftests, which also sees cooperative migration iterations reduced
> to avoid increasing test time too much.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   common/selftest-migration.c | 16 +++++++++--
>   lib/migrate.c               | 18 ++++++++++++
>   lib/migrate.h               |  3 ++
>   scripts/arch-run.bash       | 55 ++++++++++++++++++++++++++++++++-----
>   4 files changed, 82 insertions(+), 10 deletions(-)
> 
> diff --git a/common/selftest-migration.c b/common/selftest-migration.c
> index 0afd8581c..9a9b61835 100644
> --- a/common/selftest-migration.c
> +++ b/common/selftest-migration.c
> @@ -9,12 +9,13 @@
>    */
>   #include <libcflat.h>
>   #include <migrate.h>
> +#include <asm/time.h>
>   
> -#define NR_MIGRATIONS 30
> +#define NR_MIGRATIONS 15
>   
>   int main(int argc, char **argv)
>   {
> -	report_prefix_push("migration");
> +	report_prefix_push("migration harness");
>   
>   	if (argc > 1 && !strcmp(argv[1], "skip")) {
>   		migrate_skip();
> @@ -24,7 +25,16 @@ int main(int argc, char **argv)
>   
>   		for (i = 0; i < NR_MIGRATIONS; i++)
>   			migrate_quiet();
> -		report(true, "simple harness stress");
> +		report(true, "cooperative migration");
> +
> +		migrate_begin_continuous();
> +		mdelay(2000);
> +		migrate_end_continuous();
> +		mdelay(1000);
> +		migrate_begin_continuous();
> +		mdelay(2000);
> +		migrate_end_continuous();
> +		report(true, "continuous migration");
>   	}
>   
>   	report_prefix_pop();
> diff --git a/lib/migrate.c b/lib/migrate.c
> index 1d22196b7..770f76d5c 100644
> --- a/lib/migrate.c
> +++ b/lib/migrate.c
> @@ -60,3 +60,21 @@ void migrate_skip(void)
>   	puts("Skipped VM migration (quiet)\n");
>   	(void)getchar();
>   }
> +
> +void migrate_begin_continuous(void)
> +{
> +	puts("Begin continuous migration\n");
> +	(void)getchar();
> +}
> +
> +void migrate_end_continuous(void)
> +{
> +	/*
> +	 * Migration can split this output between source and dest QEMU
> +	 * output files, print twice and match once to always cope with
> +	 * a split.
> +	 */
> +	puts("End continuous migration\n");
> +	puts("End continuous migration (quiet)\n");
> +	(void)getchar();
> +}
> diff --git a/lib/migrate.h b/lib/migrate.h
> index db6e0c501..35b6703a2 100644
> --- a/lib/migrate.h
> +++ b/lib/migrate.h
> @@ -11,3 +11,6 @@ void migrate_quiet(void);
>   void migrate_once(void);
>   
>   void migrate_skip(void);
> +
> +void migrate_begin_continuous(void);
> +void migrate_end_continuous(void);
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index d0f6f098f..5c7e72036 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -125,15 +125,17 @@ qmp_events ()
>   filter_quiet_msgs ()
>   {
>   	grep -v "Now migrate the VM (quiet)" |
> +	grep -v "Begin continuous migration (quiet)" |
> +	grep -v "End continuous migration (quiet)" |
>   	grep -v "Skipped VM migration (quiet)"
>   }
>   
>   seen_migrate_msg ()
>   {
>   	if [ $skip_migration -eq 1 ]; then
> -		grep -q -e "Now migrate the VM" < $1
> +	        grep -q -e "Now migrate the VM" -e "Begin continuous migration" < $1
>   	else
> -		grep -q -e "Now migrate the VM" -e "Skipped VM migration" < $1
> +	        grep -q -e "Now migrate the VM" -e "Begin continuous migration" -e "Skipped VM migration" < $1
>   	fi
>   }
>   
> @@ -161,6 +163,7 @@ run_migration ()
>   	src_qmpout=/dev/null
>   	dst_qmpout=/dev/null
>   	skip_migration=0
> +	continuous_migration=0
>   
>   	mkfifo ${src_outfifo}
>   	mkfifo ${dst_outfifo}
> @@ -186,9 +189,12 @@ run_migration ()
>   	do_migration || return $?
>   
>   	while ps -p ${live_pid} > /dev/null ; do
> -		# Wait for test exit or further migration messages.
> -		if ! seen_migrate_msg ${src_out} ;  then
> +		if [[ ${continuous_migration} -eq 1 ]] ; then

Here you're using "[[" for testing ...

> +			do_migration || return $?
> +		elif ! seen_migrate_msg ${src_out} ;  then
>   			sleep 0.1
> +		elif grep -q "Begin continuous migration" < ${src_out} ; then
> +			do_migration || return $?
>   		elif grep -q "Now migrate the VM" < ${src_out} ; then
>   			do_migration || return $?
>   		elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then

... while the other code seems to use "[" for testing values. Can we try to 
stick to one style, please (unless it's really required to use "[[" somewhere)?

  Thanks,
   Thomas