| Message ID | 20200213225109.13120-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | testing and plugin updates | expand |
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 <alex.bennee@linaro.org> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> 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
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++;
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(-) -- 2.20.1