|
|
Created:
6 years, 9 months ago by Vyacheslav Egorov (Chromium) Modified:
6 years, 9 months ago Reviewers:
Alexander Potapenko, wibling-chromium, abarth-chromium, wibling, dvyukov, Chris Evans, Dmitry Vyukov CC:
blink-reviews, Mads Ager (chromium), abarth-chromium, Inactive, haraken, Mikhail, kouhei+heap_chromium.org, adamk+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionIntroduce WTF::releaseStore and WTF::acquireLoad.
Use them to avoid data races in the SafePointBarrier discovered by TSAN and guarantee that compiler does not move any memory accesses around m_unparkedThreadCount and m_canResume loads.
This change is likely to affect only ARM with its weakly ordered memory model.
BUG=347559
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169469
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address glider's comments #
Total comments: 2
Messages
Total messages: 18 (0 generated)
Hi Chris, Would you mind taking a look? You seem to touched wtf/Atomics.h last and you are a WTF owner. If you don't have just route to someone who you'd think is qualified. Thanks. Alexander & Dmitriy FYI as a TSAN related change.
Two style nits, feel free to ignore if they're not applicable to Blink. https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) Here you implicitly assume CPU(ARM). This may break when Blink is ported to another arch. https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode136 Source/wtf/Atomics.h:136: // Note: This is a function call, which is also an implicit compiler barrier. If the 80-column limit applies here, please fix.
LGTM This is very much appreciated, thanks!
Adam, could you take a look?
https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) On 2014/03/17 08:02:28, Alexander Potapenko wrote: > Here you implicitly assume CPU(ARM). This may break when Blink is ported to > another arch. The comment assumes ARM indeed but the code itself is simply Linux specific so it will continue to work (and it will work e.g. on Android MIPS if such thing exists). I am not sure how to handle this in the best way. https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode136 Source/wtf/Atomics.h:136: // Note: This is a function call, which is also an implicit compiler barrier. On 2014/03/17 08:02:28, Alexander Potapenko wrote: > If the 80-column limit applies here, please fix. (Fortunately) Blink does not have 80-column limit :)
https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) On 2014/03/17 12:00:41, Vyacheslav Egorov wrote: > On 2014/03/17 08:02:28, Alexander Potapenko wrote: > > Here you implicitly assume CPU(ARM). This may break when Blink is ported to > > another arch. > > The comment assumes ARM indeed but the code itself is simply Linux specific so > it will continue to work (and it will work e.g. on Android MIPS if such thing > exists). Not sure how this can work on Android MIPS (or any other non-ARM arch) if the corresponding entry point at 0xffff0fa0 isn't provided by non-ARM kernels. Am I missing something?
https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/1/Source/wtf/Atomics.h#newcode130 Source/wtf/Atomics.h:130: #elif OS(LINUX) || OS(ANDROID) On 2014/03/17 12:09:58, Alexander Potapenko wrote: > On 2014/03/17 12:00:41, Vyacheslav Egorov wrote: > > On 2014/03/17 08:02:28, Alexander Potapenko wrote: > > > Here you implicitly assume CPU(ARM). This may break when Blink is ported to > > > another arch. > > > > The comment assumes ARM indeed but the code itself is simply Linux specific so > > it will continue to work (and it will work e.g. on Android MIPS if such thing > > exists). > Not sure how this can work on Android MIPS (or any other non-ARM arch) if the > corresponding entry point at 0xffff0fa0 isn't provided by non-ARM kernels. > Am I missing something? I misread the docs (https://www.kernel.org/doc/Documentation/arm/kernel_user_helpers.txt) and somehow completely missed the part that it's ARM specific. Added CPU(ARM) check.
rslgtm This code is kind of crazy...
The CQ bit was checked by abarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vegorov@chromium.org/199413012/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by vegorov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vegorov@chromium.org/199413012/20001
Message was sent while issue was closed.
Change committed as 169469
Message was sent while issue was closed.
https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h#new... Source/wtf/Atomics.h:148: MEMORY_BARRIER(); I don't quite understand this. Wouldn't you normally put the barrier after the write and before the read? As it is now can't you have Thread0: MB store value Thread1: read value (but no barrier so reading local thread cached version) MB
Message was sent while issue was closed.
https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h File Source/wtf/Atomics.h (right): https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h#new... Source/wtf/Atomics.h:148: MEMORY_BARRIER(); On 2014/03/20 08:05:31, wibling-chromium wrote: > I don't quite understand this. Wouldn't you normally put the barrier after the > write and before the read? No > As it is now can't you have > > Thread0: > MB > store value > > Thread1: > read value (but no barrier so reading local thread cached version) > MB There are no per-thread caches for variables.
Thanks. Slava explained it offline as well. On Thu, Mar 20, 2014 at 10:27 AM, <dvyukov@google.com> wrote: > > https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h > File Source/wtf/Atomics.h (right): > > https://codereview.chromium.org/199413012/diff/20001/Source/wtf/Atomics.h# > newcode148 > Source/wtf/Atomics.h:148: MEMORY_BARRIER(); > On 2014/03/20 08:05:31, wibling-chromium wrote: > >> I don't quite understand this. Wouldn't you normally put the barrier >> > after the > >> write and before the read? >> > > No > > > As it is now can't you have >> > > Thread0: >> MB >> store value >> > > Thread1: >> read value (but no barrier so reading local thread cached version) >> MB >> > > There are no per-thread caches for variables. > > https://codereview.chromium.org/199413012/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org. |