From ebb48a17ca5c64ae7df3b508b460938136caae79 Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Thu, 18 Jul 2024 00:03:40 +0000 Subject: [PATCH] Address review This changes the way that Tids are accessed on 32-bit platforms from a seqlock to a simple tls-address check (followed by a ThreadId comparison). Additionally, `try_current_id` is changed to be infallible and is renamed to `current_id`. --- library/std/src/sync/reentrant_lock.rs | 102 +++++++++++-------------- library/std/src/thread/mod.rs | 22 ++---- 2 files changed, 52 insertions(+), 72 deletions(-) diff --git a/library/std/src/sync/reentrant_lock.rs b/library/std/src/sync/reentrant_lock.rs index 8875d95bf3a9a..ab690965318c0 100644 --- a/library/std/src/sync/reentrant_lock.rs +++ b/library/std/src/sync/reentrant_lock.rs @@ -8,7 +8,7 @@ use crate::fmt; use crate::ops::Deref; use crate::panic::{RefUnwindSafe, UnwindSafe}; use crate::sys::sync as sys; -use crate::thread::ThreadId; +use crate::thread::{current_id, ThreadId}; /// A re-entrant mutual exclusion lock /// @@ -108,66 +108,66 @@ cfg_if!( self.0.store(value, Relaxed); } } - } else if #[cfg(target_has_atomic = "32")] { - use crate::sync::atomic::{AtomicU32, Ordering::{Acquire, Relaxed, Release}}; + } else { + /// Returns the address of a TLS variable. This is guaranteed to + /// be unique across all currently alive threads. + fn tls_addr() -> usize { + thread_local! { static X: u8 = const { 0u8 } }; + + X.with(|p| <*const u8>::addr(p)) + } + + use crate::sync::atomic::{ + AtomicUsize, + Ordering, + }; struct Tid { - seq: AtomicU32, - low: AtomicU32, - high: AtomicU32, + // When a thread calls `set()`, this value gets updated to + // the address of a thread local on that thread. This is + // used as a first check in `contains()`; if the `tls_addr` + // doesn't match the TLS address of the current thread, then + // the ThreadId also can't match. Only if the TLS addresses do + // match do we read out the actual TID. + // Note also that we can use relaxed atomic operations here, because + // we only ever read from the tid if `tls_addr` matches the current + // TLS address. In that case, either the the tid has been set by + // the current thread, or by a thread that has terminated before + // the current thread was created. In either case, no further + // synchronization is needed (as per ) + tls_addr: AtomicUsize, + tid: UnsafeCell, } + unsafe impl Send for Tid {} + unsafe impl Sync for Tid {} + impl Tid { const fn new() -> Self { - Self { - seq: AtomicU32::new(0), - low: AtomicU32::new(0), - high: AtomicU32::new(0), - } + Self { tls_addr: AtomicUsize::new(0), tid: UnsafeCell::new(0) } } #[inline] // NOTE: This assumes that `owner` is the ID of the current // thread, and may spuriously return `false` if that's not the case. fn contains(&self, owner: ThreadId) -> bool { - // Synchronizes with the release-increment in `set()` to ensure - // we only read the data after it's been fully written. - let mut seq = self.seq.load(Acquire); - loop { - if seq % 2 == 0 { - let low = self.low.load(Relaxed); - let high = self.high.load(Relaxed); - // The acquire-increment in `set()` synchronizes with this release - // store to ensure that `get()` doesn't see data from a subsequent - // `set()` call. - match self.seq.compare_exchange_weak(seq, seq, Release, Acquire) { - Ok(_) => { - let tid = u64::from(low) | (u64::from(high) << 32); - return owner.as_u64().get() == tid; - }, - Err(new) => seq = new, - } - } else { - // Another thread is currently writing to the seqlock. That thread - // must also be holding the mutex, so we can't currently be the lock owner. - return false; - } - } + // SAFETY: See the comments in the struct definition. + self.tls_addr.load(Ordering::Relaxed) == tls_addr() + && unsafe { *self.tid.get() } == owner.as_u64().get() } #[inline] - // This may only be called from one thread at a time, otherwise - // concurrent `get()` calls may return teared data. + // This may only be called by one thread at a time. fn set(&self, tid: Option) { + // It's important that we set `self.tls_addr` to 0 if the + // tid is cleared. Otherwise, there might be race conditions between + // `set()` and `get()`. + let tls_addr = if tid.is_some() { tls_addr() } else { 0 }; let value = tid.map_or(0, |tid| tid.as_u64().get()); - self.seq.fetch_add(1, Acquire); - self.low.store(value as u32, Relaxed); - self.high.store((value >> 32) as u32, Relaxed); - self.seq.fetch_add(1, Release); + self.tls_addr.store(tls_addr, Ordering::Relaxed); + unsafe { *self.tid.get() = value }; } } - } else { - compile_error!("`ReentrantLock` requires at least 32 bit atomics!"); } ); @@ -272,7 +272,7 @@ impl ReentrantLock { /// assert_eq!(lock.lock().get(), 10); /// ``` pub fn lock(&self) -> ReentrantLockGuard<'_, T> { - let this_thread = current_thread_id(); + let this_thread = current_id(); // Safety: We only touch lock_count when we own the lock. unsafe { if self.owner.contains(this_thread) { @@ -314,7 +314,7 @@ impl ReentrantLock { /// /// This function does not block. pub(crate) fn try_lock(&self) -> Option> { - let this_thread = current_thread_id(); + let this_thread = current_id(); // Safety: We only touch lock_count when we own the lock. unsafe { if self.owner.contains(this_thread) { @@ -400,17 +400,3 @@ impl Drop for ReentrantLockGuard<'_, T> { } } } - -/// Returns the current thread's ThreadId value, which is guaranteed -/// to be unique across the lifetime of the process. -/// -/// Panics if called during a TLS destructor on a thread that hasn't -/// been assigned an ID. -pub(crate) fn current_thread_id() -> ThreadId { - #[cold] - fn no_tid() -> ! { - rtabort!("Thread hasn't been assigned an ID!") - } - - crate::thread::try_current_id().unwrap_or_else(|| no_tid()) -} diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index bf68a6289ff44..3d3ba1acf29ce 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -698,9 +698,7 @@ where } thread_local! { - // Invariant: `CURRENT` and `CURRENT_ID` will always be initialized - // together. However, while `CURRENT_ID` will be available during - // TLS constructors, `CURRENT` will not. + // Invariant: `CURRENT` and `CURRENT_ID` will always be initialized together. static CURRENT: OnceCell = const { OnceCell::new() }; static CURRENT_ID: Cell> = const { Cell::new(None) }; } @@ -737,18 +735,14 @@ pub(crate) fn try_current() -> Option { } /// Gets the id of the thread that invokes it. -/// -/// If called from inside a TLS destructor and the thread was never -/// assigned an id, returns `None`. #[inline] -pub(crate) fn try_current_id() -> Option { - if CURRENT_ID.get().is_none() { - // If `CURRENT_ID` isn't initialized yet, then `CURRENT` must also not be initialized. - // `try_current()` will try to initialize both `CURRENT` and `CURRENT_ID`. - // Subsequent calls to `try_current_id` will then no longer enter this if-branch. - let _ = try_current(); - } - CURRENT_ID.get() +pub(crate) fn current_id() -> ThreadId { + CURRENT_ID.get().unwrap_or_else(|| { + // If `CURRENT_ID` isn't initialized yet, then `CURRENT` must also not be initialized. + // `current()` will initialize both `CURRENT` and `CURRENT_ID` so subsequent calls to + // `current_id()` will succeed immediately. + current().id() + }) } /// Gets a handle to the thread that invokes it.