diff mbox series

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

Message ID 20240226093832.1468383-6-npiggin@gmail.com (mailing list archive)
State New, archived
Headers show
Series more migration enhancements and tests | expand

Commit Message

Nicholas Piggin Feb. 26, 2024, 9:38 a.m. UTC
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(-)

Comments

Thomas Huth March 4, 2024, 6:17 a.m. UTC | #1
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
Andrew Jones March 4, 2024, 9:19 a.m. UTC | #2
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. 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

Thanks,
drew
Nicholas Piggin March 5, 2024, 2:47 a.m. UTC | #3
On Mon Mar 4, 2024 at 4:17 PM AEST, 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)?

Good point. Will do.

Thanks,
Nick
Nicholas Piggin March 5, 2024, 2:58 a.m. UTC | #4
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
diff mbox series

Patch

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
+			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
@@ -218,7 +224,7 @@  do_migration ()
 
 	# The test must prompt the user to migrate, so wait for the
 	# "Now migrate VM" or similar console message.
-	while ! seen_migrate_msg ${src_out} ; do
+	while [[ ${continuous_migration} -eq 0 ]] && ! seen_migrate_msg ${src_out} ; do
 		if ! ps -p ${live_pid} > /dev/null ; then
 			echo > ${dst_infifo}
 			qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
@@ -229,12 +235,32 @@  do_migration ()
 		sleep 0.1
 	done
 
+	if grep -q "Begin continuous migration" < ${src_out} ; then
+		if [[ ${continuous_migration} -eq 1 ]] ; then
+			echo > ${dst_infifo}
+			qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
+			echo "ERROR: Continuous migration already begun." >&2
+			qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
+			return 3
+		fi
+		continuous_migration=1
+		echo > ${src_infifo}
+	fi
+
 	# Wait until the destination has created the incoming and qmp sockets
 	while ! [ -S ${dst_incoming} ] ; do sleep 0.1 ; done
 	while ! [ -S ${dst_qmp} ] ; do sleep 0.1 ; done
 
 	if [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then
 		# May not get any migrations, exit to main loop for now...
+		# No migrations today, shut down dst in an orderly manner...
+		if [[ ${continuous_migration} -eq 1 ]] ; then
+			echo > ${dst_infifo}
+			qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
+			echo "ERROR: Can't skip in continuous migration." >&2
+			qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
+			return 3
+		fi
 		echo > ${dst_infifo}
 		qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
 		echo > ${src_infifo} # Resume src and carry on.
@@ -266,8 +292,23 @@  do_migration ()
 
 	qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
 
-	# keypress to dst so getchar completes and test continues
-	echo > ${dst_infifo}
+	# Should we end continuous migration?
+	if grep -q "End continuous migration" < ${src_out} ; then
+		if [[ ${continuous_migration} -eq 0 ]] ; then
+			echo "ERROR: Can't end continuous migration when not started." >&2
+			echo > ${dst_infifo}
+			qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
+			qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
+			return 3
+		fi
+		continuous_migration=0
+		echo > ${src_infifo}
+	fi
+
+	if [[ ${continuous_migration} -eq 0 ]]; then
+		# keypress to dst so getchar completes and test continues
+		echo > ${dst_infifo}
+	fi
 
 	# Ensure the incoming socket is removed, ready for next destination
 	if [ -S ${dst_incoming} ] ; then