Re: [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence

[Jonathan Neuschäfer]

On Fri, Sep 30, 2016 at 11:45:19PM +0100, Alex Bennée wrote:
> 
> Jonathan Neuschäfer <address@hidden> writes:
> 
> > On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote:
> >> From: Paolo Bonzini <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> >> Signed-off-by: Alex Bennée <address@hidden>
> >> ---
> >>  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