| Message ID | 20160930213106.20186-6-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote: > From: Paolo Bonzini <pbonzini@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.bennee@linaro.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Alex Bennée <alex.bennee@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
Jonathan Neuschäfer <j.neuschaefer@gmx.net> writes: > On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote: >> From: Paolo Bonzini <pbonzini@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.bennee@linaro.org> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Alex Bennée <alex.bennee@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? There can only be one seqlock_write going on at a time (that is protected by a lock). The atomic_set only ensures that the seqlock_read side unambiguously sees one value or the other - you don't need to use an atomic_inc in this case. > > 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 -- Alex Bennée
On Fri, Sep 30, 2016 at 11:45:19PM +0100, Alex Bennée wrote: > > Jonathan Neuschäfer <j.neuschaefer@gmx.net> writes: > > > On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote: > >> From: Paolo Bonzini <pbonzini@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.bennee@linaro.org> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Alex Bennée <alex.bennee@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? > > There can only be one seqlock_write going on at a time (that is > protected by a lock). The atomic_set only ensures that the seqlock_read > side unambiguously sees one value or the other - you don't need to use > an atomic_inc in this case. Ah, good, that makes sense. Jonathan Neuschäfer
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); /* Write sequence before updating other fields. */ smp_wmb(); @@ -42,7 +42,7 @@ static inline void seqlock_write_end(QemuSeqLock *sl) /* Write other fields before finalizing sequence. */ smp_wmb(); - ++sl->sequence; + atomic_set(&sl->sequence, sl->sequence + 1); } static inline unsigned seqlock_read_begin(QemuSeqLock *sl)