nodes[id].next is written by other threads. If atomic_read is not used
(matching atomic_set in mcs_mutex_lock!) the compiler can optimize the
whole "if" away!
Reported-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
tests/test-aio-multithread.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
index f11e990..8b0b40e 100644
--- a/tests/test-aio-multithread.c
+++ b/tests/test-aio-multithread.c
@@ -309,7 +309,7 @@ static void mcs_mutex_lock(void)
static void mcs_mutex_unlock(void)
{
int next;
- if (nodes[id].next == -1) {
+ if (atomic_read(&nodes[id].next) == -1) {
if (atomic_read(&mutex_head) == id &&
atomic_cmpxchg(&mutex_head, id, -1) == id) {
/* Last item in the list, exit. */
@@ -323,7 +323,7 @@ static void mcs_mutex_unlock(void)
}
/* Wake up the next in line. */
- next = nodes[id].next;
+ next = atomic_read(&nodes[id].next);
nodes[next].locked = 0;
qemu_futex_wake(&nodes[next].locked, 1);
}
--
2.9.3
On Mon, 27 Feb 2017 12:17:26 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > nodes[id].next is written by other threads. If atomic_read is not used > (matching atomic_set in mcs_mutex_lock!) the compiler can optimize the > whole "if" away! > > Reported-by: Alex Bennée <alex.bennee@linaro.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- Cool ! I can use travis again :) Tested-by: Greg Kurz <groug@kaod.org> > tests/test-aio-multithread.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c > index f11e990..8b0b40e 100644 > --- a/tests/test-aio-multithread.c > +++ b/tests/test-aio-multithread.c > @@ -309,7 +309,7 @@ static void mcs_mutex_lock(void) > static void mcs_mutex_unlock(void) > { > int next; > - if (nodes[id].next == -1) { > + if (atomic_read(&nodes[id].next) == -1) { > if (atomic_read(&mutex_head) == id && > atomic_cmpxchg(&mutex_head, id, -1) == id) { > /* Last item in the list, exit. */ > @@ -323,7 +323,7 @@ static void mcs_mutex_unlock(void) > } > > /* Wake up the next in line. */ > - next = nodes[id].next; > + next = atomic_read(&nodes[id].next); > nodes[next].locked = 0; > qemu_futex_wake(&nodes[next].locked, 1); > }
On 27 February 2017 at 12:01, Greg Kurz <groug@kaod.org> wrote: > On Mon, 27 Feb 2017 12:17:26 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> nodes[id].next is written by other threads. If atomic_read is not used >> (matching atomic_set in mcs_mutex_lock!) the compiler can optimize the >> whole "if" away! >> >> Reported-by: Alex Bennée <alex.bennee@linaro.org> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- > > Cool ! I can use travis again :) > > Tested-by: Greg Kurz <groug@kaod.org> > Thanks; applied to master as a build fix. -- PMM
On Mon, Feb 27, 2017 at 12:17:26PM +0100, Paolo Bonzini wrote: > nodes[id].next is written by other threads. If atomic_read is not used > (matching atomic_set in mcs_mutex_lock!) the compiler can optimize the > whole "if" away! > > Reported-by: Alex Bennée <alex.bennee@linaro.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/test-aio-multithread.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
© 2016 - 2024 Red Hat, Inc.