Re: [Qemu-devel] [PATCH v2 4/9] seqlock: use atomic writes for the sequence

[Richard Henderson]

On 09/22/2016 03:13 AM, Alex Bennée wrote:

From: Paolo Bonzini 

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 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Alex Bennée 
---
 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(>sequence, sl->sequence + 1);


The read side isn't using a atomic_read right here.

This appears to be tsan silliness to me.


r~

Re: [Qemu-devel] [PATCH v2 4/9] seqlock: use atomic writes for the sequence

[Paolo Bonzini]

On 22/09/2016 17:38, Richard Henderson wrote:
>> 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(>sequence, sl->sequence + 1);
> 
> The read side isn't using a atomic_read right here.

There cannot be concurrent writes, so this is okay (and actually helps
catching seqlock write-side critical sections not protected by a mutex
or something else).

The rule is that there is a race if two accesses, one of them a write
_and_ one of them not atomic, are concurrent.

So reads not concurrent with any writes are fine (e.g. if all writes are
under a mutex, reads under the mutex are fine too).

Paolo