[PATCH v1 0/5] testing/next (rcutorture and misc tweaks)

[PATCH v1 0/5] testing/next (rcutorture and misc tweaks)

[Alex Bennée]

Hi,

The current state of testing/next is mostly a rubber duck exercise as
I tried to figure out what might cause the rcutorture test to fail.
The only real practical change is asserting we are never about to
reset and existing active RCU structure. So far I haven't seen the
rcutorture crash occur since (although git stability seems to be a
current problem for Travis).

Alex Bennée (4):
  tests/tcg: include a skip runner for pauth3 with plugins
  tests/rcutorture: update usage hint
  tests/rcutorture: better document locking of stats
  tests/rcutorture: mild documenting refactor of update thread

Thomas Huth (1):
  travis.yml: Test the s390-ccw build, too

 tests/rcutorture.c                        | 55 +++++++++++++++++------
 .travis.yml                               | 10 +++++
 tests/tcg/aarch64/Makefile.softmmu-target |  2 +
 3 files changed, 53 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH v1 1/5] tests/tcg: include a skip runner for pauth3 with plugins

[Alex Bennée]

If we have plugins enabled we still need to have built the test to be
able to run it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/Makefile.softmmu-target | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index d2299b98b76..71f72cfbe34 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -70,4 +70,6 @@ pauth-3:
 	$(call skip-test, "BUILD of $@", "missing compiler support")
 run-pauth-3:
 	$(call skip-test, "RUN of pauth-3", "not built")
+run-plugin-pauth-3-with-%:
+	$(call skip-test, "RUN of pauth-3 ($*)", "not built")
 endif
-- 
2.20.1

[PATCH v1 2/5] tests/rcutorture: update usage hint

[Alex Bennée]

Although documented in the comments we don't display all the various
invocations we can in the usage.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/rcutorture.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index 49311c82ea4..e8b2169e7dd 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -413,7 +413,8 @@ static void gtest_stress_10_5(void)
 
 static void usage(int argc, char *argv[])
 {
-    fprintf(stderr, "Usage: %s [nreaders [ perf | stress ] ]\n", argv[0]);
+    fprintf(stderr, "Usage: %s [nreaders [ [r|u]perf | stress [duration]]\n",
+            argv[0]);
     exit(-1);
 }
 
-- 
2.20.1

[PATCH v1 3/5] tests/rcutorture: better document locking of stats

[Alex Bennée]

This is pure code motion with no functional effect.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/rcutorture.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index e8b2169e7dd..256d24ed5ba 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -65,8 +65,6 @@
 #include "qemu/rcu.h"
 #include "qemu/thread.h"
 
-long long n_reads = 0LL;
-long n_updates = 0L;
 int nthreadsrunning;
 
 #define GOFLAG_INIT 0
@@ -78,11 +76,20 @@ static volatile int goflag = GOFLAG_INIT;
 #define RCU_READ_RUN 1000
 
 #define NR_THREADS 100
-static QemuMutex counts_mutex;
 static QemuThread threads[NR_THREADS];
 static struct rcu_reader_data *data[NR_THREADS];
 static int n_threads;
 
