Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong

[Paolo Bonzini]

On 03/10/2016 11:32, Alex Bennée wrote:
> 1. For atomic_read/set fall back to plain access for sizeof(*ptr) >
> sizeof(void *)
> 
> This would throw up warnings in ThreadSanitizer on 64-on-32 but
> practically generate the same code as before.
> 
> 2. Split sizeof(*ptr) > sizeof(void *) accesses over two
> 
> This would keep the sanitizer happy but be incorrect behaviour, you
> could get tears.
> 
> 3. Add a generic 64-on-32 lock for these accesses
> 
> It would be a global lock for any oversized access which could end up
> being highly contended but at least any behaviour would be always
> correct.

(3) is what libatomic does.

For generic statistics/counters I have written, but not yet upstreamed,
a module which defines two abstract data types:

- Stat64 is a 64-bit value and it can do 64-bit add/min/max.  Reads
block writes, but writes try to bypass the lock if possible (e.g.
min/max that don't modify the stored value, or add that only touches the
lower 32-bits).

- Count64 is actually a 63-bit value and can do 32-bit add or 63-bit
get/set.  Writes block reads, but 32-bit adds try pretty hard not to.

These are meant for the block layer, so they would be necessary even if
64-on-32 went away.  Unfortunately, neither is a perfect replacement for
a 64-bit variable...

Paolo

> It's tricky because pretty much all of the atomic_set/read use is purely
> for C11 correctness, actual sequential correctness is guaranteed by
> using more restrictive primitives and/or additional locking. I'm not
> suggesting we support 64-on-32 for any of the more strict primitives.
> 
> However the series as a whole does have value. As you can see from the
> other patches there are some real races being picked up by the sanitizer
> which only really become visible when a) you remove the noise of the
> "false" positives and b) run the test many many times. For example this
> one:
> 
> ==================
> WARNING: ThreadSanitizer: data race (pid=24906)
>   Read of size 8 at 0x7db4000261f0 by thread T3 (mutexes: write M8203):
>     #0 do_tb_flush /home/alex/lsrc/qemu/qemu.git/translate-all.c:872 
> (qemu-arm+0x00006000ce68)
>     #1 process_queued_cpu_work 
> /home/alex/lsrc/qemu/qemu.git/cpus-common.c:337 (qemu-arm+0x000060116712)
>     #2 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:654 
> (qemu-arm+0x000060052213)
>     #3 clone_func /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6070 
> (qemu-arm+0x0000600686fb)
>     #4 <null> <null> (libtsan.so.0+0x0000000230d9)
> 
>   Previous write of size 8 at 0x7db4000261f0 by main thread (mutexes: write 
> M8):
>     #0 cpu_list_add /home/alex/lsrc/qemu/qemu.git/cpus-common.c:87 
> (qemu-arm+0x000060115b7a)
>     #1 cpu_exec_init /home/alex/lsrc/qemu/qemu.git/exec.c:641 
> (qemu-arm+0x000060009900)
>     #2 arm_cpu_initfn /home/alex/lsrc/qemu/qemu.git/target-arm/cpu.c:447 
> (qemu-arm+0x0000600f833b)
>     #3 object_init_with_type qom/object.c:339 (qemu-arm+0x000060156c73)
>     #4 object_init_with_type qom/object.c:335 (qemu-arm+0x000060156c35)
>     #5 object_initialize_with_type qom/object.c:370 (qemu-arm+0x000060156f35)
>     #6 object_new_with_type qom/object.c:478 (qemu-arm+0x00006015763a)
>     #7 object_new qom/object.c:488 (qemu-arm+0x00006015769e)
>     #8 cpu_generic_init qom/cpu.c:76 (qemu-arm+0x0000601537f4)
>     #9 cpu_arm_init /home/alex/lsrc/qemu/qemu.git/target-arm/helper.c:5101 
> (qemu-arm+0x0000600eed19)
>     #10 cpu_copy /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:3744 
> (qemu-arm+0x000060053516)
>     #11 do_fork /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6107 
> (qemu-arm+0x000060068832)
>     #12 do_syscall /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:9489 
> (qemu-arm+0x000060073bcc)
>     #13 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 
> (qemu-arm+0x00006005305e)
>     #14 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 
> (qemu-arm+0x0000600555c9)
> 
>   Location is heap block of size 37200 at 0x7db40001e000 allocated by main 
> thread:
>     #0 malloc <null> (libtsan.so.0+0x0000000254a3)
>     #1 g_malloc <null> (libglib-2.0.so.0+0x00000004f728)
>     #2 object_new qom/object.c:488 (qemu-arm+0x00006015769e)
>     #3 cpu_generic_init qom/cpu.c:76 (qemu-arm+0x0000601537f4)
>     #4 cpu_arm_init /home/alex/lsrc/qemu/qemu.git/target-arm/helper.c:5101 
> (qemu-arm+0x0000600eed19)
>     #5 cpu_copy /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:3744 
> (qemu-arm+0x000060053516)
>     #6 do_fork /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6107 
> (qemu-arm+0x000060068832)
>     #7 do_syscall /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:9489 
> (qemu-arm+0x000060073bcc)
>     #8 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 
> (qemu-arm+0x00006005305e)
>     #9 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 
> (qemu-arm+0x0000600555c9)
> 
>   Mutex M8203 (0x0000604ad638) created at:
>     #0 pthread_mutex_init <null> (libtsan.so.0+0x0000000280b5)
>     #1 qemu_mutex_init util/qemu-thread-posix.c:46 (qemu-arm+0x0000601be220)
>     #2 code_gen_alloc /home/alex/lsrc/qemu/qemu.git/translate-all.c:739 
> (qemu-arm+0x00006000c7ce)
>     #3 tcg_exec_init /home/alex/lsrc/qemu/qemu.git/translate-all.c:757 
> (qemu-arm+0x00006000c845)
>     #4 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4250 
> (qemu-arm+0x000060054aca)
> 
>   Mutex M8 (0x000060508920) created at:
>     #0 pthread_mutex_init <null> (libtsan.so.0+0x0000000280b5)
>     #1 qemu_mutex_init util/qemu-thread-posix.c:46 (qemu-arm+0x0000601be220)
>     #2 qemu_init_cpu_list /home/alex/lsrc/qemu/qemu.git/cpus-common.c:42 
> (qemu-arm+0x000060115973)
>     #3 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4161 
> (qemu-arm+0x000060054842)
> 
>   Thread T3 (tid=24910, running) created by main thread at:
>     #0 pthread_create <null> (libtsan.so.0+0x000000027577)
>     #1 do_fork /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6147 
> (qemu-arm+0x000060068c1a)
>     #2 do_syscall /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:9489 
> (qemu-arm+0x000060073bcc)
>     #3 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 
> (qemu-arm+0x00006005305e)
>     #4 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 
> (qemu-arm+0x0000600555c9)
> 
> SUMMARY: ThreadSanitizer: data race 
> /home/alex/lsrc/qemu/qemu.git/translate-all.c:872 do_tb_flush
> ==================
> 
> which showed up on run 619 of a 1000 run test...
> 
> 
>>
>> Paolo
> 
> 
> --
> Alex Bennée
>