[Qemu-devel] [PATCH v2 0/9] A couple of fixes for ThreadSanitizer

[Qemu-devel] [PATCH v2 0/9] A couple of fixes for ThreadSanitizer

[Alex Bennée]

Hi,

This is v2 of the ThreadSanitizer fixes. Changes from the last
version:

  - added Marc-André's review tags
  - added qga/command: use QEMU atomic primitives
  - simplified ui/vnc-enc-tight: remove switch and have single return
  - fixed the Travis CI build (that was painful....)

There is still some work to do to go through and fix warnings from the
sanitizer. Notably "make check" doesn't complete and generates a load
of warnings and I haven't investigated the warnings generated by
co-routines.

With this series applied you can enable ThreadSanitizer with the
following command line:

  ./configure --extra-cflags="-g3 -O0 \
    -fsantize=thread \
    -fsanitize-blacklist=/home/alex/lsrc/qemu/qemu.git/blacklist.tsan" \
    --with-coroutine=gthread --disable-pie --enable-debug --enable-debug-info

breakdown:
  -fsanitize=thread - enables sanitizer
  -fsanitize-blacklist - skip things the compiler finds hard, like SSE
  --with-coroutine=gthread - tsan chokes on other forms of coroutine
  --disable-pie - tsan no longer works with PIE
  --enable-debug --enable-debug-info - better backtraces

Alex Bennée (8):
  ui/vnc-enc-tight: remove switch and have single return
  tcg/optimize: move default return out of if statement
  new: blacklist.tsan
  qom/object: update class cache atomically
  cpu: atomically modify cpu->exit_request
  util/qht: atomically set b->hashes
  qga/command: use QEMU atomic primitives
  .travis.yml: add gcc sanitizer build

Paolo Bonzini (1):
  seqlock: use atomic writes for the sequence

 .travis.yml            | 45 +++++++++++++++++++++++++++++++++++++++++++++
 blacklist.tsan         |  2 ++
 cpu-exec.c             |  8 ++++----
 include/qemu/seqlock.h |  4 ++--
 qga/commands.c         | 17 +++++++++--------
 qom/cpu.c              |  4 ++--
 qom/object.c           | 15 ++++++++-------
 tcg/optimize.c         |  3 +--
 ui/vnc-enc-tight.c     |  6 ++----
 util/qht.c             | 10 +++++-----
 10 files changed, 80 insertions(+), 34 deletions(-)
 create mode 100644 blacklist.tsan

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/9] ui/vnc-enc-tight: remove switch and have single return

[Alex Bennée]

When enabling the sanitizer build it will complain about control
reaching a non-void function. Normally the compiler should detect that
there is only one possible exit given a static VNC_SERVER_FB_BYTES.

As we always expect a static VNC_SERVER_FB_BYTES I've added a compile
time assert and just called the sub-function directly.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
v2
  - use QEMU_BUILD_BUG_ON instead and just return
---
 ui/vnc-enc-tight.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 49df85e..1e53b1c 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -707,10 +707,8 @@ check_solid_tile32(VncState *vs, int x, int y, int w, int h,
 static bool check_solid_tile(VncState *vs, int x, int y, int w, int h,
                              uint32_t* color, bool samecolor)
 {
-    switch (VNC_SERVER_FB_BYTES) {
-    case 4:
-        return check_solid_tile32(vs, x, y, w, h, color, samecolor);
-    }
+    QEMU_BUILD_BUG_ON(VNC_SERVER_FB_BYTES != 4);
+    return check_solid_tile32(vs, x, y, w, h, color, samecolor);
 }
 
 static void find_best_solid_area(VncState *vs, int x, int y, int w, int h,
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/9] tcg/optimize: move default return out of if statement

[Alex Bennée]

This is to appease sanitizer builds which complain that:

  "error: control reaches end of non-void function"

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tcg/optimize.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 9998ac7..0f13490 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -468,9 +468,8 @@ static TCGArg do_constant_folding_cond(TCGOpcode op, TCGArg x,
         default:
             return 2;
         }
-    } else {
-        return 2;
     }