+/*
+ * Statistical counts
+ *
+ * These are the sum of local counters at the end of a run.
+ * Updates are protected by a mutex.
+ */
+static QemuMutex counts_mutex;
+long long n_reads = 0LL;
+long n_updates = 0L;
+
 static void create_thread(void *(*func)(void *))
 {
     if (n_threads >= NR_THREADS) {
@@ -230,8 +237,9 @@ struct rcu_stress {
 struct rcu_stress rcu_stress_array[RCU_STRESS_PIPE_LEN] = { { 0 } };
 struct rcu_stress *rcu_stress_current;
 int rcu_stress_idx;
-
 int n_mberror;
+
+/* Updates protected by counts_mutex */
 long long rcu_stress_count[RCU_STRESS_PIPE_LEN + 1];
 
 
-- 
2.20.1

[PATCH v1 4/5] tests/rcutorture: mild documenting refactor of update thread

[Alex Bennée]

This is mainly to help with reasoning what the test is trying to do.
We can move rcu_stress_idx to a local variable as there is only ever
one updater thread. I've also added an assert to catch the case where
we end up updating the current structure to itself which is the only
way I can see the mberror cases we are seeing on Travis.

We shall see if the rcutorture test failures go away now.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/rcutorture.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index 256d24ed5ba..9559f13cd1f 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -236,7 +236,6 @@ struct rcu_stress {
 
 struct rcu_stress rcu_stress_array[RCU_STRESS_PIPE_LEN] = { { 0 } };
 struct rcu_stress *rcu_stress_current;
-int rcu_stress_idx;
 int n_mberror;
 
 /* Updates protected by counts_mutex */
@@ -288,29 +287,48 @@ static void *rcu_read_stress_test(void *arg)
     return NULL;
 }
 
+/*
+ * Stress Test Updater
+ *
+ * The updater cycles around updating rcu_stress_current to point at
+ * one of the rcu_stress_array_entries and resets it's pipe_count. It
+ * then increments pipe_count of all the other entries. The pipe_count
+ * will be read under an rcu_read_lock() and distribution of values
+ * calculated. The final result gives an indication of how many
+ * previously current rcu_stress entries are in flight until the RCU
+ * cycle complete.
+ */
 static void *rcu_update_stress_test(void *arg)
 {
-    int i;
-    struct rcu_stress *p;
+    int i, rcu_stress_idx = 0;
+    struct rcu_stress *cp = atomic_read(&rcu_stress_current);
 
     rcu_register_thread();
-
     *(struct rcu_reader_data **)arg = &rcu_reader;
+
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
     }
+
     while (goflag == GOFLAG_RUN) {
-        i = rcu_stress_idx + 1;
-        if (i >= RCU_STRESS_PIPE_LEN) {
-            i = 0;
+        struct rcu_stress *p;
+        rcu_stress_idx++;
+        if (rcu_stress_idx >= RCU_STRESS_PIPE_LEN) {
+            rcu_stress_idx = 0;
         }
-        p = &rcu_stress_array[i];
+        p = &rcu_stress_array[rcu_stress_idx];
+        /* catching up with ourselves would be a bug */
+        assert(p != cp);
         p->mbtest = 0;
         smp_mb();
         p->pipe_count = 0;
         p->mbtest = 1;
         atomic_rcu_set(&rcu_stress_current, p);
-        rcu_stress_idx = i;
+        cp = p;
+        /*
+         * New RCU structure is now live, update pipe counts on old
+         * ones.
+         */
         for (i = 0; i < RCU_STRESS_PIPE_LEN; i++) {
             if (i != rcu_stress_idx) {
                 rcu_stress_array[i].pipe_count++;
-- 
2.20.1

[PATCH v1 5/5] travis.yml: Test the s390-ccw build, too

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

Since we can now use a s390x host on Travis, we can also build and
test the s390-ccw bios images there. For this we have to make sure
that roms/SLOF is checked out, too, and then move the generated *.img
files to the right location before running the tests.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200206202543.7085-1-thuth@redhat.com>
---
 .travis.yml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 6c0ec6cf69f..c0eeff9caa9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -508,6 +508,16 @@ matrix:
       env:
         - TEST_CMD="make check check-tcg V=1"
         - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
+      script:
+        - ( cd ${SRC_DIR} ; git submodule update --init roms/SLOF )
+        - BUILD_RC=0 && make -j${JOBS} || BUILD_RC=$?
+        - |
+          if [ "$BUILD_RC" -eq 0 ] ; then
+              mv pc-bios/s390-ccw/*.img pc-bios/ ;
+              ${TEST_CMD} ;
+          else
+              $(exit $BUILD_RC);
+          fi
 
     # Release builds
     # The make-release script expect a QEMU version, so our tag must start with a 'v'.
-- 
2.20.1

Re: [PATCH v1 0/5] testing/next (rcutorture and misc tweaks)

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> The current state of testing/next is mostly a rubber duck exercise as
> I tried to figure out what might cause the rcutorture test to fail.
> The only real practical change is asserting we are never about to
> reset and existing active RCU structure. So far I haven't seen the
> rcutorture crash occur since (although git stability seems to be a
> current problem for Travis).
>
> Alex Bennée (4):
>   tests/tcg: include a skip runner for pauth3 with plugins
>   tests/rcutorture: update usage hint
>   tests/rcutorture: better document locking of stats
>   tests/rcutorture: mild documenting refactor of update thread
>
> Thomas Huth (1):
>   travis.yml: Test the s390-ccw build, too
>
>  tests/rcutorture.c                        | 55 +++++++++++++++++------
>  .travis.yml                               | 10 +++++
>  tests/tcg/aarch64/Makefile.softmmu-target |  2 +
>  3 files changed, 53 insertions(+), 14 deletions(-)

Ping?

I'm particularly interested in the rcutorture changes which so far have
not yielded any:

  ERROR:/home/travis/build/qemu/qemu/tests/rcutorture.c:384:gtest_stress: assertion failed (n_mberror == 0): (1 == 0)

in my various test trees.

-- 
Alex Bennée

Re: [PATCH v1 2/5] tests/rcutorture: update usage hint

[Philippe Mathieu-Daudé]

On 2/7/20 12:39 PM, Alex Bennée wrote:
> Although documented in the comments we don't display all the various
> invocations we can in the usage.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/rcutorture.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/rcutorture.c b/tests/rcutorture.c
> index 49311c82ea4..e8b2169e7dd 100644
> --- a/tests/rcutorture.c
> +++ b/tests/rcutorture.c
> @@ -413,7 +413,8 @@ static void gtest_stress_10_5(void)
>   
>   static void usage(int argc, char *argv[])
>   {
> -    fprintf(stderr, "Usage: %s [nreaders [ perf | stress ] ]\n", argv[0]);
> +    fprintf(stderr, "Usage: %s [nreaders [ [r|u]perf | stress [duration]]\n",
> +            argv[0]);
>       exit(-1);
>   }
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v1 3/5] tests/rcutorture: better document locking of stats

[Philippe Mathieu-Daudé]

On 2/7/20 12:39 PM, Alex Bennée wrote:
> This is pure code motion with no functional effect.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/rcutorture.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/rcutorture.c b/tests/rcutorture.c
> index e8b2169e7dd..256d24ed5ba 100644
> --- a/tests/rcutorture.c
> +++ b/tests/rcutorture.c
> @@ -65,8 +65,6 @@
>   #include "qemu/rcu.h"
>   #include "qemu/thread.h"
>   
> -long long n_reads = 0LL;
> -long n_updates = 0L;
>   int nthreadsrunning;
>   
>   #define GOFLAG_INIT 0
> @@ -78,11 +76,20 @@ static volatile int goflag = GOFLAG_INIT;
>   #define RCU_READ_RUN 1000
>   
>   #define NR_THREADS 100
> -static QemuMutex counts_mutex;
>   static QemuThread threads[NR_THREADS];
>   static struct rcu_reader_data *data[NR_THREADS];
>   static int n_threads;
>   
> +/*
> + * Statistical counts
> + *
> + * These are the sum of local counters at the end of a run.
> + * Updates are protected by a mutex.
> + */
> +static QemuMutex counts_mutex;
> +long long n_reads = 0LL;
> +long n_updates = 0L;
> +
>   static void create_thread(void *(*func)(void *))
>   {
>       if (n_threads >= NR_THREADS) {
> @@ -230,8 +237,9 @@ struct rcu_stress {
>   struct rcu_stress rcu_stress_array[RCU_STRESS_PIPE_LEN] = { { 0 } };
>   struct rcu_stress *rcu_stress_current;
>   int rcu_stress_idx;
> -
>   int n_mberror;
> +
> +/* Updates protected by counts_mutex */
>   long long rcu_stress_count[RCU_STRESS_PIPE_LEN + 1];
>   
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v1 5/5] travis.yml: Test the s390-ccw build, too

[Philippe Mathieu-Daudé]

On 2/7/20 12:39 PM, Alex Bennée wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> Since we can now use a s390x host on Travis, we can also build and
> test the s390-ccw bios images there. For this we have to make sure
> that roms/SLOF is checked out, too, and then move the generated *.img
> files to the right location before running the tests.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200206202543.7085-1-thuth@redhat.com>
> ---
>   .travis.yml | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 6c0ec6cf69f..c0eeff9caa9 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -508,6 +508,16 @@ matrix:
>         env:
>           - TEST_CMD="make check check-tcg V=1"
>           - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
> +      script:
> +        - ( cd ${SRC_DIR} ; git submodule update --init roms/SLOF )
> +        - BUILD_RC=0 && make -j${JOBS} || BUILD_RC=$?
> +        - |
> +          if [ "$BUILD_RC" -eq 0 ] ; then
> +              mv pc-bios/s390-ccw/*.img pc-bios/ ;
> +              ${TEST_CMD} ;
> +          else
> +              $(exit $BUILD_RC);
> +          fi
>   
>       # Release builds
>       # The make-release script expect a QEMU version, so our tag must start with a 'v'.
> 

Already reviewed/tested [*] with comment:

Maybe remove the trailing ", too" in subject...

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg677641.html

Re: [PATCH v1 5/5] travis.yml: Test the s390-ccw build, too

[Cornelia Huck]

On Wed, 12 Feb 2020 21:48:42 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 2/7/20 12:39 PM, Alex Bennée wrote:
> > From: Thomas Huth <thuth@redhat.com>
> > 
> > Since we can now use a s390x host on Travis, we can also build and
> > test the s390-ccw bios images there. For this we have to make sure
> > that roms/SLOF is checked out, too, and then move the generated *.img
> > files to the right location before running the tests.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Message-Id: <20200206202543.7085-1-thuth@redhat.com>
> > ---
> >   .travis.yml | 10 ++++++++++
> >   1 file changed, 10 insertions(+)

> Already reviewed/tested [*] with comment:
> 
> Maybe remove the trailing ", too" in subject...
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg677641.html
> 

Hm, I also gave an

Acked-by: Cornelia Huck <cohuck@redhat.com>