On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote: > From: Paolo Bonzini <pbonz...@redhat.com> > > There is a data race if the sequence is written concurrently to the > read. In C11 this has undefined behavior. Use atomic_set; the > read side is already using atomic_read. > > Reported-by: Alex Bennée <alex.ben...@linaro.org> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > Signed-off-by: Alex Bennée <alex.ben...@linaro.org> > --- > include/qemu/seqlock.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h > index 2e2be4c..8dee11d 100644 > --- a/include/qemu/seqlock.h > +++ b/include/qemu/seqlock.h > @@ -31,7 +31,7 @@ static inline void seqlock_init(QemuSeqLock *sl) > /* Lock out other writers and update the count. */ > static inline void seqlock_write_begin(QemuSeqLock *sl) > { > - ++sl->sequence; > + atomic_set(&sl->sequence, sl->sequence + 1);
I'm not an atomics expert, but as I read the code, the load of sl->sequence (on the right side) isn't atomic relative to the store (atomic_set). Should this be atomic_inc(&sl->sequence) instead, or am I missing something? In particular, I'm worried about this situation: thread 0 thread 1 seqlock_write_begin: seqlock_write_begin: unsigned tmp = sl->sequence unsigned tmp = sl->sequence tmp += 1 tmp += 1 atomic_set(&sl->sequence, tmp) atomic_set(&sl->sequence, tmp) ... where sl->sequence will effectively only be incremented once (as far as I can see). Regards, Jonathan Neuschäfer
signature.asc
Description: PGP signature