From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bq6YZ-0005wG-HC for qemu-devel@nongnu.org; Fri, 30 Sep 2016 18:45:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bq6YU-00071j-I1 for qemu-devel@nongnu.org; Fri, 30 Sep 2016 18:45:26 -0400 Received: from mail-wm0-x234.google.com ([2a00:1450:400c:c09::234]:38518) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bq6YU-00070P-2m for qemu-devel@nongnu.org; Fri, 30 Sep 2016 18:45:22 -0400 Received: by mail-wm0-x234.google.com with SMTP id p138so49624929wmb.1 for ; Fri, 30 Sep 2016 15:45:21 -0700 (PDT) References: <20160930213106.20186-1-alex.bennee@linaro.org> <20160930213106.20186-6-alex.bennee@linaro.org> <20160930221426.xlx72nz3owdyi73o@latitude> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <20160930221426.xlx72nz3owdyi73o@latitude> Date: Fri, 30 Sep 2016 23:45:19 +0100 Message-ID: <87r381uhvk.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, mttcg@listserver.greensocs.com, peter.maydell@linaro.org, claudio.fontana@huawei.com, nikunj@linux.vnet.ibm.com, jan.kiszka@siemens.com, mark.burton@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, serge.fdrv@gmail.com, bobby.prani@gmail.com, rth@twiddle.net, fred.konrad@greensocs.com Jonathan Neuschäfer writes: > On Fri, Sep 30, 2016 at 10:30:56PM +0100, 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(&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