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

["Alex Bennée" <alex.bennee@linaro.org>]

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