Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the parking_lot locking primitives #56410

Closed
wants to merge 40 commits into from
Closed
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
7b0ad27
Add parking_lot as submodule
faern Dec 14, 2018
1aab11d
Exclude parking_lot benchmark crate from root workspace
faern Mar 19, 2019
e313500
Add needed winapi bindings
faern Nov 26, 2018
20ab8d2
Use parking_lot::RawRwLock in panicking module
faern Feb 14, 2019
7cafa6d
Use parking_lot::RawRwLock to implement std::sync::RwLock
faern Feb 14, 2019
7c42fc8
Remove unused sys/sys_common::rwlock
faern Feb 20, 2019
3b23add
Use parking_lot::ReentrantMutex in stdio
faern Feb 19, 2019
f58fc9b
Use parking_lot::Mutex for stdin
faern Feb 20, 2019
81a1e21
Remove unused sys/sys_common ReentrantMutex
faern Feb 20, 2019
5a3dd8a
Base Mutex/Condvar on parking_lot primitives
faern Feb 20, 2019
d354b95
Remove unused sys/sys_common::condvar
faern Feb 20, 2019
8edc128
Remove Mutex init, try_lock, raw_unlock and destroy
faern Feb 27, 2019
4f0f8e1
Remove sys/sys_common::mutex::raw functions
faern Feb 27, 2019
a0b366c
Add sync::RawMutex
faern Mar 3, 2019
8dfad4b
Move io::lazy over to RawMutex
faern Mar 3, 2019
d6fab63
Move std::time over to RawMutex
faern Mar 3, 2019
fc379ee
Move at_exit over to RawMutex
faern Mar 3, 2019
142bda2
Move thread_local over to RawMutex
faern Mar 3, 2019
eaaca60
Move thread over to RawMutex
faern Mar 3, 2019
0d679ef
Move unix::args over to RawMutex
faern Mar 3, 2019
e8e355e
Move unix::os over to RawMutex
faern Mar 3, 2019
55f1ed2
Fix unix fork locking to work with parking_lot
faern Mar 3, 2019
c6c6fb1
Use RawMutex in backtrace
faern Feb 19, 2019
f1a805b
Remove unused sys/sys_common::mutex
faern Mar 3, 2019
0b1a6e9
Lock locks outside unsafe blocks
faern Feb 19, 2019
a562580
Use sys_common::mutex in sys::windows::process
faern Feb 21, 2019
b50a20e
Remove unused sys::c items from Windows
faern Feb 21, 2019
6ad5485
Remove unused sgx waitqueue types
faern Apr 1, 2019
0070390
Convert thread parking to use parking_lot primitives
faern Mar 2, 2019
c294204
Move redox::args over to RawMutex
faern Mar 4, 2019
c301ce0
Make redox::os use RawMutex
faern Mar 4, 2019
dd30a55
Remove wasi locking modules
faern Mar 31, 2019
e0fd309
Make cloudabi::abi module public inside libstd
faern Apr 1, 2019
0048440
Update SGX libunwind FFI interface to use new RwLock
faern Apr 1, 2019
e49bbd1
Make Lazy::get not unsafe any more
faern Apr 17, 2019
f40d609
Base Once on parking_lot::Once
faern Apr 18, 2019
56c2805
Add better documentation for RawMutex
faern Apr 20, 2019
7124cae
Improve Lazy documentation
faern Apr 20, 2019
6695be8
Base park/unpark directly on parking_lot_core
faern May 5, 2019
795285f
Fix park/unpark after feedback
faern May 12, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,7 @@
[submodule "src/doc/embedded-book"]
path = src/doc/embedded-book
url = https://github.com/rust-embedded/book.git
[submodule "src/parking_lot"]
path = src/parking_lot
url = https://github.com/faern/parking_lot
faern marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

@faern faern Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where should this be pointing before merging? Back to @Amanieu's repo, or will that be moved to the @rust-lang organization, like what I think is the plan for hashbrown?


1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ exclude = [
"build",
# HACK(eddyb) This hardcodes the fact that our CI uses `/checkout/obj`.
"obj",
"src/parking_lot/benchmark",
]

