[ScopedSpinGuard] Use std::atomic::compare_exchange_strong() for spinlock [crashpad/crashpad : cxx17]
1 view
Skip to first unread message
Ben Hamilton (Gerrit)
Jun 13, 2024, 4:52:41 PMJun 13
to Justin Cohen, Mark Mentovai, crashp...@chromium.org
Attention needed from Justin Cohen and Mark Mentovai
Ben Hamilton added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Justin Cohen
- Mark Mentovai
Submit Requirements:
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Jun 14, 2024, 11:26:27 AMJun 14
to Ben Hamilton, Mark Mentovai, crashp...@chromium.org
Attention needed from Ben Hamilton and Mark Mentovai
Justin Cohen voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Ben Hamilton
- Mark Mentovai
Submit Requirements:
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Jun 14, 2024, 11:33:13 AMJun 14
to Justin Cohen, Mark Mentovai, crashp...@chromium.org
Attention needed from Mark Mentovai
Ben Hamilton voted Commit-Queue+2![]( "Open in Gerrit")
Attention is currently required from:
- Mark Mentovai
Submit Requirements:
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Jun 14, 2024, 11:41:46 AMJun 14
to Crashpad LUCI CQ, Justin Cohen, Mark Mentovai, crashp...@chromium.org
Attention needed from Mark Mentovai
Ben Hamilton added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Ben Hamilton . resolved
Hmm. The `cxx17` branch was forked from `8df174c64ca2b9dc0f83b089d30760867966b173`:
https://chromium.googlesource.com/crashpad/crashpad/+/refs/heads/cxx17
which previously built OK. Maybe something in the builder infra changed and I need to cherry-pick https://crrev.com/c/5585353 first?
Related details
Attention is currently required from:
- Mark Mentovai
Submit Requirements:
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Jun 14, 2024, 12:01:12 PMJun 14
to Crashpad LUCI CQ, Justin Cohen, Mark Mentovai, crashp...@chromium.org
Ben Hamilton submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[ScopedSpinGuard] Use std::atomic::compare_exchange_strong() for spinlock
Previously, ScopedSpinGuard used std::atomic::compare_exchange_weak()
in a loop to implement a spinlock. After looping for the specified
number of nanoseconds, it would give up and return an error.
A few bugs have come in on ARM platforms (https://crbug.com/340980960,
http://b/296082201) which indicate that this can fail even in
single-threaded cases where nothing else has the spinlock.
From https://cbloomrants.blogspot.com/2011/07/07-14-11-compareexchangestrong-vs.html :
> compare_exchange_weak exists for LL-SC (load linked/store
> conditional) type architectures (Power, ARM, basically everything
> except x86), because on them compare_exchange_strong must be
> implemented as a loop, while compare_exchange_weak can be
> non-looping.
and:
https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange#Notes
> compare_exchange_weak is allowed to fail spuriously, that is, acts
> as if *this != expected even if they are equal. When a
> compare-and-exchange is in a loop, compare_exchange_weak will yield
> better performance on some platforms.
>
> When compare_exchange_weak would require a loop and
> compare_exchange_strong would not, compare_exchange_strong is
> preferable [...]
My conclusion is that this logic needs to use
`compare_exchange_strong` to avoid spurious failures on ARM in the
common case when there's no other thread holding the spinlock.
Change-Id: I2a08031db6b219d7d14a5cd02b3634985f81ab06
Bug: b:340980960,b:296082201
Change-Id: I2a08031db6b219d7d14a5cd02b3634985f81ab06
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5545257
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Commit-Queue: Ben Hamilton <benha...@google.com>
(cherry picked from commit d588c50b16f735b0c069538651d06bdd9490c6f3)
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5631682
Reviewed-by: Justin Cohen <justi...@chromium.org>
Files:
- M util/synchronization/scoped_spin_guard.h
Change size: S
Delta: 1 file changed, 11 insertions(+), 4 deletions(-)
Branch: refs/heads/cxx17
Submit Requirements:
Code-Review: +1 by Justin Cohen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Jun 14, 2024, 1:40:47 PMJun 14
to Ben Hamilton, Crashpad LUCI CQ, Justin Cohen, crashp...@chromium.org
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Mark Mentovai . unresolved
Ben, in a separate change, can you also get the DEPS file on this branch pointing to the right place? I think we branched mini_chromium at a different point than the Crashpad cxx17 branch’s DEPS file is pointing to.
Related details
Attention set is empty
Submit Requirements:
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages