Skip to content

Commit 43ad753

Browse files
authored
Rollup merge of #122729 - m-ou-se:relax, r=Amanieu
Relax SeqCst ordering in standard library. Every single SeqCst in the standard library is unnecessary. In all cases, Relaxed or Release+Acquire was sufficient. As I [wrote](https://marabos.nl/atomics/memory-ordering.html#common-misconceptions) in my book on atomics: > [..] when reading code, SeqCst basically tells the reader: "this operation depends on the total order of every single SeqCst operation in the program," which is an incredibly far-reaching claim. The same code would likely be easier to review and verify if it used weaker memory ordering instead, if possible. For example, Release effectively tells the reader: "this relates to an acquire operation on the same variable," which involves far fewer considerations when forming an understanding of the code. > > It is advisable to see SeqCst as a warning sign. Seeing it in the wild often means that either something complicated is going on, or simply that the author did not take the time to analyze their memory ordering related assumptions, both of which are reasons for extra scrutiny. r? ````@Amanieu```` ````@joboet````
2 parents 31adfd7 + 3462175 commit 43ad753

File tree

17 files changed

+70
-59
lines changed

17 files changed

+70
-59
lines changed

library/alloc/src/sync.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ macro_rules! acquire {
233233
/// let val = Arc::clone(&val);
234234
///
235235
/// thread::spawn(move || {
236-
/// let v = val.fetch_add(1, Ordering::SeqCst);
236+
/// let v = val.fetch_add(1, Ordering::Relaxed);
237237
/// println!("{v:?}");
238238
/// });
239239
/// }

library/core/src/alloc/global.rs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,7 @@ use crate::ptr;
2424
/// use std::alloc::{GlobalAlloc, Layout};
2525
/// use std::cell::UnsafeCell;
2626
/// use std::ptr::null_mut;
27-
/// use std::sync::atomic::{
28-
/// AtomicUsize,
29-
/// Ordering::{Acquire, SeqCst},
30-
/// };
27+
/// use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};
3128
///
3229
/// const ARENA_SIZE: usize = 128 * 1024;
3330
/// const MAX_SUPPORTED_ALIGN: usize = 4096;
@@ -61,7 +58,7 @@ use crate::ptr;
6158
/// let mut allocated = 0;
6259
/// if self
6360
/// .remaining
64-
/// .fetch_update(SeqCst, SeqCst, |mut remaining| {
61+
/// .fetch_update(Relaxed, Relaxed, |mut remaining| {
6562
/// if size > remaining {
6663
/// return None;
6764
/// }
@@ -81,7 +78,7 @@ use crate::ptr;
8178
///
8279
/// fn main() {
8380
/// let _s = format!("allocating a string!");
84-
/// let currently = ALLOCATOR.remaining.load(Acquire);
81+
/// let currently = ALLOCATOR.remaining.load(Relaxed);
8582
/// println!("allocated so far: {}", ARENA_SIZE - currently);
8683
/// }
8784
/// ```

library/panic_unwind/src/emcc.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> {
8484
super::__rust_foreign_exception();
8585
}
8686

87-
let was_caught = (*adjusted_ptr).caught.swap(true, Ordering::SeqCst);
87+
let was_caught = (*adjusted_ptr).caught.swap(true, Ordering::Relaxed);
8888
if was_caught {
8989
// Since cleanup() isn't allowed to panic, we just abort instead.
9090
intrinsics::abort();

library/proc_macro/src/bridge/handle.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,15 @@ impl<T> OwnedStore<T> {
2121
pub(super) fn new(counter: &'static AtomicU32) -> Self {
2222
// Ensure the handle counter isn't 0, which would panic later,
2323
// when `NonZero::new` (aka `Handle::new`) is called in `alloc`.
24-
assert_ne!(counter.load(Ordering::SeqCst), 0);
24+
assert_ne!(counter.load(Ordering::Relaxed), 0);
2525

2626
OwnedStore { counter, data: BTreeMap::new() }
2727
}
2828
}
2929

3030
impl<T> OwnedStore<T> {
3131
pub(super) fn alloc(&mut self, x: T) -> Handle {
32-
let counter = self.counter.fetch_add(1, Ordering::SeqCst);
32+
let counter = self.counter.fetch_add(1, Ordering::Relaxed);
3333
let handle = Handle::new(counter).expect("`proc_macro` handle counter overflowed");
3434
assert!(self.data.insert(handle, x).is_none());
3535
handle

library/std/src/alloc.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,7 @@ static HOOK: AtomicPtr<()> = AtomicPtr::new(ptr::null_mut());
329329
/// ```
330330
#[unstable(feature = "alloc_error_hook", issue = "51245")]
331331
pub fn set_alloc_error_hook(hook: fn(Layout)) {
332-
HOOK.store(hook as *mut (), Ordering::SeqCst);
332+
HOOK.store(hook as *mut (), Ordering::Release);
333333
}
334334

335335
/// Unregisters the current allocation error hook, returning it.
@@ -339,7 +339,7 @@ pub fn set_alloc_error_hook(hook: fn(Layout)) {
339339
/// If no custom hook is registered, the default hook will be returned.
340340
#[unstable(feature = "alloc_error_hook", issue = "51245")]
341341
pub fn take_alloc_error_hook() -> fn(Layout) {
342-
let hook = HOOK.swap(ptr::null_mut(), Ordering::SeqCst);
342+
let hook = HOOK.swap(ptr::null_mut(), Ordering::Acquire);
343343
if hook.is_null() { default_alloc_error_hook } else { unsafe { mem::transmute(hook) } }
344344
}
345345

@@ -362,7 +362,7 @@ fn default_alloc_error_hook(layout: Layout) {
362362
#[alloc_error_handler]
363363
#[unstable(feature = "alloc_internals", issue = "none")]
364364
pub fn rust_oom(layout: Layout) -> ! {
365-
let hook = HOOK.load(Ordering::SeqCst);
365+
let hook = HOOK.load(Ordering::Acquire);
366366
let hook: fn(Layout) =
367367
if hook.is_null() { default_alloc_error_hook } else { unsafe { mem::transmute(hook) } };
368368
hook(layout);

library/std/src/net/test.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@ use crate::sync::atomic::{AtomicUsize, Ordering};
77
static PORT: AtomicUsize = AtomicUsize::new(0);
88

99
pub fn next_test_ip4() -> SocketAddr {
10-
let port = PORT.fetch_add(1, Ordering::SeqCst) as u16 + base_port();
10+
let port = PORT.fetch_add(1, Ordering::Relaxed) as u16 + base_port();
1111
SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), port))
1212
}
1313

1414
pub fn next_test_ip6() -> SocketAddr {
15-
let port = PORT.fetch_add(1, Ordering::SeqCst) as u16 + base_port();
15+
let port = PORT.fetch_add(1, Ordering::Relaxed) as u16 + base_port();
1616
SocketAddr::V6(SocketAddrV6::new(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1), port, 0, 0))
1717
}
1818

library/std/src/panicking.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ fn default_hook(info: &PanicInfo<'_>) {
272272
drop(backtrace::print(err, crate::backtrace_rs::PrintFmt::Full))
273273
}
274274
Some(BacktraceStyle::Off) => {
275-
if FIRST_PANIC.swap(false, Ordering::SeqCst) {
275+
if FIRST_PANIC.swap(false, Ordering::Relaxed) {
276276
let _ = writeln!(
277277
err,
278278
"note: run with `RUST_BACKTRACE=1` environment variable to display a \

library/std/src/sync/condvar/tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,14 +170,14 @@ fn wait_timeout_wake() {
170170
let t = thread::spawn(move || {
171171
let _g = m2.lock().unwrap();
172172
thread::sleep(Duration::from_millis(1));
173-
notified_copy.store(true, Ordering::SeqCst);
173+
notified_copy.store(true, Ordering::Relaxed);
174174
c2.notify_one();
175175
});
176176
let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u64::MAX)).unwrap();
177177
assert!(!timeout_res.timed_out());
178178
// spurious wakeups mean this isn't necessarily true
179179
// so execute test again, if not notified
180-
if !notified.load(Ordering::SeqCst) {
180+
if !notified.load(Ordering::Relaxed) {
181181
t.join().unwrap();
182182
continue;
183183
}

library/std/src/sys/pal/unix/thread_parking/pthread.rs

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::marker::PhantomPinned;
55
use crate::pin::Pin;
66
use crate::ptr::addr_of_mut;
77
use crate::sync::atomic::AtomicUsize;
8-
use crate::sync::atomic::Ordering::SeqCst;
8+
use crate::sync::atomic::Ordering::{Acquire, Relaxed, Release};
99
#[cfg(not(target_os = "nto"))]
1010
use crate::sys::time::TIMESPEC_MAX;
1111
#[cfg(target_os = "nto")]
@@ -150,16 +150,18 @@ impl Parker {
150150

151151
// This implementation doesn't require `unsafe`, but other implementations
152152
// may assume this is only called by the thread that owns the Parker.
153+
//
154+
// For memory ordering, see std/src/sys_common/thread_parking/futex.rs
153155
pub unsafe fn park(self: Pin<&Self>) {
154156
// If we were previously notified then we consume this notification and
155157
// return quickly.
156-
if self.state.compare_exchange(NOTIFIED, EMPTY, SeqCst, SeqCst).is_ok() {
158+
if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Relaxed).is_ok() {
157159
return;
158160
}
159161

160162
// Otherwise we need to coordinate going to sleep
161163
lock(self.lock.get());
162-
match self.state.compare_exchange(EMPTY, PARKED, SeqCst, SeqCst) {
164+
match self.state.compare_exchange(EMPTY, PARKED, Relaxed, Relaxed) {
163165
Ok(_) => {}
164166
Err(NOTIFIED) => {
165167
// We must read here, even though we know it will be `NOTIFIED`.
@@ -168,7 +170,7 @@ impl Parker {
168170
// acquire operation that synchronizes with that `unpark` to observe
169171
// any writes it made before the call to unpark. To do that we must
170172
// read from the write it made to `state`.
171-
let old = self.state.swap(EMPTY, SeqCst);
173+
let old = self.state.swap(EMPTY, Acquire);
172174

173175
unlock(self.lock.get());
174176

@@ -185,7 +187,7 @@ impl Parker {
185187
loop {
186188
wait(self.cvar.get(), self.lock.get());
187189

188-
match self.state.compare_exchange(NOTIFIED, EMPTY, SeqCst, SeqCst) {
190+
match self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Relaxed) {
189191
Ok(_) => break, // got a notification
190192
Err(_) => {} // spurious wakeup, go back to sleep
191193
}
@@ -201,16 +203,16 @@ impl Parker {
201203
// Like `park` above we have a fast path for an already-notified thread, and
202204
// afterwards we start coordinating for a sleep.
203205
// return quickly.
204-
if self.state.compare_exchange(NOTIFIED, EMPTY, SeqCst, SeqCst).is_ok() {
206+
if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Relaxed).is_ok() {
205207
return;
206208
}
207209

208210
lock(self.lock.get());
209-
match self.state.compare_exchange(EMPTY, PARKED, SeqCst, SeqCst) {
211+
match self.state.compare_exchange(EMPTY, PARKED, Relaxed, Relaxed) {
210212
Ok(_) => {}
211213
Err(NOTIFIED) => {
212214
// We must read again here, see `park`.
213-
let old = self.state.swap(EMPTY, SeqCst);
215+
let old = self.state.swap(EMPTY, Acquire);
214216
unlock(self.lock.get());
215217

216218
assert_eq!(old, NOTIFIED, "park state changed unexpectedly");
@@ -228,7 +230,7 @@ impl Parker {
228230
// parked.
229231
wait_timeout(self.cvar.get(), self.lock.get(), dur);
230232

231-
match self.state.swap(EMPTY, SeqCst) {
233+
match self.state.swap(EMPTY, Acquire) {
232234
NOTIFIED => unlock(self.lock.get()), // got a notification, hurray!
233235
PARKED => unlock(self.lock.get()), // no notification, alas
234236
n => {
@@ -245,7 +247,7 @@ impl Parker {
245247
// `state` is already `NOTIFIED`. That is why this must be a swap
246248
// rather than a compare-and-swap that returns if it reads `NOTIFIED`
247249
// on failure.
248-
match self.state.swap(NOTIFIED, SeqCst) {
250+
match self.state.swap(NOTIFIED, Release) {
249251
EMPTY => return, // no one was waiting
250252
NOTIFIED => return, // already unparked
251253
PARKED => {} // gotta go wake someone up

library/std/src/sys/pal/wasm/alloc.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,18 @@ unsafe impl GlobalAlloc for System {
5757

5858
#[cfg(target_feature = "atomics")]
5959
mod lock {
60-
use crate::sync::atomic::{AtomicI32, Ordering::SeqCst};
60+
use crate::sync::atomic::{
61+
AtomicI32,
62+
Ordering::{Acquire, Release},
63+
};
6164

6265
static LOCKED: AtomicI32 = AtomicI32::new(0);
6366

6467
pub struct DropLock;
6568

6669
pub fn lock() -> DropLock {
6770
loop {
68-
if LOCKED.swap(1, SeqCst) == 0 {
71+
if LOCKED.swap(1, Acquire) == 0 {
6972
return DropLock;
7073
}
7174
// Ok so here's where things get a little depressing. At this point
@@ -143,7 +146,7 @@ mod lock {
143146

144147
impl Drop for DropLock {
145148
fn drop(&mut self) {
146-
let r = LOCKED.swap(0, SeqCst);
149+
let r = LOCKED.swap(0, Release);
147150
debug_assert_eq!(r, 1);
148151

149152
// Note that due to the above logic we don't actually need to wake

0 commit comments

Comments
 (0)