# Curiously, LLVM 7.0 will segfault if compiled with opt-level=3
Expand Down
2 changes: 1 addition & 1 deletion src/ci/docker/dist-various-2/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ RUN /tmp/build-solaris-toolchain.sh sparcv9 sparcv9 solaris-sparc
COPY dist-various-2/build-x86_64-fortanix-unknown-sgx-toolchain.sh /tmp/
# We pass the commit id of the port of LLVM's libunwind to the build script.
# Any update to the commit id here, should cause the container image to be re-built from this point on.
RUN /tmp/build-x86_64-fortanix-unknown-sgx-toolchain.sh "53b586346f2c7870e20b170decdc30729d97c42b"
RUN /tmp/build-x86_64-fortanix-unknown-sgx-toolchain.sh "a50a70f1394b2e62d6a5d2510330eb110e31dad4"

COPY dist-various-2/build-wasi-toolchain.sh /tmp/
RUN /tmp/build-wasi-toolchain.sh
Expand Down
5 changes: 4 additions & 1 deletion src/libstd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ fortanix-sgx-abi = { version = "0.3.2", features = ['rustc-dep-of-std'] }
cc = "1.0"

[features]
default = ["compiler_builtins_c", "std_detect_file_io", "std_detect_dlsym_getauxval"]
default = ["compiler_builtins_c", "std_detect_file_io", "std_detect_dlsym_getauxval", "i-am-libstd"]

backtrace = ["backtrace-sys"]
panic-unwind = ["panic_unwind"]
Expand All @@ -75,6 +75,9 @@ wasm-bindgen-threads = []
std_detect_file_io = []
std_detect_dlsym_getauxval = []

# Feature used by parking_lot
i-am-libstd = []

[package.metadata.fortanix-sgx]
# Maximum possible number of threads when testing
threads = 125
21 changes: 9 additions & 12 deletions src/libstd/io/lazy.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
use crate::cell::Cell;
use crate::ptr;
use crate::sync::Arc;
use crate::sync::{Arc, RawMutex};
use crate::sys_common;
use crate::sys_common::mutex::Mutex;

pub struct Lazy<T> {
// We never call `lock.init()`, so it is UB to attempt to acquire this mutex reentrantly!
lock: Mutex,
lock: RawMutex,
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
ptr: Cell<*mut Arc<T>>,
}

Expand All @@ -18,24 +16,24 @@ unsafe impl<T> Sync for Lazy<T> {}
impl<T> Lazy<T> {
pub const fn new() -> Lazy<T> {
Lazy {
lock: Mutex::new(),
lock: RawMutex::new(),
ptr: Cell::new(ptr::null_mut()),
}
}
}

