3party/crashpad
0
0
Fork 0
mirror of https://github.com/chromium/crashpad.git synced 2024-12-26 06:31:50 +08:00
Code Issues Projects Releases Wiki Activity

[ScopedSpinGuard] Use std::atomic::compare_exchange_strong() for spinlock

Browse Source 
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
Change-Id: I2a08031db6b219d7d14a5cd02b3634985f81ab06
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5545257
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Ben Hamilton <benhamilton@google.com>
This commit is contained in:
Ben Hamilton 2024-05-16 10:11:56 -06:00 committed by Crashpad LUCI CQ
parent 0e043ccf70
commit d588c50b16
1 changed files with 11 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
util/synchronization/scoped_spin_guard.h
View File

@ -73,10 +73,17 @@ class ScopedSpinGuard final {
ClockMonotonicNanoseconds() + timeout_nanos; ClockMonotonicNanoseconds() + timeout_nanos;
while (true) { while (true) {
bool expected_current_value = false; bool expected_current_value = false;
if (state.locked.compare_exchange_weak(expected_current_value, // `std::atomic::compare_exchange_weak()` is allowed to spuriously fail on
true, // architectures like ARM, which can cause timeouts even for
std::memory_order_acquire, // single-threaded cases (https://crbug.com/340980960,
std::memory_order_relaxed)) { // http://b/296082201).
//
// Use `std::compare_exchange_strong()` instead to avoid spurious failures
// in the common case.
if (state.locked.compare_exchange_strong(expected_current_value,
true,
std::memory_order_acquire,
std::memory_order_relaxed)) {
return std::make_optional<ScopedSpinGuard>(state); return std::make_optional<ScopedSpinGuard>(state);
} }
if (ClockMonotonicNanoseconds() >= clock_end_time_nanos) { if (ClockMonotonicNanoseconds() >= clock_end_time_nanos) {