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

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon Mar 4, 2024 at 7:19 PM AEST, Andrew Jones wrote:
> On Mon, Mar 04, 2024 at 07:17:35AM +0100, Thomas Huth wrote:
> > 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)?
> >
>
> We should decide on a Bash coding style and on preferences like [[ and
> then write a document for it, as well as create a set of shellcheck
> includes/excludes to test it. Then, using shellcheck we'd change all our
> current Bash code and also require shellcheck to pass on all new code
> before merge.

Seems like a good idea.

> Any volunteers for that effort? For the style selection
> we can take inspiration from other projects or even just adopt their
> style guides. Google has some guidance[1][2] and googling for Bash style
> pops up other hits.
>
> [1] https://google.github.io/styleguide/shellguide.html
> [2] https://chromium.googlesource.com/chromiumos/docs/+/master/styleguide/shell.md

My bash skills consit of mashing the keyboard until the errors quieten
down, so I may not be the best to decide on this stuff. I could take a
look at getting the checker running in make and post an RFC though. No
promises if it turns out to be harder than it looks...

Thanks,
Nick