From 6813c1c69baf870a479c70e23ad0550d1d9aa9be Mon Sep 17 00:00:00 2001 From: Stefan Lankes Date: Wed, 3 Jun 2020 20:40:22 +0200 Subject: [PATCH 1/5] revise RwLock, which is derived from the wasm implementation - increasing the readability of `Condvar` - simplify the interface to the libos HermitCore --- src/libstd/sys/hermit/condvar.rs | 66 ++++++++++-------- src/libstd/sys/hermit/rwlock.rs | 115 ++++++++++++++++++++++++++++--- 2 files changed, 143 insertions(+), 38 deletions(-) diff --git a/src/libstd/sys/hermit/condvar.rs b/src/libstd/sys/hermit/condvar.rs index 132e579b3a5..cca8db2714d 100644 --- a/src/libstd/sys/hermit/condvar.rs +++ b/src/libstd/sys/hermit/condvar.rs @@ -1,60 +1,70 @@ use crate::cmp; +use crate::ffi::c_void; +use crate::mem; +use crate::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use crate::sys::hermit::abi; use crate::sys::mutex::Mutex; +use crate::ptr; use crate::time::Duration; +// The implementation is inspired by Andrew D. Birrell's paper +// "Implementing Condition Variables with Semaphores" + pub struct Condvar { - identifier: usize, + counter: AtomicUsize, + sem1: *const c_void, + sem2: *const c_void, } +unsafe impl Send for Condvar {} +unsafe impl Sync for Condvar {} + impl Condvar { pub const fn new() -> Condvar { - Condvar { identifier: 0 } + Condvar { + counter: AtomicUsize::new(0), + sem1: ptr::null(), + sem2: ptr::null(), + } } pub unsafe fn init(&mut self) { - let _ = abi::init_queue(self.id()); + let _ = abi::sem_init(&mut self.sem1 as *mut *const c_void, 0); + let _ = abi::sem_init(&mut self.sem2 as *mut *const c_void, 0); } pub unsafe fn notify_one(&self) { - let _ = abi::notify(self.id(), 1); + if self.counter.load(SeqCst) > 0 { + self.counter.fetch_sub(1, SeqCst); + abi::sem_post(self.sem1); + abi::sem_timedwait(self.sem2, 0); + } } - #[inline] pub unsafe fn notify_all(&self) { - let _ = abi::notify(self.id(), -1 /* =all */); + let counter = self.counter.swap(0, SeqCst); + for _ in 0..counter { + abi::sem_post(self.sem1); + } + for _ in 0..counter { + abi::sem_timedwait(self.sem2, 0); + } } pub unsafe fn wait(&self, mutex: &Mutex) { - // add current task to the wait queue - let _ = abi::add_queue(self.id(), -1 /* no timeout */); + self.counter.fetch_add(1, SeqCst); mutex.unlock(); - let _ = abi::wait(self.id()); + abi::sem_timedwait(self.sem1, 0); + abi::sem_post(self.sem2); mutex.lock(); } pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool { - let nanos = dur.as_nanos(); - let nanos = cmp::min(i64::MAX as u128, nanos); - - // add current task to the wait queue - let _ = abi::add_queue(self.id(), nanos as i64); - - mutex.unlock(); - // If the return value is !0 then a timeout happened, so we return - // `false` as we weren't actually notified. - let ret = abi::wait(self.id()) == 0; - mutex.lock(); - - ret + panic!("wait_timeout not supported on hermit"); } pub unsafe fn destroy(&self) { - let _ = abi::destroy_queue(self.id()); - } - - #[inline] - fn id(&self) -> usize { - &self.identifier as *const usize as usize + let _ = abi::sem_destroy(self.sem1); + let _ = abi::sem_destroy(self.sem2); } } diff --git a/src/libstd/sys/hermit/rwlock.rs b/src/libstd/sys/hermit/rwlock.rs index c19799af3c7..06442e925f4 100644 --- a/src/libstd/sys/hermit/rwlock.rs +++ b/src/libstd/sys/hermit/rwlock.rs @@ -1,49 +1,144 @@ -use super::mutex::Mutex; +use crate::cell::UnsafeCell; +use crate::sys::condvar::Condvar; +use crate::sys::mutex::Mutex; pub struct RWLock { - mutex: Mutex, + lock: Mutex, + cond: Condvar, + state: UnsafeCell, +} + +enum State { + Unlocked, + Reading(usize), + Writing, } unsafe impl Send for RWLock {} unsafe impl Sync for RWLock {} +// This rwlock implementation is a relatively simple implementation which has a +// condition variable for readers/writers as well as a mutex protecting the +// internal state of the lock. A current downside of the implementation is that +// unlocking the lock will notify *all* waiters rather than just readers or just +// writers. This can cause lots of "thundering stampede" problems. While +// hopefully correct this implementation is very likely to want to be changed in +// the future. + impl RWLock { pub const fn new() -> RWLock { - RWLock { mutex: Mutex::new() } + RWLock { lock: Mutex::new(), cond: Condvar::new(), state: UnsafeCell::new(State::Unlocked) } } #[inline] pub unsafe fn read(&self) { - self.mutex.lock(); + self.lock.lock(); + while !(*self.state.get()).inc_readers() { + self.cond.wait(&self.lock); + } + self.lock.unlock(); } #[inline] pub unsafe fn try_read(&self) -> bool { - self.mutex.try_lock() + self.lock.lock(); + let ok = (*self.state.get()).inc_readers(); + self.lock.unlock(); + return ok; } #[inline] pub unsafe fn write(&self) { - self.mutex.lock(); + self.lock.lock(); + while !(*self.state.get()).inc_writers() { + self.cond.wait(&self.lock); + } + self.lock.unlock(); } #[inline] pub unsafe fn try_write(&self) -> bool { - self.mutex.try_lock() + self.lock.lock(); + let ok = (*self.state.get()).inc_writers(); + self.lock.unlock(); + return ok; } #[inline] pub unsafe fn read_unlock(&self) { - self.mutex.unlock(); + self.lock.lock(); + let notify = (*self.state.get()).dec_readers(); + self.lock.unlock(); + if notify { + // FIXME: should only wake up one of these some of the time + self.cond.notify_all(); + } } #[inline] pub unsafe fn write_unlock(&self) { - self.mutex.unlock(); + self.lock.lock(); + (*self.state.get()).dec_writers(); + self.lock.unlock(); + // FIXME: should only wake up one of these some of the time + self.cond.notify_all(); } #[inline] pub unsafe fn destroy(&self) { - self.mutex.destroy(); + self.lock.destroy(); + self.cond.destroy(); } } + +impl State { + fn inc_readers(&mut self) -> bool { + match *self { + State::Unlocked => { + *self = State::Reading(1); + true + } + State::Reading(ref mut cnt) => { + *cnt += 1; + true + } + State::Writing => false, + } + } + + fn inc_writers(&mut self) -> bool { + match *self { + State::Unlocked => { + *self = State::Writing; + true + } + State::Reading(_) | State::Writing => false, + } + } + + fn dec_readers(&mut self) -> bool { + let zero = match *self { + State::Reading(ref mut cnt) => { + *cnt -= 1; + *cnt == 0 + } + State::Unlocked | State::Writing => invalid(), + }; + if zero { + *self = State::Unlocked; + } + zero + } + + fn dec_writers(&mut self) { + match *self { + State::Writing => {} + State::Unlocked | State::Reading(_) => invalid(), + } + *self = State::Unlocked; + } +} + +fn invalid() -> ! { + panic!("inconsistent rwlock"); +} From 3acc3ef10d2099a4b3118e8c705b36f4bbaf6f64 Mon Sep 17 00:00:00 2001 From: Stefan Lankes Date: Thu, 4 Jun 2020 00:02:57 +0200 Subject: [PATCH 2/5] minor changes to pass the format check --- src/libstd/sys/hermit/condvar.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/libstd/sys/hermit/condvar.rs b/src/libstd/sys/hermit/condvar.rs index cca8db2714d..b318890a10e 100644 --- a/src/libstd/sys/hermit/condvar.rs +++ b/src/libstd/sys/hermit/condvar.rs @@ -1,10 +1,10 @@ +use crate::ptr; use crate::cmp; use crate::ffi::c_void; use crate::mem; use crate::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use crate::sys::hermit::abi; use crate::sys::mutex::Mutex; -use crate::ptr; use crate::time::Duration; // The implementation is inspired by Andrew D. Birrell's paper @@ -21,11 +21,7 @@ unsafe impl Sync for Condvar {} impl Condvar { pub const fn new() -> Condvar { - Condvar { - counter: AtomicUsize::new(0), - sem1: ptr::null(), - sem2: ptr::null(), - } + Condvar { counter: AtomicUsize::new(0), sem1: ptr::null(), sem2: ptr::null() } } pub unsafe fn init(&mut self) { From beb1b1fa5b1047c7caf8a1d499725df3c1ad8cad Mon Sep 17 00:00:00 2001 From: Stefan Lankes Date: Thu, 4 Jun 2020 06:47:38 +0200 Subject: [PATCH 3/5] reorder crates to pass the format check --- src/libstd/sys/hermit/condvar.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/sys/hermit/condvar.rs b/src/libstd/sys/hermit/condvar.rs index b318890a10e..e41d4eb4fa7 100644 --- a/src/libstd/sys/hermit/condvar.rs +++ b/src/libstd/sys/hermit/condvar.rs @@ -1,7 +1,7 @@ -use crate::ptr; use crate::cmp; use crate::ffi::c_void; use crate::mem; +use crate::ptr; use crate::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use crate::sys::hermit::abi; use crate::sys::mutex::Mutex; From f9c609164251abc136eb9bda55c92cb99adb5c86 Mon Sep 17 00:00:00 2001 From: Stefan Lankes Date: Fri, 26 Jun 2020 19:51:55 +0200 Subject: [PATCH 4/5] remove some compiler warnings --- src/libstd/sys/hermit/condvar.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libstd/sys/hermit/condvar.rs b/src/libstd/sys/hermit/condvar.rs index e41d4eb4fa7..52c8c3b17e8 100644 --- a/src/libstd/sys/hermit/condvar.rs +++ b/src/libstd/sys/hermit/condvar.rs @@ -1,6 +1,4 @@ -use crate::cmp; use crate::ffi::c_void; -use crate::mem; use crate::ptr; use crate::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use crate::sys::hermit::abi; @@ -55,7 +53,7 @@ pub unsafe fn wait(&self, mutex: &Mutex) { mutex.lock(); } - pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool { + pub unsafe fn wait_timeout(&self, _mutex: &Mutex, _dur: Duration) -> bool { panic!("wait_timeout not supported on hermit"); } From 6925ebdd6051b3827c2978b8b285e49743306ccc Mon Sep 17 00:00:00 2001 From: Stefan Lankes Date: Mon, 6 Jul 2020 20:43:51 +0200 Subject: [PATCH 5/5] use latest version of hermit-abi --- Cargo.lock | 4 ++-- src/libstd/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b54566e7176..9838b45a983 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1402,9 +1402,9 @@ dependencies = [ [[package]] name = "hermit-abi" -version = "0.1.14" +version = "0.1.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b9586eedd4ce6b3c498bc3b4dd92fc9f11166aa908a914071953768066c67909" +checksum = "3deed196b6e7f9e44a2ae8d94225d80302d81208b1bb673fd21fe634645c85a9" dependencies = [ "compiler_builtins", "libc", diff --git a/src/libstd/Cargo.toml b/src/libstd/Cargo.toml index 490afb5a043..4f3052d0371 100644 --- a/src/libstd/Cargo.toml +++ b/src/libstd/Cargo.toml @@ -41,7 +41,7 @@ dlmalloc = { version = "0.1", features = ['rustc-dep-of-std'] } fortanix-sgx-abi = { version = "0.3.2", features = ['rustc-dep-of-std'] } [target.'cfg(all(any(target_arch = "x86_64", target_arch = "aarch64"), target_os = "hermit"))'.dependencies] -hermit-abi = { version = "0.1.14", features = ['rustc-dep-of-std'] } +hermit-abi = { version = "0.1.15", features = ['rustc-dep-of-std'] } [target.wasm32-wasi.dependencies] wasi = { version = "0.9.0", features = ['rustc-dep-of-std'], default-features = false }