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@xxxxxxxxx>
> > > ---
> > > 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
