qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jonathan Neuschäfer
Subject: Re: [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence
Date: Sat, 1 Oct 2016 00:14:26 +0200
User-agent: NeoMutt/20160910 (1.7.0)

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?

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

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]