+    return 2;
 }
 
 /* Return 2 if the condition can't be simplified, and the result
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/9] tcg/optimize: move default return out of if statement

[Richard Henderson]

On 09/22/2016 03:13 AM, Alex Bennée wrote:
> This is to appease sanitizer builds which complain that:
>
>   "error: control reaches end of non-void function"
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tcg/optimize.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index 9998ac7..0f13490 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -468,9 +468,8 @@ static TCGArg do_constant_folding_cond(TCGOpcode op, TCGArg x,
>          default:
>              return 2;
>          }
> -    } else {
> -        return 2;
>      }
> +    return 2;

I'm disappointed that the sanitizer doesn't see the abort above as a noreturn. 
That said, this isn't wrong so,

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH v2 3/9] new: blacklist.tsan

[Alex Bennée]

A list of blacklists for tsan instrumentation. One hopes more can be
removed over time as tsan improves.

The path needs to be absolute so it doesn't break when directories
change during the build:

  ./configure --with-coroutine=gthread --disable-pie \
    --extra-cflags="-g3 -O0 -fsanitize=thread \
    -fsanitize-blacklist=/home/alex/lsrc/qemu/qemu.git/blacklist.tsan"

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 blacklist.tsan | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 blacklist.tsan

diff --git a/blacklist.tsan b/blacklist.tsan
new file mode 100644
index 0000000..9e53a84
--- /dev/null
+++ b/blacklist.tsan
@@ -0,0 +1,2 @@
+# the vector intrinsics upset tsan
+src:bufferiszero.c
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 3/9] new: blacklist.tsan

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On 09/22/2016 05:13 AM, Alex Bennée wrote:
> A list of blacklists for tsan instrumentation. One hopes more can be
> removed over time as tsan improves.

A list of one file? It sounds like this sentence is stale, from an
earlier revision where the blacklist was longer.

> 
> The path needs to be absolute so it doesn't break when directories
> change during the build:
> 
>   ./configure --with-coroutine=gthread --disable-pie \
>     --extra-cflags="-g3 -O0 -fsanitize=thread \
>     -fsanitize-blacklist=/home/alex/lsrc/qemu/qemu.git/blacklist.tsan"
> 

Is there any way to make configure automatically convert a relative name
into an absolute?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  blacklist.tsan | 2 ++
>  1 file changed, 2 insertions(+)
>  create mode 100644 blacklist.tsan
> 
> diff --git a/blacklist.tsan b/blacklist.tsan
> new file mode 100644
> index 0000000..9e53a84
> --- /dev/null
> +++ b/blacklist.tsan
> @@ -0,0 +1,2 @@
> +# the vector intrinsics upset tsan
> +src:bufferiszero.c
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/9] new: blacklist.tsan

[Alex Bennée]

Eric Blake <eblake@redhat.com> writes:

> On 09/22/2016 05:13 AM, Alex Bennée wrote:
>> A list of blacklists for tsan instrumentation. One hopes more can be
>> removed over time as tsan improves.
>
> A list of one file? It sounds like this sentence is stale, from an
> earlier revision where the blacklist was longer.

When I was going through the compiler failures and added the first one I
assumed more would be needed ;-)

>
>>
>> The path needs to be absolute so it doesn't break when directories
>> change during the build:
>>
>>   ./configure --with-coroutine=gthread --disable-pie \
>>     --extra-cflags="-g3 -O0 -fsanitize=thread \
>>     -fsanitize-blacklist=/home/alex/lsrc/qemu/qemu.git/blacklist.tsan"
>>
>
> Is there any way to make configure automatically convert a relative name
> into an absolute?

We could teach configure about the sanitizers and embed the knowledge
there.

>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  blacklist.tsan | 2 ++
>>  1 file changed, 2 insertions(+)
>>  create mode 100644 blacklist.tsan
>>
>> diff --git a/blacklist.tsan b/blacklist.tsan
>> new file mode 100644
>> index 0000000..9e53a84
>> --- /dev/null
>> +++ b/blacklist.tsan
>> @@ -0,0 +1,2 @@
>> +# the vector intrinsics upset tsan
>> +src:bufferiszero.c
>>


--
Alex Bennée

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

[Alex Bennée]

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);
 
     /* Write sequence before updating other fields.  */
     smp_wmb();
