Merge #112

112: [WIP] New RwLock implementation r=Amanieu a=Amanieu The new implementation is inspired by boost's [`upgrade_mutex`](https://github.com/boostorg/thread/blob/fc08c1fe2840baeeee143440fba31ef9e9a813c8/include/boost/thread/v2/shared_mutex.hpp#L432) rwlock implementation. The main benefit is that it fixes #101: upgradable reads no longer block normal reads. The code has also been refactored and is now much simpler than before. cc @matklad @nikomatsakis Co-authored-by: Amanieu d'Antras <[email protected]>
Amanieu · Jan 31, 2019 · 02b1ed0 · 02b1ed0
2 parents 930d238 + 481fd42
commit 02b1ed0
Show file tree

Hide file tree

Showing 7 changed files with 599 additions and 944 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -15,6 +15,7 @@ lock_api = { path = "lock_api", version = "0.1" }
 
 [dev-dependencies]
 rand = "0.6"
+lazy_static = "1.0"
 
 [features]
 default = ["owning_ref"]

diff --git a/lock_api/Cargo.toml b/lock_api/Cargo.toml
@@ -9,7 +9,7 @@ keywords = ["mutex", "rwlock", "lock", "no_std"]
 categories = ["concurrency", "no-std"]
 
 [dependencies]
-scopeguard = { version = "0.3", default-features = false }
+scopeguard = { version = "1.0", default-features = false }
 owning_ref = { version = "0.4", optional = true }
 
 [features]

