Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2114993003:
Access FileTracing::g_provider atomically to prevent data races (Closed)
|
Created:
4 years, 5 months ago by Dan Beam Modified:
4 years, 5 months ago CC:
chromium-reviews, nduca_chromium.rg, Lei Zhang, oystein (OOO til 10th of July) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAccess FileTracing::g_provider atomically to prevent data races
R=danakj@chromium.org
BUG=522018
Committed: https://crrev.com/00dd6010cdda17cecd624c0f274aeb630f31fb83
Cr-Commit-Position: refs/heads/master@{#405907}
Patch Set 1 #
Total comments: 5
Patch Set 2 : danakj@ review #Patch Set 3 : load and store #
Total comments: 2
Patch Set 4 : glider@/groby@ review #
Total comments: 2
Patch Set 5 : silly quirks are silly #Messages
Total messages: 32 (13 generated)
dbeam@chromium.org changed reviewers: - nduca@chromium.rg, thestig@chromium.org
/cc thestig@, oysteine@, and nduca@ for any input so a while ago, I added file-level tracing, i.e. if you flip the "file" category of tracing on you get to see where each base::File is created, where they read/write/seek/etc. in the trace viewer. neato. ... but it accidentally pulled tracing into the installer (and blew up the size)[2]. so, I made a "provider" interface to fix this issue, and set it from content/browser[3] (so only Chrome pays the price, the installer doesn't). the code that actually sets the provider is not atomic, which is an issue if a whole bunch of file tracing is going on from other threads (i.e. another thread can try to use the half-set pointer). tl;dr - danakj@: does this fix look right? the warning on top of atomicops.h scares me, and I'm definitely in the "do not know what you're doing" bunch[4] [1] https://codereview.chromium.org/1072133006/ [2] https://codereview.chromium.org/1127713004/ [3] https://codereview.chromium.org/1072133006/diff/810001/content/browser/tracin... [4] https://cs.chromium.org/chromium/src/base/atomicops.h?dr=C&q=nobarrier_store&...
Description was changed from ========== Initialize FileTracing::Provider atomically to prevent data races R=thestig@chromium.org,danakj@chromium.org,nduca@chromium.rg BUG=522018 ========== to ========== Initialize FileTracing::Provider atomically to prevent data races R=danakj@chromium.org BUG=522018 ==========
ping
https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc#... base/files/file_tracing.cc:10: using base::subtle::AtomicWord; nit: you could put this in the method that uses it. https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc#... base/files/file_tracing.cc:25: base::subtle::NoBarrier_Store(reinterpret_cast<AtomicWord*>(&g_provider), I think the way most people do this would be with a static base::LazyInstance<base::Lock> beside the g_provider. And that would be easier to understand/verify for sure, and avoids subtle, so yay. I *think* this is right though (I haven't actually reviewed/used NoBarrier_Store before, fun!), because you want to ensure the pointer is set, but you don't care if another thread set it at the same timeish, and that you get their value? But the call in https://codereview.chromium.org/1072133006/diff/810001/content/browser/tracin... is setting it to a newly malloced thing. If multiple threads are doing this, they are setting it to different values. Do you care which one gets used in all the places that use g_provider below? Why/not?
https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc#... base/files/file_tracing.cc:10: using base::subtle::AtomicWord; On 2016/07/07 22:40:36, danakj wrote: > nit: you could put this in the method that uses it. Done. https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc#... base/files/file_tracing.cc:25: base::subtle::NoBarrier_Store(reinterpret_cast<AtomicWord*>(&g_provider), On 2016/07/07 22:40:36, danakj wrote: > I think the way most people do this would be with a static > base::LazyInstance<base::Lock> beside the g_provider. And that would be easier > to understand/verify for sure, and avoids subtle, so yay. I *think* this is > right though (I haven't actually reviewed/used NoBarrier_Store before, fun!), > because you want to ensure the pointer is set, but you don't care if another > thread set it at the same timeish, and that you get their value? > > But the call in > https://codereview.chromium.org/1072133006/diff/810001/content/browser/tracin... > is setting it to a newly malloced thing. If multiple threads are doing this, > they are setting it to different values. Do you care which one gets used in all > the places that use g_provider below? Why/not? g_provider should only be set once. ever. i can verify that with a CHECK() maybe.
https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/1/base/files/file_tracing.cc#... base/files/file_tracing.cc:25: base::subtle::NoBarrier_Store(reinterpret_cast<AtomicWord*>(&g_provider), On 2016/07/07 23:13:48, Dan Beam wrote: > On 2016/07/07 22:40:36, danakj wrote: > > I think the way most people do this would be with a static > > base::LazyInstance<base::Lock> beside the g_provider. And that would be easier > > to understand/verify for sure, and avoids subtle, so yay. I *think* this is > > right though (I haven't actually reviewed/used NoBarrier_Store before, fun!), > > because you want to ensure the pointer is set, but you don't care if another > > thread set it at the same timeish, and that you get their value? > > > > But the call in > > > https://codereview.chromium.org/1072133006/diff/810001/content/browser/tracin... > > is setting it to a newly malloced thing. If multiple threads are doing this, > > they are setting it to different values. Do you care which one gets used in > all > > the places that use g_provider below? Why/not? > > g_provider should only be set once. ever. i can verify that with a CHECK() > maybe. Ah, so it's just threads are reading null.. reading null.. reading half-set pointer.. A DCHECK might be nice sure :) CHECK is probably overkill. Why don't you want a memory barrier so other threads pick up the value you set here? using lock lets us not ask these questions.
glider@chromium.org changed reviewers: + glider@chromium.org
https://codereview.chromium.org/2114993003/diff/40001/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/40001/base/files/file_tracing... base/files/file_tracing.cc:19: AtomicWord provider = base::subtle::NoBarrier_Load(&g_provider); You'll need Acquire_Load and Release_Store for getter and setter, because the poiner has associated data.
groby@chromium.org changed reviewers: + groby@chromium.org
The NoBarrier_Load approach I suggest in the comment feels disturbingly "clever". Vet _very_ carefully if you really want to use it. I spent way too much time checking it already :) https://codereview.chromium.org/2114993003/diff/40001/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/40001/base/files/file_tracing... base/files/file_tracing.cc:19: AtomicWord provider = base::subtle::NoBarrier_Load(&g_provider); On 2016/07/09 10:32:04, Alexander Potapenko wrote: > You'll need Acquire_Load and Release_Store for getter and setter, because the > poiner has associated data. In a nutshell: Follow Alexander's suggestion if you want to avoid pain. release_store/acquire_load is a well-understood pattern for setting globals, and it works. If you're still concerned about speed, I _think_ you can do better, but somebody else should double-check my reasoning. (subtle is subtle for a reason. Stick with Release_Store/Acquire_Load unless you *really* need those cycles) Reasoning just about the logic: * |g_provider| is only set once * FileTracing::ProviderImpl is stateless, so there are no dependent read/writes * out-of-order reads of |g_provider| are not critical. (i.e. Provider, nullptr, Provider needs to be fine - the ScopedEnabled might be a problem here, because Enable/Disable might get mismatched. Can any tracing ops happen before SetProvider?) That would mean you really don't care about barriers. Except, in the real world, ProviderImpl carries around a vtbl pointer which is dependent data. It's read-only, though. Given all these, writing still needs Release_Store - you want to ensure the vtbl isn't set after |g_provider| is set, that'd be embarrassing :) Leaves reading. Acquire_Load prevents reordering with later reads or writes. Later writes to the vtbl can't happen after it's set, and later writes to |g_provider| (on a separate thread) don't matter because we'll always get nullptr or a valid provider - and it's not a fatal flaw if you first get a provider, then nullptr, then a provider. (AFAICT). Later dependent reads are only the vtbl, and the set operation guaranteed that's set when |g_provider| is set. The cost of a memory barrier is (wild guess) on average some 100s of CPU cycles, so this has to be on a *really* hot path to be worth it.
On 2016/07/13 20:05:04, groby wrote: > The NoBarrier_Load approach I suggest in the comment feels disturbingly > "clever". Vet _very_ carefully if you really want to use it. > > I spent way too much time checking it already :) > > https://codereview.chromium.org/2114993003/diff/40001/base/files/file_tracing.cc > File base/files/file_tracing.cc (right): > > https://codereview.chromium.org/2114993003/diff/40001/base/files/file_tracing... > base/files/file_tracing.cc:19: AtomicWord provider = > base::subtle::NoBarrier_Load(&g_provider); > On 2016/07/09 10:32:04, Alexander Potapenko wrote: > > You'll need Acquire_Load and Release_Store for getter and setter, because the > > poiner has associated data. > > In a nutshell: Follow Alexander's suggestion if you want to avoid pain. > release_store/acquire_load is a well-because understood pattern for setting globals, and > it works. > > If you're still concerned about speed, I _think_ you can do better, but somebody > else should double-check my reasoning. (subtle is subtle for a reason. Stick > with Release_Store/Acquire_Load unless you *really* need those cycles) I'd better land the Release_Store/Acquire_Load solution and assess the performance before actually doing premature optimizations. > Reasoning just about the logic: > * |g_provider| is only set once > * FileTracing::ProviderImpl is stateless, so there are no dependent read/writes > * out-of-order reads of |g_provider| are not critical. (i.e. Provider, nullptr, > Provider needs to be fine - the ScopedEnabled might be a problem here, because > Enable/Disable might get mismatched. Can any tracing ops happen before > SetProvider?) > > That would mean you really don't care about barriers. Except, in the real world, > ProviderImpl carries around a vtbl pointer which is dependent data. It's > read-only, though. > > Given all these, writing still needs Release_Store - you want to ensure the vtbl > isn't set after |g_provider| is set, that'd be embarrassing :) > > Leaves reading. Acquire_Load prevents reordering with later reads or writes. > Later writes to the vtbl can't happen after it's set, and later writes to > |g_provider| (on a separate thread) don't matter because we'll always get > nullptr or a valid provider - and it's not a fatal flaw if you first get a > provider, then nullptr, then a provider. (AFAICT). Later dependent reads are > only the vtbl, and the set operation guaranteed that's set when |g_provider| is > set. > > The cost of a memory barrier is (wild guess) on average some 100s of CPU cycles, > so this has to be on a *really* hot path to be worth it. Please note that on x86 CPU a memory barrier costs nothing.
Rachel is actually right that Acquire_Load is not strictly necessary here. What we need instead is a load with std::memory_order_consume. Unfortunately this doesn't exist in the base::subtle memory model, and std::atomic is still forbidden (IIRC). Using a NoBarrier_Load() instead of Acquire_Load() will have the following drawbacks: - TSan won't understand it, we'll need a suppression or an "#ifdef THREAD_SANITIZER" workaround - when we replace base::subtle with std::atomic, a clever enough compiler can break this code.
switched to Acuire/Release and tested with --trace-startup=disabled-by-default-file --trace-startup-file=/tmp/startup-trace.json as well as just running a "Record" from chrome://tracing. all seemed fine (though it did before this as well, so...)
On 2016/07/14 17:49:30, Dan Beam wrote: > switched to Acuire/Release and tested with *Acquire of course > --trace-startup=disabled-by-default-file > --trace-startup-file=/tmp/startup-trace.json as well as just running a "Record" > from chrome://tracing. all seemed fine (though it did before this as well, > so...)
On 2016/07/14 17:49:30, Dan Beam wrote: > switched to Acuire/Release and tested with > --trace-startup=disabled-by-default-file > --trace-startup-file=/tmp/startup-trace.json as well as just running a "Record" > from chrome://tracing. all seemed fine (though it did before this as well, > so...) FYI you won't notice any difference between barrier and nobarrier versions of these atomics on x86_64
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Initialize FileTracing::Provider atomically to prevent data races R=danakj@chromium.org BUG=522018 ========== to ========== Access FileTracing::g_provider atomically to prevent data races R=danakj@chromium.org BUG=522018 ==========
LGTM https://codereview.chromium.org/2114993003/diff/60001/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/60001/base/files/file_tracing... base/files/file_tracing.cc:15: AtomicWord g_provider = reinterpret_cast<AtomicWord>(nullptr); as a global, it would be default-initialized to 0 anyway right?
https://codereview.chromium.org/2114993003/diff/60001/base/files/file_tracing.cc File base/files/file_tracing.cc (right): https://codereview.chromium.org/2114993003/diff/60001/base/files/file_tracing... base/files/file_tracing.cc:15: AtomicWord g_provider = reinterpret_cast<AtomicWord>(nullptr); On 2016/07/15 22:29:50, danakj wrote: > as a global, it would be default-initialized to 0 anyway right? Done.
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2114993003/#ps80001 (title: "silly quirks are silly")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/14 10:15:20, Alexander Potapenko wrote: > Rachel is actually right that Acquire_Load is not strictly necessary here. What > we need instead is a load with std::memory_order_consume. > Unfortunately this doesn't exist in the base::subtle memory model, and > std::atomic is still forbidden (IIRC). Not relevant to CL directly, but for the poor soul who'll inevitably stumble upon this[1]: IIRC, memory_order_consume is also still being worked on because the current standard definition has flaws: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0190r0.pdf [1]: Likely myself, in three months. DONT DO IT! > > Using a NoBarrier_Load() instead of Acquire_Load() will have the following > drawbacks: > - TSan won't understand it, we'll need a suppression or an "#ifdef > THREAD_SANITIZER" workaround > - when we replace base::subtle with std::atomic, a clever enough compiler can > break this code. Unfortunately, compilers continue to out-clever us, so yay for Release/Acquire
Message was sent while issue was closed.
Description was changed from ========== Access FileTracing::g_provider atomically to prevent data races R=danakj@chromium.org BUG=522018 ========== to ========== Access FileTracing::g_provider atomically to prevent data races R=danakj@chromium.org BUG=522018 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Access FileTracing::g_provider atomically to prevent data races R=danakj@chromium.org BUG=522018 ========== to ========== Access FileTracing::g_provider atomically to prevent data races R=danakj@chromium.org BUG=522018 Committed: https://crrev.com/00dd6010cdda17cecd624c0f274aeb630f31fb83 Cr-Commit-Position: refs/heads/master@{#405907} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/00dd6010cdda17cecd624c0f274aeb630f31fb83 Cr-Commit-Position: refs/heads/master@{#405907} |