@@ -42,7 +42,7 @@ static inline void seqlock_write_end(QemuSeqLock *sl)
     /* Write other fields before finalizing sequence.  */
     smp_wmb();
 
-    ++sl->sequence;
+    atomic_set(&sl->sequence, sl->sequence + 1);
 }
 
 static inline unsigned seqlock_read_begin(QemuSeqLock *sl)
-- 
2.9.3

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 <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);

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(&sl->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

[Qemu-devel] [PATCH v2 5/9] qom/object: update class cache atomically

[Alex Bennée]

The idiom CPU_GET_CLASS(cpu) is fairly extensively used in various
threads and trips of ThreadSanitizer due to the fact it updates
obj->class->object_cast_cache behind the scenes. As this is just a
fast-path cache there is no need to lock updates just ensure that we
don't get torn-updates from two racing lookups. While this is unlikely
on x86 we use the plain atomic_read/set primitives to make this
explicit and keep the sanitizer happy.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/object.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 8166b7d..7a05e35 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -614,7 +614,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename,
     Object *inst;
 
     for (i = 0; obj && i < OBJECT_CLASS_CAST_CACHE; i++) {
-        if (obj->class->object_cast_cache[i] == typename) {
+        if (atomic_read(&obj->class->object_cast_cache[i]) == typename) {
             goto out;
         }
     }
@@ -631,10 +631,10 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename,
 
     if (obj && obj == inst) {
         for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
-            obj->class->object_cast_cache[i - 1] =
-                    obj->class->object_cast_cache[i];
+            atomic_set(&obj->class->object_cast_cache[i - 1],
+                       atomic_read(&obj->class->object_cast_cache[i]));
         }
-        obj->class->object_cast_cache[i - 1] = typename;
+        atomic_set(&obj->class->object_cast_cache[i - 1], typename);
     }
 
 out:
@@ -704,7 +704,7 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
     int i;
 
     for (i = 0; class && i < OBJECT_CLASS_CAST_CACHE; i++) {
-        if (class->class_cast_cache[i] == typename) {
+        if (atomic_read(&class->class_cast_cache[i]) == typename) {
             ret = class;
             goto out;
         }
@@ -725,9 +725,10 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
 #ifdef CONFIG_QOM_CAST_DEBUG
     if (class && ret == class) {
         for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
-            class->class_cast_cache[i - 1] = class->class_cast_cache[i];
+            atomic_set(&class->class_cast_cache[i - 1],
+                       atomic_read(&class->class_cast_cache[i]));
         }
-        class->class_cast_cache[i - 1] = typename;
+        atomic_set(&class->class_cast_cache[i - 1], typename);
     }
 out:
 #endif
-- 
2.9.3

[Qemu-devel] [PATCH v2 6/9] cpu: atomically modify cpu->exit_request

[Alex Bennée]

ThreadSanitizer picks up potential races although we already use
barriers to ensure things are in the correct order when processing exit
requests. For now we just use the relaxed atomic_set/atomic_read semantics
to reassure tsan that we can't tear the value.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpu-exec.c | 8 ++++----
 qom/cpu.c  | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 9f4bd0b..113d8dc 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -192,7 +192,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb)
         /* We were asked to stop executing TBs (probably a pending
          * interrupt. We've now stopped, so clear the flag.
          */
-        cpu->tcg_exit_req = 0;
+        atomic_set(&cpu->tcg_exit_req, 0);
     }
     return ret;
 }
@@ -497,8 +497,8 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
             *last_tb = NULL;
         }
     }
-    if (unlikely(cpu->exit_request || replay_has_interrupt())) {
-        cpu->exit_request = 0;
+    if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
+        atomic_set(&cpu->exit_request, 0);
         cpu->exception_index = EXCP_INTERRUPT;
         cpu_loop_exit(cpu);
     }