diff --git a/src/deadlock.rs b/src/deadlock.rs
@@ -44,10 +44,11 @@ mod tests {
     use std::thread::{self, sleep};
     use std::time::Duration;
     use {Mutex, ReentrantMutex, RwLock};
-    use lock_api::RawMutex;
 
     // We need to serialize these tests since deadlock detection uses global state
-    static DEADLOCK_DETECTION_LOCK: ::RawMutex = RawMutex::INIT;
+    lazy_static! {
+        static ref DEADLOCK_DETECTION_LOCK: Mutex<()> = Mutex::new(());
+    }
 
     fn check_deadlock() -> bool {
         use parking_lot_core::deadlock::check_deadlock;
@@ -56,7 +57,7 @@ mod tests {
 
     #[test]
     fn test_mutex_deadlock() {
-        DEADLOCK_DETECTION_LOCK.lock();
+        let _guard = DEADLOCK_DETECTION_LOCK.lock();
 
         let m1: Arc<Mutex<()>> = Default::default();
         let m2: Arc<Mutex<()>> = Default::default();
@@ -97,13 +98,11 @@ mod tests {
         assert!(check_deadlock());
 
         assert!(!check_deadlock());
-
-        DEADLOCK_DETECTION_LOCK.unlock();
     }
 
     #[test]
     fn test_mutex_deadlock_reentrant() {
-        DEADLOCK_DETECTION_LOCK.lock();
+        let _guard = DEADLOCK_DETECTION_LOCK.lock();
 
         let m1: Arc<Mutex<()>> = Default::default();
 
@@ -118,13 +117,11 @@ mod tests {
         assert!(check_deadlock());
 
         assert!(!check_deadlock());
-
-        DEADLOCK_DETECTION_LOCK.unlock();
     }
 
     #[test]
     fn test_remutex_deadlock() {
-        DEADLOCK_DETECTION_LOCK.lock();
+        let _guard = DEADLOCK_DETECTION_LOCK.lock();
 
         let m1: Arc<ReentrantMutex<()>> = Default::default();
         let m2: Arc<ReentrantMutex<()>> = Default::default();
@@ -168,13 +165,11 @@ mod tests {
         assert!(check_deadlock());
 
         assert!(!check_deadlock());
-
-        DEADLOCK_DETECTION_LOCK.unlock();
     }
 
     #[test]
     fn test_rwlock_deadlock() {
-        DEADLOCK_DETECTION_LOCK.lock();
+        let _guard = DEADLOCK_DETECTION_LOCK.lock();
 
         let m1: Arc<RwLock<()>> = Default::default();
         let m2: Arc<RwLock<()>> = Default::default();
@@ -215,13 +210,12 @@ mod tests {
         assert!(check_deadlock());
 
         assert!(!check_deadlock());
-
-        DEADLOCK_DETECTION_LOCK.unlock();
     }
 
+    #[cfg(rwlock_deadlock_detection_not_supported)]
     #[test]
     fn test_rwlock_deadlock_reentrant() {
-        DEADLOCK_DETECTION_LOCK.lock();
+        let _guard = DEADLOCK_DETECTION_LOCK.lock();
 
         let m1: Arc<RwLock<()>> = Default::default();
 
@@ -236,7 +230,5 @@ mod tests {
         assert!(check_deadlock());
 
         assert!(!check_deadlock());
-
-        DEADLOCK_DETECTION_LOCK.unlock();
     }
 }
diff --git a/src/elision.rs b/src/elision.rs
@@ -12,17 +12,14 @@ pub trait AtomicElisionExt {
     type IntType;
 
     // Perform a compare_exchange and start a transaction
-    fn elision_acquire(
-        &self,
-        current: Self::IntType,
-        new: Self::IntType,
-    ) -> Result<Self::IntType, Self::IntType>;
-    // Perform a compare_exchange and end a transaction
-    fn elision_release(
+    fn elision_compare_exchange_acquire(
         &self,
         current: Self::IntType,
         new: Self::IntType,
     ) -> Result<Self::IntType, Self::IntType>;
+
+    // Perform a fetch_sub and end a transaction
+    fn elision_fetch_sub_release(&self, val: Self::IntType) -> Self::IntType;
 }
 
 // Indicates whether the target architecture supports lock elision
@@ -41,22 +38,23 @@ impl AtomicElisionExt for AtomicUsize {
     type IntType = usize;
 
     #[inline]
-    fn elision_acquire(&self, _: usize, _: usize) -> Result<usize, usize> {
+    fn elision_compare_exchange_acquire(&self, _: usize, _: usize) -> Result<usize, usize> {
         unreachable!();
     }
 
     #[inline]
-    fn elision_release(&self, _: usize, _: usize) -> Result<usize, usize> {
+    fn elision_fetch_sub_release(&self, _: usize) -> usize {
         unreachable!();
     }
 }
 
-#[cfg(all(feature = "nightly", target_arch = "x86"))]
+#[cfg(all(feature = "nightly", any(target_arch = "x86", target_arch = "x86_64")))]
 impl AtomicElisionExt for AtomicUsize {
     type IntType = usize;
 
+    #[cfg(target_pointer_width = "32")]
     #[inline]
-    fn elision_acquire(&self, current: usize, new: usize) -> Result<usize, usize> {
+    fn elision_compare_exchange_acquire(&self, current: usize, new: usize) -> Result<usize, usize> {
         unsafe {
             let prev: usize;
             asm!("xacquire; lock; cmpxchgl $2, $1"
@@ -71,55 +69,12 @@ impl AtomicElisionExt for AtomicUsize {
             }
         }
     }
-
-    #[inline]
-    fn elision_release(&self, current: usize, new: usize) -> Result<usize, usize> {
-        unsafe {
-            let prev: usize;
-            asm!("xrelease; lock; cmpxchgl $2, $1"
-                 : "={eax}" (prev), "+*m" (self)
-                 : "r" (new), "{eax}" (current)
-                 : "memory"
-                 : "volatile");
-            if prev == current {
-                Ok(prev)
-            } else {
-                Err(prev)
-            }
-        }
-    }
-}
-
-#[cfg(all(
-    feature = "nightly",
-    target_arch = "x86_64",
-    target_pointer_width = "32"
-))]
-impl AtomicElisionExt for AtomicUsize {
-    type IntType = usize;
-
-    #[inline]
-    fn elision_acquire(&self, current: usize, new: usize) -> Result<usize, usize> {
-        unsafe {
-            let prev: usize;
-            asm!("xacquire; lock; cmpxchgl $2, $1"
-                 : "={rax}" (prev), "+*m" (self)
-                 : "r" (new), "{rax}" (current)
-                 : "memory"
-                 : "volatile");
-            if prev == current {
-                Ok(prev)
-            } else {
-                Err(prev)
-            }
-        }
-    }
-
+    #[cfg(target_pointer_width = "64")]
     #[inline]
-    fn elision_release(&self, current: usize, new: usize) -> Result<usize, usize> {
+    fn elision_compare_exchange_acquire(&self, current: usize, new: usize) -> Result<usize, usize> {
         unsafe {
             let prev: usize;
-            asm!("xrelease; lock; cmpxchgl $2, $1"
+            asm!("xacquire; lock; cmpxchgq $2, $1"
                  : "={rax}" (prev), "+*m" (self)
                  : "r" (new), "{rax}" (current)
                  : "memory"
@@ -131,47 +86,31 @@ impl AtomicElisionExt for AtomicUsize {
             }
         }
     }
-}
-
-#[cfg(all(
-    feature = "nightly",
-    target_arch = "x86_64",
-    target_pointer_width = "64"
-))]
-impl AtomicElisionExt for AtomicUsize {
-    type IntType = usize;
 
+    #[cfg(target_pointer_width = "32")]
     #[inline]
-    fn elision_acquire(&self, current: usize, new: usize) -> Result<usize, usize> {
+    fn elision_fetch_sub_release(&self, val: usize) -> usize {
         unsafe {
             let prev: usize;
-            asm!("xacquire; lock; cmpxchgq $2, $1"
-                 : "={rax}" (prev), "+*m" (self)
-                 : "r" (new), "{rax}" (current)
+            asm!("xrelease; lock; xaddl $2, $1"
+                 : "=r" (prev), "+*m" (self)
+                 : "0" (val.wrapping_neg())
                  : "memory"
                  : "volatile");
-            if prev == current {
-                Ok(prev)
-            } else {
-                Err(prev)
-            }
+            prev
         }
     }
-
+    #[cfg(target_pointer_width = "64")]
     #[inline]
-    fn elision_release(&self, current: usize, new: usize) -> Result<usize, usize> {
+    fn elision_fetch_sub_release(&self, val: usize) -> usize {
         unsafe {
             let prev: usize;
-            asm!("xrelease; lock; cmpxchgq $2, $1"
-                 : "={rax}" (prev), "+*m" (self)
-                 : "r" (new), "{rax}" (current)
+            asm!("xrelease; lock; xaddq $2, $1"
+                 : "=r" (prev), "+*m" (self)
+                 : "0" (val.wrapping_neg())
                  : "memory"
                  : "volatile");
-            if prev == current {
-                Ok(prev)
-            } else {
-                Err(prev)
-            }
+            prev
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
@@ -18,6 +18,11 @@
 extern crate lock_api;
 extern crate parking_lot_core;
 
+#[cfg(test)]
+#[cfg(feature = "deadlock_detection")]
+#[macro_use]
+extern crate lazy_static;
+
 mod condvar;
 mod elision;
 mod mutex;

diff --git a/src/raw_mutex.rs b/src/raw_mutex.rs
@@ -82,7 +82,7 @@ unsafe impl RawMutexTrait for RawMutex {
         unsafe { deadlock::release_resource(self as *const _ as usize) };
         if self
             .state
-            .compare_exchange_weak(LOCKED_BIT, 0, Ordering::Release, Ordering::Relaxed)
+            .compare_exchange(LOCKED_BIT, 0, Ordering::Release, Ordering::Relaxed)
             .is_ok()
         {
             return;
@@ -97,7 +97,7 @@ unsafe impl RawMutexFair for RawMutex {
         unsafe { deadlock::release_resource(self as *const _ as usize) };
         if self
             .state
-            .compare_exchange_weak(LOCKED_BIT, 0, Ordering::Release, Ordering::Relaxed)
+            .compare_exchange(LOCKED_BIT, 0, Ordering::Release, Ordering::Relaxed)
             .is_ok()
         {
             return;
@@ -263,15 +263,6 @@ impl RawMutex {
     #[cold]
     #[inline(never)]
     fn unlock_slow(&self, force_fair: bool) {
-        // Unlock directly if there are no parked threads
-        if self
-            .state
-            .compare_exchange(LOCKED_BIT, 0, Ordering::Release, Ordering::Relaxed)
-            .is_ok()
-        {
-            return;
-        }
-
         // Unpark one thread and leave the parked bit set if there might
         // still be parked threads on this address.
         unsafe {