Closed GoogleCodeExporter closed 8 years ago
GoogleCodeExporter
commented
8 years ago Not sure if bugdroid hangs around here, I committed this in webrtc to suppress
the race for now, but it'd be nice to be rid of it.
https://codereview.webrtc.org/1414093003/Original comment by pbos@google.com on 20 Oct 2015 at 12:00
GoogleCodeExporter
commented
8 years ago The global variable is basically a shadow of cpuid flags from the cpu, which
wont vary. But the implementation enables/disables the flags as it goes, so it
is unsafe.
I have a CL that fixes that, so only the final result is written to the global.
The other concern would only be for unittests, but theres a function to disable
flags. Its definitely thread unsafe in nature.
e.g.
disable cpu flags
run function1
enable cpu flags
run function2
compare result
during function1, another thread could change the flags.
If the code is working correctly, the only behavior change will be performance.
Its not necessary to use this global... I could query the cpuid directly every
time. It just came up high on profiles, whereas the current implementation
checks the flag in the calling code inline. Modern cpu detection is getting
fairly complicated, checking for OS support, so if cpuid were not shadowed, it
would get increasingly slow in future.
The cpu dispatching is done on every function call.
Most libraries set up function pointers once. That would be faster from a
dispatching point of view, but less flexible and complicates the low level
assembly. The SIMD code is simple and only able to do specific widths.. e.g.
multiple of 16. The init for those function pointers would probably have the
same issue.
If the race is coming up in practice, you could call the libyuv::InitCpuFlags()
once in your startup.Original comment by fbarch...@google.com on 20 Oct 2015 at 6:13
GoogleCodeExporter
commented
8 years ago For unittests this should be fine, since (I'd expect) you wouldn't change those
while any test is running.
For this to be totally race free we have to use something like __atomic_load_n
and __atomic_store_n for the lazy initialization I think. Or mandate that we
call InitCpuFlags() in webrtc before we use it, but I don't think that we have
a single common initialization point where we'd put this (for Chromium).Original comment by pbos@google.com on 21 Oct 2015 at 10:01
GoogleCodeExporter
commented
8 years ago The code now avoids read/modify/write during InitCpuFlags.
You may be able to remove the suppression:
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc
// Race between InitCpuFlags and TestCpuFlag in libyuv.
// https://code.google.com/p/libyuv/issues/detail?id=508
"race:libyuv::TestCpuFlag\n"
Original comment by fbarch...@chromium.org on 22 Oct 2015 at 6:05
GoogleCodeExporter
commented
8 years ago It's still racy (as detected by tsan, true in standard C++ and also true in
non-x86 assembly), so we can't remove the suppression.
https://chromium.googlesource.com/libyuv/libyuv/+/9be6d21ae726f66bc96502c2498866
d41dcce629/source/cpu_id.cc#293 is not an atomic write, nor is the
corresponding read atomic. On x86 these have the same performance (x86
read/writes are acquireload and releasestore), but the compiler isn't allowed
to do funky optimizations with proper atomics present. Compilers are also
getting better at doing these funky optimizations.
Something else that you could do if you don't want to use explicit atomics
(likely slower on any non-x86 platform) is to have cpu_info_ in TLS. Does this
make sense?Original comment by pbos@google.com on 22 Oct 2015 at 8:05
Original issue reported on code.google.com by
pbos@google.comon 19 Oct 2015 at 8:58