@@ -510,7 +510,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
 {
     uintptr_t ret;
 
-    if (unlikely(cpu->exit_request)) {
+    if (unlikely(atomic_read(&cpu->exit_request))) {
         return;
     }
 
diff --git a/qom/cpu.c b/qom/cpu.c
index f783b5a..adc8c11 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -119,10 +119,10 @@ void cpu_reset_interrupt(CPUState *cpu, int mask)
 
 void cpu_exit(CPUState *cpu)
 {
-    cpu->exit_request = 1;
+    atomic_set(&cpu->exit_request, 1);
     /* Ensure cpu_exec will see the exit request after TCG has exited.  */
     smp_wmb();
-    cpu->tcg_exit_req = 1;
+    atomic_set(&cpu->tcg_exit_req, 1);
 }
 
 int cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
-- 
2.9.3

[Qemu-devel] [PATCH v2 7/9] util/qht: atomically set b->hashes

[Alex Bennée]

ThreadSanitizer detects a possible race between reading/writing the
hashes. As ordering semantics are already documented for qht we just
need to ensure a race can't tear the hash value so we can use the
relaxed atomic_set/read functions.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 util/qht.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/qht.c b/util/qht.c
index 16a8d79..571639d 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -379,7 +379,7 @@ static void qht_bucket_reset__locked(struct qht_bucket *head)
             if (b->pointers[i] == NULL) {
                 goto done;
             }
-            b->hashes[i] = 0;
+            atomic_set(&b->hashes[i], 0);
             atomic_set(&b->pointers[i], NULL);
         }
         b = b->next;
@@ -444,7 +444,7 @@ void *qht_do_lookup(struct qht_bucket *head, qht_lookup_func_t func,
 
     do {
         for (i = 0; i < QHT_BUCKET_ENTRIES; i++) {
-            if (b->hashes[i] == hash) {
+            if (atomic_read(&b->hashes[i]) == hash) {
                 /* The pointer is dereferenced before seqlock_read_retry,
                  * so (unlike qht_insert__locked) we need to use
                  * atomic_rcu_read here.
@@ -538,8 +538,8 @@ static bool qht_insert__locked(struct qht *ht, struct qht_map *map,
     if (new) {
         atomic_rcu_set(&prev->next, b);
     }
-    b->hashes[i] = hash;
     /* smp_wmb() implicit in seqlock_write_begin.  */
+    atomic_set(&b->hashes[i], hash);
     atomic_set(&b->pointers[i], p);
     seqlock_write_end(&head->sequence);
     return true;
@@ -607,10 +607,10 @@ qht_entry_move(struct qht_bucket *to, int i, struct qht_bucket *from, int j)
     qht_debug_assert(to->pointers[i]);
     qht_debug_assert(from->pointers[j]);
 
-    to->hashes[i] = from->hashes[j];
+    atomic_set(&to->hashes[i], from->hashes[j]);
     atomic_set(&to->pointers[i], from->pointers[j]);
 
-    from->hashes[j] = 0;
+    atomic_set(&from->hashes[j], 0);
     atomic_set(&from->pointers[j], NULL);
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 7/9] util/qht: atomically set b->hashes

[Emilio G. Cota]

On Thu, Sep 22, 2016 at 11:13:14 +0100, Alex Bennée wrote:
> ThreadSanitizer detects a possible race between reading/writing the
> hashes. As ordering semantics are already documented for qht we just
> need to ensure a race can't tear the hash value so we can use the
> relaxed atomic_set/read functions.

Just being pedantic, but I think the commit log could be improved.
I think it would be more correct to say we're avoiding being out
of C11's spec by using atomic_read/set, instead of tolerating concurrent
regular loads/stores.

Tearing is not really the issue, in the sense that the seqlock protects
against that. IOW, we're not worried about tearing, we're worried about
being out of spec, as Paolo pointed out:

On Mon, Sep 19, 2016 at 20:37:06 +0200, Paolo Bonzini wrote:
> On 19/09/2016 20:06, Emilio G. Cota wrote:
> > On Mon, Sep 19, 2016 at 16:51:38 +0100, Alex Bennée wrote:
> >> > ThreadSanitizer detects a possible race between reading/writing the
> >> > hashes. As ordering semantics are already documented for qht we just
> >> > need to ensure a race can't tear the hash value so we can use the
> >> > relaxed atomic_set/read functions.
> > This was discussed here:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg03658.html
> >
> > To reiterate: reading torn hash values is fine, since the retry will
> > happen regardless (and all pointers[] remain valid through the RCU
> > read-critical section).
>
> True, but C11 says data races are undefined, not merely unspecified.
> seqlock-protected data requires a relaxed read and write, because they
> are read concurrently in the read and write sides.

Acknowledging in the commit log the tiny-yet-measurable perf hit would be
good, too (I'd just copy the before/after results I posted).

That said,

  Reviewed-by: Emilio G. Cota <cota@braap.org>

Thanks,

		E.

Re: [Qemu-devel] [PATCH v2 7/9] util/qht: atomically set b->hashes

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

> On Thu, Sep 22, 2016 at 11:13:14 +0100, Alex Bennée wrote:
>> ThreadSanitizer detects a possible race between reading/writing the
>> hashes. As ordering semantics are already documented for qht we just
>> need to ensure a race can't tear the hash value so we can use the
>> relaxed atomic_set/read functions.
>
> Just being pedantic, but I think the commit log could be improved.
> I think it would be more correct to say we're avoiding being out
> of C11's spec by using atomic_read/set, instead of tolerating concurrent
> regular loads/stores.
>
> Tearing is not really the issue, in the sense that the seqlock protects
> against that. IOW, we're not worried about tearing, we're worried about
> being out of spec, as Paolo pointed out:
>
> On Mon, Sep 19, 2016 at 20:37:06 +0200, Paolo Bonzini wrote:
>> On 19/09/2016 20:06, Emilio G. Cota wrote:
>> > On Mon, Sep 19, 2016 at 16:51:38 +0100, Alex Bennée wrote:
>> >> > ThreadSanitizer detects a possible race between reading/writing the
>> >> > hashes. As ordering semantics are already documented for qht we just
>> >> > need to ensure a race can't tear the hash value so we can use the
>> >> > relaxed atomic_set/read functions.
>> > This was discussed here:
>> >
>> > https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg03658.html
>> >
>> > To reiterate: reading torn hash values is fine, since the retry will
>> > happen regardless (and all pointers[] remain valid through the RCU
>> > read-critical section).
>>
>> True, but C11 says data races are undefined, not merely unspecified.
>> seqlock-protected data requires a relaxed read and write, because they
>> are read concurrently in the read and write sides.

You are quite right. Having been in the guts of the ThreadSanitizer with
the toolchain guys I'm starting to see this is the only real way to mark
things (it doesn't actually implement stand-along barriers).

>
> Acknowledging in the commit log the tiny-yet-measurable perf hit would be
> good, too (I'd just copy the before/after results I posted).

Will do.

>
> That said,
>
>   Reviewed-by: Emilio G. Cota <cota@braap.org>
>
> Thanks,

Thnaks,


--
Alex Bennée

[Qemu-devel] [PATCH v2 8/9] qga/command: use QEMU atomic primitives

[Alex Bennée]

The guest client's use of the glib's g_atomic primitives causes newer
GCC's to barf when built on Travis. As QEMU has its own primitives with
well understood semantics we might as well use them.

The use of atomics was a little inconsistent so I've also ensure the
values are correctly set with atomic primitives at the same time.

I also made the usage of bool consistent while I was at it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 qga/commands.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/qga/commands.c b/qga/commands.c
index 50fd26a..edd3e83 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -16,6 +16,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/base64.h"
 #include "qemu/cutils.h"
+#include "qemu/atomic.h"
 
 /* Maximum captured guest-exec out_data/err_data - 16MB */
 #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
@@ -82,7 +83,7 @@ struct GuestExecIOData {
     guchar *data;
     gsize size;
     gsize length;
-    gint closed;
+    bool closed;
     bool truncated;
     const char *name;
 };
@@ -93,7 +94,7 @@ struct GuestExecInfo {
     int64_t pid_numeric;
     gint status;
     bool has_output;
-    gint finished;
+    bool finished;
     GuestExecIOData in;
     GuestExecIOData out;
     GuestExecIOData err;
@@ -156,13 +157,13 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
 
     ges = g_new0(GuestExecStatus, 1);
 
-    bool finished = g_atomic_int_get(&gei->finished);
+    bool finished = atomic_mb_read(&gei->finished);
 
     /* need to wait till output channels are closed
      * to be sure we captured all output at this point */
     if (gei->has_output) {
-        finished = finished && g_atomic_int_get(&gei->out.closed);
-        finished = finished && g_atomic_int_get(&gei->err.closed);
+        finished = finished && atomic_mb_read(&gei->out.closed);
+        finished = finished && atomic_mb_read(&gei->err.closed);
     }
 
     ges->exited = finished;
@@ -264,7 +265,7 @@ static void guest_exec_child_watch(GPid pid, gint status, gpointer data)
             (int32_t)gpid_to_int64(pid), (uint32_t)status);
 
     gei->status = status;
-    gei->finished = true;
+    atomic_mb_set(&gei->finished, true);
 
     g_spawn_close_pid(pid);
 }
@@ -320,7 +321,7 @@ static gboolean guest_exec_input_watch(GIOChannel *ch,
 done:
     g_io_channel_shutdown(ch, true, NULL);
     g_io_channel_unref(ch);
-    g_atomic_int_set(&p->closed, 1);
+    atomic_mb_set(&p->closed, true);
     g_free(p->data);
 
     return false;
@@ -374,7 +375,7 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
 close:
     g_io_channel_shutdown(ch, true, NULL);
     g_io_channel_unref(ch);
-    g_atomic_int_set(&p->closed, 1);
+    atomic_mb_set(&p->closed, true);
     return false;
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v2 9/9] .travis.yml: add gcc sanitizer build

[Alex Bennée]

As it seems easy to break the ThreadSanitizer build we should defend it to
ensure that fixes get applied when it breaks. We use the Ubuntu GCC PPA
to get the latest GCC goodness.

As we need to use the -fuse-ld=gold work around we have to disable the
linux-user targets as these trip up the linker.

The make check run is also disabled for Travis but this can be
re-enabled once the check targets have been fixed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - move to container instead of using Trusty
  - work around link problem with -fuse-ld=gold
  - disable linux-user builds
  - drop blacklist (gcc doesn't support it)
  - dump config.log if configure step fails
  - disable make check
  - ensure we use gthread coroutines
---
 .travis.yml | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index f30b10e..9916178 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -9,6 +9,7 @@ cache: ccache
 addons:
   apt:
     packages:
+      # Build dependencies
       - libaio-dev
       - libattr1-dev
       - libbrlapi-dev
@@ -89,6 +90,7 @@ matrix:
     - env: CONFIG=""
       os: osx
       compiler: clang
+    # Plain Trusty Build
     - env: CONFIG=""
       sudo: required
       addons:
@@ -99,3 +101,46 @@ matrix:
         - sudo apt-get build-dep -qq qemu
         - wget -O - http://people.linaro.org/~alex.bennee/qemu-submodule-git-seed.tar.xz | tar -xvJ
         - git submodule update --init --recursive
+    # Using newer GCC with sanitizers
+    - addons:
+        apt:
+          sources:
+            # PPAs for newer toolchains
+            - ubuntu-toolchain-r-test
+          packages:
+            # Extra toolchains
+            - gcc-5
+            - g++-5
+            # Build dependencies
+            - libaio-dev
+            - libattr1-dev
+            - libbrlapi-dev
+            - libcap-ng-dev
+            - libgnutls-dev
+            - libgtk-3-dev
+            - libiscsi-dev
+            - liblttng-ust-dev
+            - libnfs-dev
+            - libncurses5-dev
+            - libnss3-dev
+            - libpixman-1-dev
+            - libpng12-dev
+            - librados-dev
+            - libsdl1.2-dev
+            - libseccomp-dev
+            - libspice-protocol-dev
+            - libspice-server-dev
+            - libssh2-1-dev
+            - liburcu-dev
+            - libusb-1.0-0-dev
+            - libvte-2.90-dev
+            - sparse
+            - uuid-dev
+      language: generic
+      compiler: none
+      env:
+        - COMPILER_NAME=gcc CXX=g++-5 CC=gcc-5
+        - CONFIG="--cc=gcc-5 --cxx=g++-5 --disable-pie --disable-linux-user --with-coroutine=gthread"
+        - TEST_CMD=""
+      before_script:
+        - ./configure ${CONFIG} --extra-cflags="-g3 -O0 -fsanitize=thread -fuse-ld=gold" || cat config.log
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 0/9] A couple of fixes for ThreadSanitizer

[Paolo Bonzini]

On 22/09/2016 12:13, Alex Bennée wrote:
> Hi,
> 
> This is v2 of the ThreadSanitizer fixes. Changes from the last
> version:
> 
>   - added Marc-André's review tags
>   - added qga/command: use QEMU atomic primitives
>   - simplified ui/vnc-enc-tight: remove switch and have single return
>   - fixed the Travis CI build (that was painful....)
> 
> There is still some work to do to go through and fix warnings from the
> sanitizer. Notably "make check" doesn't complete and generates a load
> of warnings and I haven't investigated the warnings generated by
> co-routines.
> 
> With this series applied you can enable ThreadSanitizer with the
> following command line:
> 
>   ./configure --extra-cflags="-g3 -O0 \
>     -fsantize=thread \
>     -fsanitize-blacklist=/home/alex/lsrc/qemu/qemu.git/blacklist.tsan" \
>     --with-coroutine=gthread --disable-pie --enable-debug --enable-debug-info
> 
> breakdown:
>   -fsanitize=thread - enables sanitizer
>   -fsanitize-blacklist - skip things the compiler finds hard, like SSE
>   --with-coroutine=gthread - tsan chokes on other forms of coroutine
>   --disable-pie - tsan no longer works with PIE
>   --enable-debug --enable-debug-info - better backtraces
> 
> Alex Bennée (8):
>   ui/vnc-enc-tight: remove switch and have single return
>   tcg/optimize: move default return out of if statement
>   new: blacklist.tsan
>   qom/object: update class cache atomically
>   cpu: atomically modify cpu->exit_request
>   util/qht: atomically set b->hashes
>   qga/command: use QEMU atomic primitives
>   .travis.yml: add gcc sanitizer build
> 
> Paolo Bonzini (1):
>   seqlock: use atomic writes for the sequence
> 
>  .travis.yml            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  blacklist.tsan         |  2 ++
>  cpu-exec.c             |  8 ++++----
>  include/qemu/seqlock.h |  4 ++--
>  qga/commands.c         | 17 +++++++++--------
>  qom/cpu.c              |  4 ++--
>  qom/object.c           | 15 ++++++++-------
>  tcg/optimize.c         |  3 +--
>  ui/vnc-enc-tight.c     |  6 ++----
>  util/qht.c             | 10 +++++-----
>  10 files changed, 80 insertions(+), 34 deletions(-)
>  create mode 100644 blacklist.tsan
> 

Queued patches 2-8 (1 is already in and 9 is outside my knowledge), thanks.

Re: [Qemu-devel] [PATCH v2 0/9] A couple of fixes for ThreadSanitizer

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 22/09/2016 12:13, Alex Bennée wrote:
>> Hi,
>>
<snip>
>
> Queued patches 2-8 (1 is already in and 9 is outside my knowledge), thanks.

Actually could you take them from the v3 I've just posted? I've cleaned
up a bunch of the commit messages and dropped the blacklist patch.

--
Alex Bennée