From: "Alex Bennée" <alex.bennee@linaro.org>
To: Paolo Bonzini <pbonzini@redhat.com>
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 <crosthwaite.peter@gmail.com>
Subject: Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong
Date: Mon, 03 Oct 2016 10:32:55 +0100 [thread overview]
Message-ID: <87h98tu69k.fsf@linaro.org> (raw)
In-Reply-To: <3cb98ad6-1832-57af-e06d-450a031c15f1@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 30/09/2016 23:30, Alex Bennée wrote:
>> Commit b480d9b74 converted tb_page_addr_t to abi_ulong which while the
>> right size imposes additional alignment restrictions on the type. This
>> gets in the way of using atomic accesses on certain guest platforms
>> which allow finer alignments. As tb_page_addr_t isn't actually visible
>> to the guest we can revert the change.
>>
>> This is potentially less efficient for ILP32 style guests but it is the
>> simpler change to make.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> include/exec/exec-all.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 336a57c..c3596a6 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -30,7 +30,7 @@
>> addresses in userspace mode. Define tb_page_addr_t to be an appropriate
>> type. */
>> #if defined(CONFIG_USER_ONLY)
>> -typedef abi_ulong tb_page_addr_t;
>> +typedef target_ulong tb_page_addr_t;
>> #else
>> typedef ram_addr_t tb_page_addr_t;
>> #endif
>>
>
> I think target_ulong is in general not accessible atomically, because we
> still haven't dropped 64-on-32 support.
True. I've been thinking about our options here.
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.
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
next prev parent reply other threads:[~2016-10-03 9:33 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-30 21:30 [Qemu-devel] [PATCH v3 00/15] A number of fixes for ThreadSanitizer Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 01/15] atomic.h: fix __SANITIZE_THREAD__ build Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 02/15] atomic.h: comment on use of atomic_read/set Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong Alex Bennée
2016-10-03 8:59 ` Paolo Bonzini
2016-10-03 9:32 ` Alex Bennée [this message]
2016-10-03 10:10 ` Paolo Bonzini
2016-10-04 14:08 ` Alex Bennée
2016-10-03 15:31 ` Emilio G. Cota
2016-10-03 16:16 ` Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 04/15] tcg/optimize: move default return out of if statement Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence Alex Bennée
2016-09-30 22:14 ` Jonathan Neuschäfer
2016-09-30 22:45 ` Alex Bennée
2016-09-30 22:58 ` Jonathan Neuschäfer
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 06/15] qom/object: update class cache atomically Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 07/15] qom/cpu: atomically clear the tb_jmp_cache Alex Bennée
2016-09-30 21:30 ` [Qemu-devel] [PATCH v3 08/15] cpu: atomically modify cpu->exit_request Alex Bennée
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 09/15] util/qht: atomically set b->hashes Alex Bennée
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 10/15] linux-user/syscall: extend lock around cpu-list Alex Bennée
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 11/15] qga/command: use QEMU atomic primitives Alex Bennée
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 12/15] .travis.yml: add gcc sanitizer build Alex Bennée
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 13/15] tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write Alex Bennée
2016-09-30 22:12 ` [Qemu-devel] [PATCH] fixup! " Alex Bennée
2016-10-03 8:43 ` [Qemu-devel] [PATCH v3 13/15] " Paolo Bonzini
2016-10-03 9:48 ` Alex Bennée
2016-10-03 9:53 ` Paolo Bonzini
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 14/15] tcg: update remaining TranslationBuffer fields atomically Alex Bennée
2016-09-30 21:31 ` [Qemu-devel] [PATCH v3 15/15] translate-all: mark updates to PageDesc as atomic Alex Bennée
2016-10-03 8:50 ` Paolo Bonzini
2016-09-30 21:54 ` [Qemu-devel] [PATCH v3 00/15] A number of fixes for ThreadSanitizer no-reply
2016-09-30 22:06 ` no-reply
2016-10-03 9:25 ` Paolo Bonzini
2016-10-03 9:43 ` Alex Bennée
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h98tu69k.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=a.rigo@virtualopensystems.com \
--cc=bobby.prani@gmail.com \
--cc=claudio.fontana@huawei.com \
--cc=cota@braap.org \
--cc=crosthwaite.peter@gmail.com \
--cc=fred.konrad@greensocs.com \
--cc=jan.kiszka@siemens.com \
--cc=mark.burton@greensocs.com \
--cc=mttcg@greensocs.com \
--cc=nikunj@linux.vnet.ibm.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
--cc=serge.fdrv@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.