On Tue, 13 Dec 2022 12:11:29 +0100
Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> wrote:
> 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?
there is no checkpatch for kvm unit tests :)
> Although I think I've ignored that before, too.
>
