Using atomics here is a mistake since they're not guaranteed
to compile.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 util/qsp.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/util/qsp.c b/util/qsp.c
index b0c2575d10..a1ee03b84b 100644
--- a/util/qsp.c
+++ b/util/qsp.c
@@ -351,6 +351,7 @@ static QSPEntry *qsp_entry_get(const void *obj, const char *file, int line,
 static void qsp_entry_aggregate(QSPEntry *to, const QSPEntry *from)
 {
 #ifdef CONFIG_ATOMIC64
+    /* use __nocheck because sizeof(void *) might be < sizeof(u64) */
     to->ns += atomic_read__nocheck(&from->ns);
     to->n_acqs += atomic_read__nocheck(&from->n_acqs);
 #else
@@ -359,8 +360,8 @@ static void qsp_entry_aggregate(QSPEntry *to, const QSPEntry *from)
 
     do {
         version = seqlock_read_begin(&from->sequence);
-        ns = atomic_read__nocheck(&from->ns);
-        n_acqs = atomic_read__nocheck(&from->n_acqs);
+        ns = from->ns;
+        n_acqs = from->n_acqs;
     } while (seqlock_read_retry(&from->sequence, version));
 
     to->ns += ns;
@@ -375,14 +376,17 @@ static void qsp_entry_aggregate(QSPEntry *to, const QSPEntry *from)
  */
 static inline void do_qsp_entry_record(QSPEntry *e, int64_t delta, bool acq)
 {
-#ifndef CONFIG_ATOMIC64
-    seqlock_write_begin(&e->sequence);
-#endif
+#ifdef CONFIG_ATOMIC64
     atomic_set__nocheck(&e->ns, e->ns + delta);
     if (acq) {
         atomic_set__nocheck(&e->n_acqs, e->n_acqs + 1);
     }
-#ifndef CONFIG_ATOMIC64
+#else
+    seqlock_write_begin(&e->sequence);
+    e->ns += delta;
+    if (acq) {
+        e->n_acqs++;
+    }
     seqlock_write_end(&e->sequence);
 #endif
 }
-- 
2.17.1

On 03/09/2018 19:18, Emilio G. Cota wrote:
> Using atomics here is a mistake since they're not guaranteed
> to compile.

But isn't it technically a C11 data race if you don't use atomics?
Could we make nocheck read/set degrade to just a volatile access when
used on a variable that is bigger than pointers, or perhaps always
except when using tsan?

Paolo

> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  util/qsp.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 

On Mon, Sep 10, 2018 at 01:32:15 +0200, Paolo Bonzini wrote:
> On 03/09/2018 19:18, Emilio G. Cota wrote:
> > Using atomics here is a mistake since they're not guaranteed
> > to compile.
> 
> But isn't it technically a C11 data race if you don't use atomics?

Yes, it's undefined behaviour.

> Could we make nocheck read/set degrade to just a volatile access when
> used on a variable that is bigger than pointers, or perhaps always
> except when using tsan?

But volatile wouldn't save you from undefined behaviour, would it?

A simpler and definitely correct alternative is to just use a
spinlock instead of the seqlock also for reads when !CONFIG_ATOMIC64.
We don't care about scalability on those rare hosts anyway, so
I'd go with that.

Thanks,

		Emilio

On 10/09/2018 17:44, Emilio G. Cota wrote:
> On Mon, Sep 10, 2018 at 01:32:15 +0200, Paolo Bonzini wrote:
>> On 03/09/2018 19:18, Emilio G. Cota wrote:
>>> Using atomics here is a mistake since they're not guaranteed
>>> to compile.
>>
>> But isn't it technically a C11 data race if you don't use atomics?
> 
> Yes, it's undefined behaviour.
> 
>> Could we make nocheck read/set degrade to just a volatile access when
>> used on a variable that is bigger than pointers, or perhaps always
>> except when using tsan?
> 
> But volatile wouldn't save you from undefined behaviour, would it?

Yeah, but 1) only on those hosts that cannot do CONFIG_ATOMIC64 2) we
pretty much already define what we expect from volatile.

Paolo

> A simpler and definitely correct alternative is to just use a
> spinlock instead of the seqlock also for reads when !CONFIG_ATOMIC64.
> We don't care about scalability on those rare hosts anyway, so
> I'd go with that.
> 
> Thanks,
> 
> 		Emilio
>