From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br0D9-0003Ag-SX for qemu-devel@nongnu.org; Mon, 03 Oct 2016 06:11:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1br0D6-0001ns-JU for qemu-devel@nongnu.org; Mon, 03 Oct 2016 06:11:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54536) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br0D6-0001nb-A1 for qemu-devel@nongnu.org; Mon, 03 Oct 2016 06:11:00 -0400 References: <20160930213106.20186-1-alex.bennee@linaro.org> <20160930213106.20186-4-alex.bennee@linaro.org> <3cb98ad6-1832-57af-e06d-450a031c15f1@redhat.com> <87h98tu69k.fsf@linaro.org> From: Paolo Bonzini Message-ID: <9cc2a403-2116-2c8b-9573-62f1fd316fcf@redhat.com> Date: Mon, 3 Oct 2016 12:10:52 +0200 MIME-Version: 1.0 In-Reply-To: <87h98tu69k.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= Cc: qemu-devel@nongnu.org, mttcg@greensocs.com, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, nikunj@linux.vnet.ibm.com, mark.burton@greensocs.com, jan.kiszka@siemens.com, serge.fdrv@gmail.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite On 03/10/2016 11:32, Alex Benn=C3=A9e wrote: > 1. For atomic_read/set fall back to plain access for sizeof(*ptr) > > sizeof(void *) >=20 > This would throw up warnings in ThreadSanitizer on 64-on-32 but > practically generate the same code as before. >=20 > 2. Split sizeof(*ptr) > sizeof(void *) accesses over two >=20 > This would keep the sanitizer happy but be incorrect behaviour, you > could get tears. >=20 > 3. Add a generic 64-on-32 lock for these accesses >=20 > 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 purel= y > 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. >=20 > 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 sanitize= r > 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: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > WARNING: ThreadSanitizer: data race (pid=3D24906) > 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 (q= emu-arm+0x00006000ce68) > #1 process_queued_cpu_work /home/alex/lsrc/qemu/qemu.git/cpus-commo= n.c:337 (qemu-arm+0x000060116712) > #2 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:654 (qe= mu-arm+0x000060052213) > #3 clone_func /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:60= 70 (qemu-arm+0x0000600686fb) > #4 (libtsan.so.0+0x0000000230d9) >=20 > Previous write of size 8 at 0x7db4000261f0 by main thread (mutexes: w= rite M8): > #0 cpu_list_add /home/alex/lsrc/qemu/qemu.git/cpus-common.c:87 (qem= u-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:44= 7 (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+0x0000601= 56f35) > #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:5= 101 (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:9= 489 (qemu-arm+0x000060073bcc) > #13 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 (q= emu-arm+0x00006005305e) > #14 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 (qemu= -arm+0x0000600555c9) >=20 > Location is heap block of size 37200 at 0x7db40001e000 allocated by m= ain thread: > #0 malloc (libtsan.so.0+0x0000000254a3) > #1 g_malloc (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:5= 101 (qemu-arm+0x0000600eed19) > #5 cpu_copy /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:3744 (q= emu-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:94= 89 (qemu-arm+0x000060073bcc) > #8 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 (qe= mu-arm+0x00006005305e) > #9 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 (qemu-= arm+0x0000600555c9) >=20 > Mutex M8203 (0x0000604ad638) created at: > #0 pthread_mutex_init (libtsan.so.0+0x0000000280b5) > #1 qemu_mutex_init util/qemu-thread-posix.c:46 (qemu-arm+0x0000601b= e220) > #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) >=20 > Mutex M8 (0x000060508920) created at: > #0 pthread_mutex_init (libtsan.so.0+0x0000000280b5) > #1 qemu_mutex_init util/qemu-thread-posix.c:46 (qemu-arm+0x0000601b= e220) > #2 qemu_init_cpu_list /home/alex/lsrc/qemu/qemu.git/cpus-common.c:4= 2 (qemu-arm+0x000060115973) > #3 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4161 (qemu-= arm+0x000060054842) >=20 > Thread T3 (tid=3D24910, running) created by main thread at: > #0 pthread_create (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:94= 89 (qemu-arm+0x000060073bcc) > #3 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:789 (qe= mu-arm+0x00006005305e) > #4 main /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:4751 (qemu-= arm+0x0000600555c9) >=20 > SUMMARY: ThreadSanitizer: data race /home/alex/lsrc/qemu/qemu.git/trans= late-all.c:872 do_tb_flush > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > which showed up on run 619 of a 1000 run test... >=20 >=20 >> >> Paolo >=20 >=20 > -- > Alex Benn=C3=A9e >=20