Skip to content

Commit

Permalink
Rollup merge of rust-lang#122729 - m-ou-se:relax, r=Amanieu
Browse files Browse the repository at this point in the history
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`
  • Loading branch information
GuillaumeGomez authored Mar 20, 2024
2 parents 04fae8c + 3462175 commit 87aac9d
Show file tree
Hide file tree
Showing 17 changed files with 70 additions and 59 deletions.
2 changes: 1 addition & 1 deletion library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ macro_rules! acquire {
/// let val = Arc::clone(&val);
///
/// thread::spawn(move || {
/// let v = val.fetch_add(1, Ordering::SeqCst);
/// let v = val.fetch_add(1, Ordering::Relaxed);
/// println!("{v:?}");
/// });
/// }
Expand Down
9 changes: 3 additions & 6 deletions library/core/src/alloc/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ use crate::ptr;
/// use std::alloc::{GlobalAlloc, Layout};
/// use std::cell::UnsafeCell;
/// use std::ptr::null_mut;
/// use std::sync::atomic::{
/// AtomicUsize,
/// Ordering::{Acquire, SeqCst},
/// };
/// use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};
///
/// const ARENA_SIZE: usize = 128 * 1024;
/// const MAX_SUPPORTED_ALIGN: usize = 4096;
Expand Down Expand Up @@ -61,7 +58,7 @@ use crate::ptr;
/// let mut allocated = 0;
/// if self
/// .remaining
/// .fetch_update(SeqCst, SeqCst, |mut remaining| {
/// .fetch_update(Relaxed, Relaxed, |mut remaining| {
/// if size > remaining {
/// return None;
/// }
Expand All @@ -81,7 +78,7 @@ use crate::ptr;
///
/// fn main() {
/// let _s = format!("allocating a string!");
/// let currently = ALLOCATOR.remaining.load(Acquire);
/// let currently = ALLOCATOR.remaining.load(Relaxed);
/// println!("allocated so far: {}", ARENA_SIZE - currently);
/// }
/// ```
Expand Down
2 changes: 1 addition & 1 deletion library/panic_unwind/src/emcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> {
super::__rust_foreign_exception();
}

