| Message ID | 1426952091-18019-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 21 March 2015 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote: > 32-bit PPC cannot do atomic operations on long long. Inside the loops, > we are already using local counters that are summed at the end of > the run---with one exception in rcu_read_stress_test: fix it to use > the same technique. Then, use a mutex to protect the global counts. > Performance does not matter there because every thread will only enter > the critical section once. > > Remaining uses of atomic instructions are for ints or pointers. I don't suppose there's a way to make the atomic functions enforce that 'not for anything that larger than pointer type', is there? It would be nice if this kind of bug caused compile failures on all 32-bit systems rather than only ppc-32... -- PMM
Am 21.03.2015 um 16:34 schrieb Paolo Bonzini: > 32-bit PPC cannot do atomic operations on long long. Inside the loops, > we are already using local counters that are summed at the end of > the run---with one exception in rcu_read_stress_test: fix it to use > the same technique. Then, use a mutex to protect the global counts. > Performance does not matter there because every thread will only enter > the critical section once. > > Remaining uses of atomic instructions are for ints or pointers. > > Reported-by: Andreas Faerber <afaerber@suse.de> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/rcutorture.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) Tested-by: Andreas Färber <afaerber@suse.de> This lets rcutorture pass, but the same error occurs in test-rcu-list... Btw any reason rcutorture does not fit one of the three naming schemes? Regards, Andreas
On 21/03/2015 16:44, Peter Maydell wrote: > On 21 March 2015 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote: >> 32-bit PPC cannot do atomic operations on long long. Inside the loops, >> we are already using local counters that are summed at the end of >> the run---with one exception in rcu_read_stress_test: fix it to use >> the same technique. Then, use a mutex to protect the global counts. >> Performance does not matter there because every thread will only enter >> the critical section once. >> >> Remaining uses of atomic instructions are for ints or pointers. > > I don't suppose there's a way to make the atomic functions > enforce that 'not for anything that larger than pointer type', > is there? It would be nice if this kind of bug caused compile > failures on all 32-bit systems rather than only ppc-32... Yes, it should be possible (e.g. with __builtin_choose_expr or with a statement expression that includes QEMU_BUILD_BUG_ON). Paolo
diff --git a/tests/rcutorture.c b/tests/rcutorture.c index 60a2ccf..d6b304d 100644 --- a/tests/rcutorture.c +++ b/tests/rcutorture.c @@ -82,6 +82,7 @@ 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; @@ -130,7 +131,9 @@ static void *rcu_read_perf_test(void *arg) } n_reads_local += RCU_READ_RUN; } - atomic_add(&n_reads, n_reads_local); + qemu_mutex_lock(&counts_mutex); + n_reads += n_reads_local; + qemu_mutex_unlock(&counts_mutex); rcu_unregister_thread(); return NULL; @@ -151,7 +154,9 @@ static void *rcu_update_perf_test(void *arg) synchronize_rcu(); n_updates_local++; } - atomic_add(&n_updates, n_updates_local); + qemu_mutex_lock(&counts_mutex); + n_updates += n_updates_local; + qemu_mutex_unlock(&counts_mutex); rcu_unregister_thread(); return NULL; @@ -241,6 +246,7 @@ static void *rcu_read_stress_test(void *arg) struct rcu_stress *p; int pc; long long n_reads_local = 0; + long long rcu_stress_local[RCU_STRESS_PIPE_LEN + 1] = { 0 }; volatile int garbage = 0; rcu_register_thread(); @@ -265,13 +271,18 @@ static void *rcu_read_stress_test(void *arg) if ((pc > RCU_STRESS_PIPE_LEN) || (pc < 0)) { pc = RCU_STRESS_PIPE_LEN; } - atomic_inc(&rcu_stress_count[pc]); + rcu_stress_local[pc]++; n_reads_local++; if ((++itercnt % 0x1000) == 0) { synchronize_rcu(); } } - atomic_add(&n_reads, n_reads_local); + qemu_mutex_lock(&counts_mutex); + n_reads += n_reads_local; + for (i = 0; i <= RCU_STRESS_PIPE_LEN; i++) { + rcu_stress_count[i] += rcu_stress_local[i]; + } + qemu_mutex_unlock(&counts_mutex); rcu_unregister_thread(); return NULL; @@ -419,6 +430,7 @@ int main(int argc, char *argv[]) int nreaders = 1; int duration = 1; + qemu_mutex_init(&counts_mutex); if (argc >= 2 && argv[1][0] == '-') { g_test_init(&argc, &argv, NULL); if (g_test_quick()) {
32-bit PPC cannot do atomic operations on long long. Inside the loops, we are already using local counters that are summed at the end of the run---with one exception in rcu_read_stress_test: fix it to use the same technique. Then, use a mutex to protect the global counts. Performance does not matter there because every thread will only enter the critical section once. Remaining uses of atomic instructions are for ints or pointers. Reported-by: Andreas Faerber <afaerber@suse.de> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tests/rcutorture.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)