Skip to content

Commit

Permalink
Rollup merge of #96040 - m-ou-se:futex-u32, r=Amanieu
Browse files Browse the repository at this point in the history
Use u32 instead of i32 for futexes.

This changes futexes from i32 to u32. The [Linux man page](https://man7.org/linux/man-pages/man2/futex.2.html) uses `uint32_t` for them, so I'm not sure why I used i32 for them. Maybe because I first used them for thread parkers, where I used -1, 0, and 1 as the states.

(Wasm's `memory.atomic.wait32` does use `i32`, because wasm doesn't support `u32`.)

It doesn't matter much, but using the unsigned type probably results in fewer surprises when shifting bits around or using comparison operators.

r? `@Amanieu`
  • Loading branch information
Dylan-DPC authored Apr 14, 2022
2 parents 599af74 + 7a35c0f commit 5c4d9f8
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 53 deletions.
30 changes: 14 additions & 16 deletions library/std/src/sys/unix/futex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
all(target_os = "emscripten", target_feature = "atomics")
))]

use crate::sync::atomic::AtomicI32;
use crate::sync::atomic::AtomicU32;
use crate::time::Duration;

/// Wait for a futex_wake operation to wake us.
Expand All @@ -13,7 +13,7 @@ use crate::time::Duration;
///
/// Returns false on timeout, and true in all other cases.
#[cfg(any(target_os = "linux", target_os = "android"))]
pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option<Duration>) -> bool {
pub fn futex_wait(futex: &AtomicU32, expected: u32, timeout: Option<Duration>) -> bool {
use super::time::Timespec;
use crate::ptr::null;
use crate::sync::atomic::Ordering::Relaxed;
Expand All @@ -35,7 +35,7 @@ pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option<Duration>) -
let r = unsafe {
libc::syscall(
libc::SYS_futex,
futex as *const AtomicI32,
futex as *const AtomicU32,
libc::FUTEX_WAIT_BITSET | libc::FUTEX_PRIVATE_FLAG,
expected,
timespec.as_ref().map_or(null(), |t| &t.t as *const libc::timespec),
Expand All @@ -53,21 +53,19 @@ pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option<Duration>) -
}

#[cfg(target_os = "emscripten")]
pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option<Duration>) {
pub fn futex_wait(futex: &AtomicU32, expected: u32, timeout: Option<Duration>) {
extern "C" {
fn emscripten_futex_wait(
addr: *const AtomicI32,
addr: *const AtomicU32,
val: libc::c_uint,
max_wait_ms: libc::c_double,
) -> libc::c_int;
}

unsafe {
emscripten_futex_wait(
futex as *const AtomicI32,
// `val` is declared unsigned to match the Emscripten headers, but since it's used as
// an opaque value, we can ignore the meaning of signed vs. unsigned and cast here.
expected as libc::c_uint,
futex,
expected,
timeout.map_or(crate::f64::INFINITY, |d| d.as_secs_f64() * 1000.0),
);
}
Expand All @@ -78,11 +76,11 @@ pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option<Duration>) {
/// Returns true if this actually woke up such a thread,
/// or false if no thread was waiting on this futex.
#[cfg(any(target_os = "linux", target_os = "android"))]
pub fn futex_wake(futex: &AtomicI32) -> bool {
pub fn futex_wake(futex: &AtomicU32) -> bool {
unsafe {
libc::syscall(
libc::SYS_futex,
futex as *const AtomicI32,
futex as *const AtomicU32,
libc::FUTEX_WAKE | libc::FUTEX_PRIVATE_FLAG,
1,
) > 0
Expand All @@ -91,22 +89,22 @@ pub fn futex_wake(futex: &AtomicI32) -> bool {

/// Wake up all threads that are waiting on futex_wait on this futex.
#[cfg(any(target_os = "linux", target_os = "android"))]
pub fn futex_wake_all(futex: &AtomicI32) {
pub fn futex_wake_all(futex: &AtomicU32) {
unsafe {
libc::syscall(
libc::SYS_futex,
futex as *const AtomicI32,
futex as *const AtomicU32,
libc::FUTEX_WAKE | libc::FUTEX_PRIVATE_FLAG,
i32::MAX,
);
}
}

#[cfg(target_os = "emscripten")]
pub fn futex_wake(futex: &AtomicI32) -> bool {
pub fn futex_wake(futex: &AtomicU32) -> bool {
extern "C" {
fn emscripten_futex_wake(addr: *const AtomicI32, count: libc::c_int) -> libc::c_int;
fn emscripten_futex_wake(addr: *const AtomicU32, count: libc::c_int) -> libc::c_int;
}

unsafe { emscripten_futex_wake(futex as *const AtomicI32, 1) > 0 }
unsafe { emscripten_futex_wake(futex, 1) > 0 }
}
12 changes: 6 additions & 6 deletions library/std/src/sys/unix/locks/futex.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::cell::UnsafeCell;
use crate::sync::atomic::{
AtomicI32, AtomicUsize,
AtomicU32, AtomicUsize,
Ordering::{Acquire, Relaxed, Release},
};
use crate::sys::futex::{futex_wait, futex_wake, futex_wake_all};
Expand All @@ -13,13 +13,13 @@ pub struct Mutex {
/// 0: unlocked
/// 1: locked, no other threads waiting
/// 2: locked, and other threads waiting (contended)
futex: AtomicI32,
futex: AtomicU32,
}

impl Mutex {
#[inline]
pub const fn new() -> Self {
Self { futex: AtomicI32::new(0) }
Self { futex: AtomicU32::new(0) }
}

#[inline]
Expand Down Expand Up @@ -71,7 +71,7 @@ impl Mutex {
}
}

fn spin(&self) -> i32 {
fn spin(&self) -> u32 {
let mut spin = 100;
loop {
// We only use `load` (and not `swap` or `compare_exchange`)
Expand Down Expand Up @@ -110,13 +110,13 @@ pub struct Condvar {
// The value of this atomic is simply incremented on every notification.
// This is used by `.wait()` to not miss any notifications after
// unlocking the mutex and before waiting for notifications.
futex: AtomicI32,
futex: AtomicU32,
}

impl Condvar {
#[inline]
pub const fn new() -> Self {
Self { futex: AtomicI32::new(0) }
Self { futex: AtomicU32::new(0) }
}

#[inline]
Expand Down
40 changes: 20 additions & 20 deletions library/std/src/sys/unix/locks/futex_rwlock.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::sync::atomic::{
AtomicI32,
AtomicU32,
Ordering::{Acquire, Relaxed, Release},
};
use crate::sys::futex::{futex_wait, futex_wake, futex_wake_all};
Expand All @@ -14,36 +14,36 @@ pub struct RwLock {
// 0x3FFF_FFFF: Write locked
// Bit 30: Readers are waiting on this futex.
// Bit 31: Writers are waiting on the writer_notify futex.
state: AtomicI32,
state: AtomicU32,
// The 'condition variable' to notify writers through.
// Incremented on every signal.
writer_notify: AtomicI32,
writer_notify: AtomicU32,
}

const READ_LOCKED: i32 = 1;
const MASK: i32 = (1 << 30) - 1;
const WRITE_LOCKED: i32 = MASK;
const MAX_READERS: i32 = MASK - 1;
const READERS_WAITING: i32 = 1 << 30;
const WRITERS_WAITING: i32 = 1 << 31;
const READ_LOCKED: u32 = 1;
const MASK: u32 = (1 << 30) - 1;
const WRITE_LOCKED: u32 = MASK;
const MAX_READERS: u32 = MASK - 1;
const READERS_WAITING: u32 = 1 << 30;
const WRITERS_WAITING: u32 = 1 << 31;

fn is_unlocked(state: i32) -> bool {
fn is_unlocked(state: u32) -> bool {
state & MASK == 0
}

fn is_write_locked(state: i32) -> bool {
fn is_write_locked(state: u32) -> bool {
state & MASK == WRITE_LOCKED
}

fn has_readers_waiting(state: i32) -> bool {
fn has_readers_waiting(state: u32) -> bool {
state & READERS_WAITING != 0
}

fn has_writers_waiting(state: i32) -> bool {
fn has_writers_waiting(state: u32) -> bool {
state & WRITERS_WAITING != 0
}

fn is_read_lockable(state: i32) -> bool {
fn is_read_lockable(state: u32) -> bool {
// This also returns false if the counter could overflow if we tried to read lock it.
//
// We don't allow read-locking if there's readers waiting, even if the lock is unlocked
Expand All @@ -53,14 +53,14 @@ fn is_read_lockable(state: i32) -> bool {
state & MASK < MAX_READERS && !has_readers_waiting(state) && !has_writers_waiting(state)
}

fn has_reached_max_readers(state: i32) -> bool {
fn has_reached_max_readers(state: u32) -> bool {
state & MASK == MAX_READERS
}

impl RwLock {
#[inline]
pub const fn new() -> Self {
Self { state: AtomicI32::new(0), writer_notify: AtomicI32::new(0) }
Self { state: AtomicU32::new(0), writer_notify: AtomicU32::new(0) }
}

#[inline]
Expand Down Expand Up @@ -227,7 +227,7 @@ impl RwLock {
/// If both are waiting, this will wake up only one writer, but will fall
/// back to waking up readers if there was no writer to wake up.
#[cold]
fn wake_writer_or_readers(&self, mut state: i32) {
fn wake_writer_or_readers(&self, mut state: u32) {
assert!(is_unlocked(state));

// The readers waiting bit might be turned on at any point now,
Expand Down Expand Up @@ -287,7 +287,7 @@ impl RwLock {
}

/// Spin for a while, but stop directly at the given condition.
fn spin_until(&self, f: impl Fn(i32) -> bool) -> i32 {
fn spin_until(&self, f: impl Fn(u32) -> bool) -> u32 {
let mut spin = 100; // Chosen by fair dice roll.
loop {
let state = self.state.load(Relaxed);
Expand All @@ -299,12 +299,12 @@ impl RwLock {
}
}

fn spin_write(&self) -> i32 {
fn spin_write(&self) -> u32 {
// Stop spinning when it's unlocked or when there's waiting writers, to keep things somewhat fair.
self.spin_until(|state| is_unlocked(state) || has_writers_waiting(state))
}

fn spin_read(&self) -> i32 {
fn spin_read(&self) -> u32 {
// Stop spinning when it's unlocked or read locked, or when there's waiting threads.
self.spin_until(|state| {
!is_write_locked(state) || has_readers_waiting(state) || has_writers_waiting(state)
Expand Down
14 changes: 9 additions & 5 deletions library/std/src/sys/wasm/atomics/futex.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
use crate::arch::wasm32;
use crate::convert::TryInto;
use crate::sync::atomic::AtomicI32;
use crate::sync::atomic::AtomicU32;
use crate::time::Duration;

pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option<Duration>) {
pub fn futex_wait(futex: &AtomicU32, expected: u32, timeout: Option<Duration>) {
let timeout = timeout.and_then(|t| t.as_nanos().try_into().ok()).unwrap_or(-1);
unsafe {
wasm32::memory_atomic_wait32(futex as *const AtomicI32 as *mut i32, expected, timeout);
wasm32::memory_atomic_wait32(
futex as *const AtomicU32 as *mut i32,
expected as i32,
timeout,
);
}
}

pub fn futex_wake(futex: &AtomicI32) {
pub fn futex_wake(futex: &AtomicU32) {
unsafe {
wasm32::memory_atomic_notify(futex as *const AtomicI32 as *mut i32, 1);
wasm32::memory_atomic_notify(futex as *const AtomicU32 as *mut i32, 1);
}
}
12 changes: 6 additions & 6 deletions library/std/src/sys_common/thread_parker/futex.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
use crate::sync::atomic::AtomicI32;
use crate::sync::atomic::AtomicU32;
use crate::sync::atomic::Ordering::{Acquire, Release};
use crate::sys::futex::{futex_wait, futex_wake};
use crate::time::Duration;

const PARKED: i32 = -1;
const EMPTY: i32 = 0;
const NOTIFIED: i32 = 1;
const PARKED: u32 = u32::MAX;
const EMPTY: u32 = 0;
const NOTIFIED: u32 = 1;

pub struct Parker {
state: AtomicI32,
state: AtomicU32,
}

// Notes about memory ordering:
Expand All @@ -34,7 +34,7 @@ pub struct Parker {
impl Parker {
#[inline]
pub const fn new() -> Self {
Parker { state: AtomicI32::new(EMPTY) }
Parker { state: AtomicU32::new(EMPTY) }
}

// Assumes this is only called by the thread that owns the Parker,
Expand Down

0 comments on commit 5c4d9f8

Please sign in to comment.