let was_caught = (*adjusted_ptr).caught.swap(true, Ordering::SeqCst);
let was_caught = (*adjusted_ptr).caught.swap(true, Ordering::Relaxed);
if was_caught {
// Since cleanup() isn't allowed to panic, we just abort instead.
intrinsics::abort();
Expand Down
4 changes: 2 additions & 2 deletions library/proc_macro/src/bridge/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ impl<T> OwnedStore<T> {
pub(super) fn new(counter: &'static AtomicU32) -> Self {
// Ensure the handle counter isn't 0, which would panic later,
// when `NonZero::new` (aka `Handle::new`) is called in `alloc`.
assert_ne!(counter.load(Ordering::SeqCst), 0);
assert_ne!(counter.load(Ordering::Relaxed), 0);

OwnedStore { counter, data: BTreeMap::new() }
}
}

impl<T> OwnedStore<T> {
pub(super) fn alloc(&mut self, x: T) -> Handle {
let counter = self.counter.fetch_add(1, Ordering::SeqCst);
let counter = self.counter.fetch_add(1, Ordering::Relaxed);
let handle = Handle::new(counter).expect("`proc_macro` handle counter overflowed");
assert!(self.data.insert(handle, x).is_none());
handle
Expand Down
6 changes: 3 additions & 3 deletions library/std/src/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ static HOOK: AtomicPtr<()> = AtomicPtr::new(ptr::null_mut());
/// ```
#[unstable(feature = "alloc_error_hook", issue = "51245")]
pub fn set_alloc_error_hook(hook: fn(Layout)) {
HOOK.store(hook as *mut (), Ordering::SeqCst);
HOOK.store(hook as *mut (), Ordering::Release);
}

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

Expand All @@ -362,7 +362,7 @@ fn default_alloc_error_hook(layout: Layout) {
#[alloc_error_handler]
#[unstable(feature = "alloc_internals", issue = "none")]
pub fn rust_oom(layout: Layout) -> ! {
let hook = HOOK.load(Ordering::SeqCst);
let hook = HOOK.load(Ordering::Acquire);
let hook: fn(Layout) =
if hook.is_null() { default_alloc_error_hook } else { unsafe { mem::transmute(hook) } };
hook(layout);
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/net/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ use crate::sync::atomic::{AtomicUsize, Ordering};
static PORT: AtomicUsize = AtomicUsize::new(0);

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

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

Expand Down
2 changes: 1 addition & 1 deletion library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ fn default_hook(info: &PanicInfo<'_>) {
drop(backtrace::print(err, crate::backtrace_rs::PrintFmt::Full))
}
Some(BacktraceStyle::Off) => {
if FIRST_PANIC.swap(false, Ordering::SeqCst) {
if FIRST_PANIC.swap(false, Ordering::Relaxed) {
let _ = writeln!(
err,
"note: run with `RUST_BACKTRACE=1` environment variable to display a \
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sync/condvar/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,14 @@ fn wait_timeout_wake() {
let t = thread::spawn(move || {
let _g = m2.lock().unwrap();
thread::sleep(Duration::from_millis(1));
notified_copy.store(true, Ordering::SeqCst);
notified_copy.store(true, Ordering::Relaxed);
c2.notify_one();
});
let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u64::MAX)).unwrap();
assert!(!timeout_res.timed_out());
// spurious wakeups mean this isn't necessarily true
// so execute test again, if not notified
if !notified.load(Ordering::SeqCst) {
if !notified.load(Ordering::Relaxed) {
t.join().unwrap();
continue;
}
Expand Down
22 changes: 12 additions & 10 deletions library/std/src/sys/pal/unix/thread_parking/pthread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::marker::PhantomPinned;
use crate::pin::Pin;
use crate::ptr::addr_of_mut;
use crate::sync::atomic::AtomicUsize;
use crate::sync::atomic::Ordering::SeqCst;
use crate::sync::atomic::Ordering::{Acquire, Relaxed, Release};
#[cfg(not(target_os = "nto"))]
use crate::sys::time::TIMESPEC_MAX;
#[cfg(target_os = "nto")]
Expand Down Expand Up @@ -150,16 +150,18 @@ impl Parker {

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

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

unlock(self.lock.get());

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

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

lock(self.lock.get());
match self.state.compare_exchange(EMPTY, PARKED, SeqCst, SeqCst) {
match self.state.compare_exchange(EMPTY, PARKED, Relaxed, Relaxed) {
Ok(_) => {}
Err(NOTIFIED) => {
// We must read again here, see `park`.
let old = self.state.swap(EMPTY, SeqCst);
let old = self.state.swap(EMPTY, Acquire);
unlock(self.lock.get());

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

match self.state.swap(EMPTY, SeqCst) {
match self.state.swap(EMPTY, Acquire) {
NOTIFIED => unlock(self.lock.get()), // got a notification, hurray!
PARKED => unlock(self.lock.get()), // no notification, alas
n => {
Expand All @@ -245,7 +247,7 @@ impl Parker {
// `state` is already `NOTIFIED`. That is why this must be a swap
// rather than a compare-and-swap that returns if it reads `NOTIFIED`
// on failure.
match self.state.swap(NOTIFIED, SeqCst) {
match self.state.swap(NOTIFIED, Release) {
EMPTY => return, // no one was waiting
NOTIFIED => return, // already unparked
PARKED => {} // gotta go wake someone up
Expand Down
9 changes: 6 additions & 3 deletions library/std/src/sys/pal/wasm/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,18 @@ unsafe impl GlobalAlloc for System {

#[cfg(target_feature = "atomics")]
mod lock {
use crate::sync::atomic::{AtomicI32, Ordering::SeqCst};
use crate::sync::atomic::{
AtomicI32,
Ordering::{Acquire, Release},
};

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

pub struct DropLock;

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

impl Drop for DropLock {
fn drop(&mut self) {
let r = LOCKED.swap(0, SeqCst);
let r = LOCKED.swap(0, Release);
debug_assert_eq!(r, 1);

// Note that due to the above logic we don't actually need to wake
Expand Down
8 changes: 4 additions & 4 deletions library/std/src/sys/pal/windows/pipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::path::Path;
use crate::ptr;
use crate::slice;
use crate::sync::atomic::AtomicUsize;
use crate::sync::atomic::Ordering::SeqCst;
use crate::sync::atomic::Ordering::Relaxed;
use crate::sys::c;
use crate::sys::fs::{File, OpenOptions};
use crate::sys::handle::Handle;
Expand Down Expand Up @@ -214,11 +214,11 @@ pub fn spawn_pipe_relay(
fn random_number() -> usize {
static N: AtomicUsize = AtomicUsize::new(0);
loop {
if N.load(SeqCst) != 0 {
return N.fetch_add(1, SeqCst);
if N.load(Relaxed) != 0 {
return N.fetch_add(1, Relaxed);
}

N.store(hashmap_random_keys().0 as usize, SeqCst);
N.store(hashmap_random_keys().0 as usize, Relaxed);
}
}

Expand Down
9 changes: 6 additions & 3 deletions library/std/src/sys/pal/xous/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,18 @@ unsafe impl GlobalAlloc for System {
}

mod lock {
use crate::sync::atomic::{AtomicI32, Ordering::SeqCst};
use crate::sync::atomic::{
AtomicI32,
Ordering::{Acquire, Release},
};

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

pub struct DropLock;

pub fn lock() -> DropLock {
loop {
if LOCKED.swap(1, SeqCst) == 0 {
if LOCKED.swap(1, Acquire) == 0 {
return DropLock;
}
crate::os::xous::ffi::do_yield();
Expand All @@ -63,7 +66,7 @@ mod lock {

impl Drop for DropLock {
fn drop(&mut self) {
let r = LOCKED.swap(0, SeqCst);
let r = LOCKED.swap(0, Release);
debug_assert_eq!(r, 1);
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/pal/xous/net/tcpstream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ impl TcpStream {
}

pub fn set_nonblocking(&self, nonblocking: bool) -> io::Result<()> {
self.nonblocking.store(nonblocking, Ordering::SeqCst);
self.nonblocking.store(nonblocking, Ordering::Relaxed);
Ok(())
}
}
Expand Down
10 changes: 5 additions & 5 deletions library/std/src/sys/pal/xous/thread_local_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::mem::ManuallyDrop;
use crate::ptr;
use crate::sync::atomic::AtomicPtr;
use crate::sync::atomic::AtomicUsize;
use crate::sync::atomic::Ordering::SeqCst;
use crate::sync::atomic::Ordering::{Acquire, Relaxed, Release};
use core::arch::asm;

use crate::os::xous::ffi::{map_memory, unmap_memory, MemoryFlags};
Expand Down Expand Up @@ -92,7 +92,7 @@ fn tls_table() -> &'static mut [*mut u8] {
pub unsafe fn create(dtor: Option<Dtor>) -> Key {
// Allocate a new TLS key. These keys are shared among all threads.
#[allow(unused_unsafe)]
let key = unsafe { TLS_KEY_INDEX.fetch_add(1, SeqCst) };
let key = unsafe { TLS_KEY_INDEX.fetch_add(1, Relaxed) };
if let Some(f) = dtor {
unsafe { register_dtor(key, f) };
}
Expand Down Expand Up @@ -154,11 +154,11 @@ unsafe fn register_dtor(key: Key, dtor: Dtor) {
let mut node = ManuallyDrop::new(Box::new(Node { key, dtor, next: ptr::null_mut() }));

#[allow(unused_unsafe)]
let mut head = unsafe { DTORS.load(SeqCst) };
let mut head = unsafe { DTORS.load(Acquire) };
loop {
node.next = head;
#[allow(unused_unsafe)]
match unsafe { DTORS.compare_exchange(head, &mut **node, SeqCst, SeqCst) } {
match unsafe { DTORS.compare_exchange(head, &mut **node, Release, Acquire) } {
Ok(_) => return, // nothing to drop, we successfully added the node to the list
Err(cur) => head = cur,
}
Expand Down Expand Up @@ -199,7 +199,7 @@ unsafe fn run_dtors() {
}
any_run = false;
#[allow(unused_unsafe)]
let mut cur = unsafe { DTORS.load(SeqCst) };
let mut cur = unsafe { DTORS.load(Acquire) };
while !cur.is_null() {
let ptr = unsafe { get((*cur).key) };

Expand Down
11 changes: 7 additions & 4 deletions library/std/src/sys/sync/mutex/xous.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use crate::os::xous::ffi::{blocking_scalar, do_yield};
use crate::os::xous::services::{ticktimer_server, TicktimerScalar};
use crate::sync::atomic::{AtomicBool, AtomicUsize, Ordering::Relaxed, Ordering::SeqCst};
use crate::sync::atomic::{
AtomicBool, AtomicUsize,
Ordering::{Acquire, Relaxed, Release},
};

pub struct Mutex {
/// The "locked" value indicates how many threads are waiting on this
Expand Down Expand Up @@ -68,7 +71,7 @@ impl Mutex {

#[inline]
pub unsafe fn unlock(&self) {
let prev = self.locked.fetch_sub(1, SeqCst);
let prev = self.locked.fetch_sub(1, Release);

// If the previous value was 1, then this was a "fast path" unlock, so no
// need to involve the Ticktimer server
Expand All @@ -89,12 +92,12 @@ impl Mutex {

#[inline]
pub unsafe fn try_lock(&self) -> bool {
self.locked.compare_exchange(0, 1, SeqCst, SeqCst).is_ok()
self.locked.compare_exchange(0, 1, Acquire, Relaxed).is_ok()
}

#[inline]
pub unsafe fn try_lock_or_poison(&self) -> bool {
self.locked.fetch_add(1, SeqCst) == 0
self.locked.fetch_add(1, Acquire) == 0
}
}

Expand Down
Loading

0 comments on commit 87aac9d

Please sign in to comment.