On Tue, 2022-12-13 at 09:50 +0100, Nico Boehr 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?
Well, if the condition is false on the first iteration, there won't be a second one.
>
> > 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?
Fine by me. Could also use READ_ONCE for result. You decide.
Btw, doesn't checkpatch complain about mb() without comment?
Although I think I've ignored that before, too.
