Re: [kvm-unit-tests PATCH v2 1/1] s390x: add parallel skey migration test

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

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@linux.ibm.com> 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@linux.ibm.com>
> > > > > ---
> > > > >  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.
> > 
>