[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 04/19] tests/rcutorture: mild documenting refactor of upda
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH v2 04/19] tests/rcutorture: mild documenting refactor of update thread |
Date: |
Fri, 14 Feb 2020 10:18:11 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
On 13/02/20 23:50, Alex Bennée wrote:
> 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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Yes, the failures are quire mysterious and I agree that they can only happen if:
1) p == cp in your patch below
2) the memory barrier can be overtaken by the store above it.
Even then, (2) would be unlikely because then the compiler would
coalesce the two stores to p->mbtest. However we could add a patch such
as this to rule it out:
diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index 9559f13..969a19a 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -260,7 +260,7 @@ static void *rcu_read_stress_test(void *arg)
while (goflag == GOFLAG_RUN) {
rcu_read_lock();
p = atomic_rcu_read(&rcu_stress_current);
- if (p->mbtest == 0) {
+ if (atomic_read(&p->mbtest) == 0) {
n_mberror++;
}
rcu_read_lock();
@@ -268,7 +268,7 @@ static void *rcu_read_stress_test(void *arg)
garbage++;
}
rcu_read_unlock();
- pc = p->pipe_count;
+ pc = atomic_read(&p->pipe_count);
rcu_read_unlock();
if ((pc > RCU_STRESS_PIPE_LEN) || (pc < 0)) {
pc = RCU_STRESS_PIPE_LEN;
@@ -319,10 +319,10 @@ static void *rcu_update_stress_test(void *arg)
p = &rcu_stress_array[rcu_stress_idx];
/* catching up with ourselves would be a bug */
assert(p != cp);
- p->mbtest = 0;
+ atomic_set(&p->mbtest, 0);
smp_mb();
- p->pipe_count = 0;
- p->mbtest = 1;
+ atomic_set(&p->pipe_count, 0);
+ atomic_set(&p->mbtest, 1);
atomic_rcu_set(&rcu_stress_current, p);
cp = p;
/*
@@ -331,7 +331,8 @@ static void *rcu_update_stress_test(void *arg)
*/
for (i = 0; i < RCU_STRESS_PIPE_LEN; i++) {
if (i != rcu_stress_idx) {
- rcu_stress_array[i].pipe_count++;
+ atomic_set(&rcu_stress_array[i].pipe_count,
+ rcu_stress_array[i].pipe_count + 1);
}
}
synchronize_rcu();
Finally, the "pipe_count" naming is a bit mysterious. The idea behind the test
is that RCU can only ever see the current or the previous version of a
struct (in this case, the current or the previous index) and a "pipe_count"
of 3 means for example that the index is 3 updates behind the current
index. To check that the RCU invariant is preserved, the test checks that
the reader does not observe an index that is 2 updates or more behind the
current index.
I have never changed it because I took it directly from Paul McKenney's
rcutorture and I didn't want to deviate too much in order to keep Paul's
code as a reference. But perhaps we should rename it to something more
intuitive like "age" (which is short enough that "pc" could also be
renamed to "age").
Paolo
- [PATCH v2 00/19] testing and plugin updates, Alex Bennée, 2020/02/13
- [PATCH v2 01/19] tests/tcg: include a skip runner for pauth3 with plugins, Alex Bennée, 2020/02/13
- [PATCH v2 02/19] tests/rcutorture: update usage hint, Alex Bennée, 2020/02/13
- [PATCH v2 03/19] tests/rcutorture: better document locking of stats, Alex Bennée, 2020/02/13
- [PATCH v2 05/19] travis.yml: Test the s390-ccw build, too, Alex Bennée, 2020/02/13
- [PATCH v2 06/19] travis.yml: Fix Travis YAML configuration warnings, Alex Bennée, 2020/02/13
- [PATCH v2 04/19] tests/rcutorture: mild documenting refactor of update thread, Alex Bennée, 2020/02/13
- Re: [PATCH v2 04/19] tests/rcutorture: mild documenting refactor of update thread,
Paolo Bonzini <=
- [PATCH v2 07/19] travis.yml: single-thread build-tcg stages, Alex Bennée, 2020/02/13
- [PATCH v2 08/19] tests/iotests: be a little more forgiving on the size test, Alex Bennée, 2020/02/13
- [PATCH v2 13/19] qemu/bitops.h: Add extract8 and extract16, Alex Bennée, 2020/02/13
- [PATCH v2 10/19] docs/devel: document query handle lifetimes, Alex Bennée, 2020/02/13
- [PATCH v2 11/19] plugins/core: add missing break in cb_to_tcg_flags, Alex Bennée, 2020/02/13
- [PATCH v2 09/19] tracing: only allow -trace to override -D if set, Alex Bennée, 2020/02/13
- [PATCH v2 12/19] tests/plugin: prevent uninitialized warning, Alex Bennée, 2020/02/13