[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to t
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong |
Date: |
Mon, 3 Oct 2016 12:10:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
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
>