On Tue, 2022-12-13 at 13:14 +0100, Claudio Imbrenda wrote:
> 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 :)
Well, ok, depends what you consider part of the coding style then :)
Since k-u-t uses kernel style. I run it and then ignore what I judge reasonable to ignore :)
>
> > Although I think I've ignored that before, too.
> >
>
