On Fri, Sep 30, 2016 at 11:45:19PM +0100, Alex Bennée wrote:
>
> Jonathan Neuschäfer <j.neuschae...@gmx.net> writes:
>
> > 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?
>
> 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
signature.asc
Description: PGP signature