impl<T: Send + Sync + 'static> Lazy<T> {
/// Safety: `init` must not call `get` on the variable that is being
/// initialized.
pub unsafe fn get(&'static self, init: fn() -> Arc<T>) -> Option<Arc<T>> {
/// Warning: `init` must not call `get` on the variable that is being
/// initialized. Doing so will cause a deadlock.
pub fn get(&'static self, init: fn() -> Arc<T>) -> Option<Arc<T>> {
let _guard = self.lock.lock();
let ptr = self.ptr.get();
if ptr.is_null() {
Some(self.init(init))
Some(unsafe { self.init(init) })
} else if ptr == done() {
None
} else {
Some((*ptr).clone())
Some(unsafe { (*ptr).clone() })
}
}

Expand All @@ -53,8 +51,7 @@ impl<T: Send + Sync + 'static> Lazy<T> {
drop(Box::from_raw(ptr))
});
// This could reentrantly call `init` again, which is a problem
// because our `lock` allows reentrancy!
// That's why `get` is unsafe and requires the caller to ensure no reentrancy happens.
// because our `lock` will then deadlock!
let ret = init();
if registered.is_ok() {
self.ptr.set(Box::into_raw(Box::new(ret.clone())));
Expand Down
46 changes: 31 additions & 15 deletions src/libstd/io/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ use crate::cell::RefCell;
use crate::fmt;
use crate::io::lazy::Lazy;
use crate::io::{self, Initializer, BufReader, LineWriter, IoSlice, IoSliceMut};
use crate::sync::{Arc, Mutex, MutexGuard};
use crate::sync::Arc;
use crate::sys::stdio;
use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
use crate::panic::{UnwindSafe, RefUnwindSafe};
use crate::parking_lot::{Mutex, MutexGuard, ReentrantMutex, ReentrantMutexGuard};
use crate::thread::LocalKey;

thread_local! {
Expand Down Expand Up @@ -242,9 +243,7 @@ pub struct StdinLock<'a> {
pub fn stdin() -> Stdin {
static INSTANCE: Lazy<Mutex<BufReader<Maybe<StdinRaw>>>> = Lazy::new();
return Stdin {
inner: unsafe {
INSTANCE.get(stdin_init).expect("cannot access stdin during shutdown")
},
inner: INSTANCE.get(stdin_init).expect("cannot access stdin during shutdown"),
};

fn stdin_init() -> Arc<Mutex<BufReader<Maybe<StdinRaw>>>> {
Expand Down Expand Up @@ -285,7 +284,7 @@ impl Stdin {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn lock(&self) -> StdinLock<'_> {
StdinLock { inner: self.inner.lock().unwrap_or_else(|e| e.into_inner()) }
StdinLock { inner: self.inner.lock() }
}

/// Locks this handle and reads a line of input into the specified buffer.
Expand Down Expand Up @@ -466,9 +465,7 @@ pub struct StdoutLock<'a> {
pub fn stdout() -> Stdout {
static INSTANCE: Lazy<ReentrantMutex<RefCell<LineWriter<Maybe<StdoutRaw>>>>> = Lazy::new();
return Stdout {
inner: unsafe {
INSTANCE.get(stdout_init).expect("cannot access stdout during shutdown")
},
inner: INSTANCE.get(stdout_init).expect("cannot access stdout during shutdown"),
};

fn stdout_init() -> Arc<ReentrantMutex<RefCell<LineWriter<Maybe<StdoutRaw>>>>> {
Expand Down Expand Up @@ -504,7 +501,7 @@ impl Stdout {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn lock(&self) -> StdoutLock<'_> {
StdoutLock { inner: self.inner.lock().unwrap_or_else(|e| e.into_inner()) }
StdoutLock { inner: self.inner.lock() }
}
}

Expand Down Expand Up @@ -533,6 +530,12 @@ impl Write for Stdout {
self.lock().write_fmt(args)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl UnwindSafe for Stdout {}
#[stable(feature = "rust1", since = "1.0.0")]
impl RefUnwindSafe for Stdout {}
faern marked this conversation as resolved.
Show resolved Hide resolved

#[stable(feature = "rust1", since = "1.0.0")]
impl Write for StdoutLock<'_> {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
Expand All @@ -553,6 +556,11 @@ impl fmt::Debug for StdoutLock<'_> {
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl UnwindSafe for StdoutLock<'_> {}
#[stable(feature = "rust1", since = "1.0.0")]
impl RefUnwindSafe for StdoutLock<'_> {}

/// A handle to the standard error stream of a process.
///
/// For more information, see the [`io::stderr`] method.
Expand Down Expand Up @@ -625,9 +633,7 @@ pub struct StderrLock<'a> {
pub fn stderr() -> Stderr {
static INSTANCE: Lazy<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> = Lazy::new();
return Stderr {
inner: unsafe {
INSTANCE.get(stderr_init).expect("cannot access stderr during shutdown")
},
inner: INSTANCE.get(stderr_init).expect("cannot access stderr during shutdown"),
};

fn stderr_init() -> Arc<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> {
Expand Down Expand Up @@ -663,7 +669,7 @@ impl Stderr {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn lock(&self) -> StderrLock<'_> {
StderrLock { inner: self.inner.lock().unwrap_or_else(|e| e.into_inner()) }
StderrLock { inner: self.inner.lock() }
}
}

Expand Down Expand Up @@ -692,6 +698,12 @@ impl Write for Stderr {
self.lock().write_fmt(args)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl UnwindSafe for Stderr {}
#[stable(feature = "rust1", since = "1.0.0")]
impl RefUnwindSafe for Stderr {}

#[stable(feature = "rust1", since = "1.0.0")]
impl Write for StderrLock<'_> {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
Expand All @@ -712,6 +724,11 @@ impl fmt::Debug for StderrLock<'_> {
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl UnwindSafe for StderrLock<'_> {}
#[stable(feature = "rust1", since = "1.0.0")]
impl RefUnwindSafe for StderrLock<'_> {}

/// Resets the thread-local stderr handle to the specified writer
///
/// This will replace the current thread's stderr handle, returning the old
Expand Down Expand Up @@ -816,7 +833,6 @@ pub use realstd::io::{_eprint, _print};

#[cfg(test)]
mod tests {
use crate::panic::{UnwindSafe, RefUnwindSafe};
use crate::thread;
use super::*;

Expand Down
12 changes: 12 additions & 0 deletions src/libstd/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,18 @@ pub mod rt;
#[cfg(not(test))]
mod std_detect;

#[path = "../parking_lot/core/src/lib.rs"]
mod parking_lot_core;

#[path = "../parking_lot/lock_api/src/lib.rs"]
#[allow(dead_code)]
mod lock_api;

#[path = "../parking_lot/src/lib.rs"]
#[allow(dead_code)]
mod parking_lot;


#[doc(hidden)]
#[unstable(feature = "stdsimd", issue = "48556")]
#[cfg(not(test))]
Expand Down
17 changes: 9 additions & 8 deletions src/libstd/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ use core::panic::{BoxMeUp, PanicInfo, Location};
use crate::any::Any;
use crate::fmt;
use crate::intrinsics;
use crate::lock_api::RawRwLock as _;
use crate::mem;
use crate::parking_lot::RawRwLock;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So everything else uses std's RawLock but this uses the one from parking_lot... why that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's because it needs the RwLock, not the Mutex, I guess.

Seems like another agument to make the rest also use parking_lot's stuff directly, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they are not equivalent. See my other comment. parking_lot::RawMutex is lower level and would require a lot of manual unlocking where we now get that much nicer with the guard. The standard library had a mutex with a guard prior to this PR. So going for the parking_lot stuff directly for the mutexes would make everything more low level and harder to get fully correct.

Copy link
Contributor Author

@faern faern Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous crate::sys_common::rwlock::RWLock did not have any guards. But the crate::sys_common::mutex::Mutex did. So doing what this PR does currently keeps libstd as similar as possible. Going directly for parking_lot::RawMutex would be a step backwards IMO. We could however write a small wrapper for the RwLock that has guards. But given it's only used in so few places that felt a bit overkill.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this one file here is where we are okay with not having guards?

Copy link
Contributor Author

@faern faern Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose so. It's how the standard library is currently implemented. So someone must have thought it was okay at some point. Well, here and in one place SGX also uses parking_lot::RawRwLock directly in this PR. I would not be against introducing a simple sync::RawRwLock with guards. But it would not be that efficient to use in the SGX place, so it would still only be panicking.rs that would use it 🤷‍♂️

use crate::ptr;
use crate::raw;
use crate::sys::stdio::panic_output;
use crate::sys_common::rwlock::RWLock;
use crate::sys_common::thread_info;
use crate::sys_common::util;
use crate::thread;
Expand Down Expand Up @@ -54,7 +55,7 @@ enum Hook {
Custom(*mut (dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send)),
}

static HOOK_LOCK: RWLock = RWLock::new();
static HOOK_LOCK: RawRwLock = RawRwLock::INIT;
static mut HOOK: Hook = Hook::Default;

/// Registers a custom panic hook, replacing any that was previously registered.
Expand Down Expand Up @@ -97,10 +98,10 @@ pub fn set_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>) {
}

unsafe {
HOOK_LOCK.write();
HOOK_LOCK.lock_exclusive();
let old_hook = HOOK;
HOOK = Hook::Custom(Box::into_raw(hook));
HOOK_LOCK.write_unlock();
HOOK_LOCK.unlock_exclusive();

if let Hook::Custom(ptr) = old_hook {
Box::from_raw(ptr);
Expand Down Expand Up @@ -142,10 +143,10 @@ pub fn take_hook() -> Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send> {
}

unsafe {
HOOK_LOCK.write();
HOOK_LOCK.lock_exclusive();
let hook = HOOK;
HOOK = Hook::Default;
HOOK_LOCK.write_unlock();
HOOK_LOCK.unlock_exclusive();

match hook {
Hook::Default => Box::new(default_hook),
Expand Down Expand Up @@ -463,7 +464,7 @@ fn rust_panic_with_hook(payload: &mut dyn BoxMeUp,
message,
Location::internal_constructor(file, line, col),
);
HOOK_LOCK.read();
HOOK_LOCK.lock_shared();
match HOOK {
// Some platforms know that printing to stderr won't ever actually
// print anything, and if that's the case we can skip the default
Expand All @@ -478,7 +479,7 @@ fn rust_panic_with_hook(payload: &mut dyn BoxMeUp,
(*ptr)(&info);
}
};
HOOK_LOCK.read_unlock();
HOOK_LOCK.unlock_shared();
}

if panics > 1 {
Expand Down
Loading