On Tue, 13 Dec 2022 09:50:06 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
> Quoting Nina Schoetterl-Glausch (2022-12-12 21:37:28)
> > On Fri, 2022-12-09 at 11:21 +0100, Nico Boehr wrote:
> > > Right now, we have a test which sets storage keys, then migrates the VM
> > > and - after migration finished - verifies the skeys are still there.
> > >
> > > Add a new version of the test which changes storage keys while the
> > > migration is in progress. This is achieved by adding a command line
> > > argument to the existing migration-skey test.
> > >
> > > Signed-off-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
> > > ---
> > > s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> > > s390x/unittests.cfg | 15 ++-
> > > 2 files changed, 198 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > > index b7bd82581abe..9b9a45f4ad3b 100644
> > > --- a/s390x/migration-skey.c
> > > +++ b/s390x/migration-skey.c
> > >
> > [...]
> >
> > > +static void test_skey_migration_parallel(void)
> > > +{
> > > + report_prefix_push("parallel");
> > > +
> > > + if (smp_query_num_cpus() == 1) {
> > > + report_skip("need at least 2 cpus for this test");
> > > + goto error;
> > > + }
> > > +
> > > + smp_cpu_setup(1, PSW_WITH_CUR_MASK(set_skeys_thread));
> > > +
> > > + migrate_once();
> > > +
> > > + WRITE_ONCE(thread_should_exit, 1);
> > > +
> > > + while (!thread_exited)
> > > + mb();
> >
> > Are you doing it this way instead of while(!READ_ONCE(thread_exited)); so the mb() does double duty
> > and ensures "result" is also read from memory a couple of lines down?
>
> It is a good point, actually I just did what we already do in wait_for_flag in s390x/smp.c. :-)
>
> > If so, I wonder if the compiler is allowed to arrange the control flow such that if the loop condition
> > is false on the first iteration it uses a cached value of "result" (I'd be guessing yes, but what do I know).
>
> I agree, but it does not matter, does it? At latest the second iteration will actually read from memory, no?
>
> > In any case using a do while loop instead would eliminate the question.
> > A comment might be nice, too.
>
> How about I change to
> while(!READ_ONCE(thread_exited));
> and add an explicit mb() below to ensure result is read from memory?
yes please
