Swap `HashSet`s with custom `Hash` in debug_sync for `HashMap`s 2022-04-moar-lockorder
authorMatt Corallo <git@bluematt.me>
Tue, 19 Jul 2022 17:51:45 +0000 (17:51 +0000)
committerMatt Corallo <git@bluematt.me>
Wed, 20 Jul 2022 22:08:59 +0000 (22:08 +0000)
lightning/src/debug_sync.rs

index 6f5f532013e2fcce6a52d6fe8c6485f5a0af51b7..daec309e9b5e35f81684ef9cdc5ed1df2be9e55c 100644 (file)
@@ -2,7 +2,6 @@ pub use ::alloc::sync::Arc;
 use core::ops::{Deref, DerefMut};
 use core::time::Duration;
 
-use std::collections::HashSet;
 use std::cell::RefCell;
 
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -13,8 +12,10 @@ use std::sync::RwLockReadGuard as StdRwLockReadGuard;
 use std::sync::RwLockWriteGuard as StdRwLockWriteGuard;
 use std::sync::Condvar as StdCondvar;
 
+use prelude::HashMap;
+
 #[cfg(feature = "backtrace")]
-use {prelude::{HashMap, hash_map}, backtrace::Backtrace, std::sync::Once};
+use {prelude::hash_map, backtrace::Backtrace, std::sync::Once};
 
 #[cfg(not(feature = "backtrace"))]
 struct Backtrace{}
@@ -48,7 +49,7 @@ impl Condvar {
 
 thread_local! {
        /// We track the set of locks currently held by a reference to their `LockMetadata`
-       static LOCKS_HELD: RefCell<HashSet<Arc<LockMetadata>>> = RefCell::new(HashSet::new());
+       static LOCKS_HELD: RefCell<HashMap<u64, Arc<LockMetadata>>> = RefCell::new(HashMap::new());
 }
 static LOCK_IDX: AtomicUsize = AtomicUsize::new(0);
 
@@ -61,34 +62,13 @@ static LOCKS_INIT: Once = Once::new();
 /// when the Mutex itself was constructed.
 struct LockMetadata {
        lock_idx: u64,
-       locked_before: StdMutex<HashSet<LockDep>>,
+       locked_before: StdMutex<HashMap<u64, LockDep>>,
        _lock_construction_bt: Backtrace,
 }
-impl PartialEq for LockMetadata {
-       fn eq(&self, o: &LockMetadata) -> bool { self.lock_idx == o.lock_idx }
-}
-impl Eq for LockMetadata {}
-impl std::hash::Hash for LockMetadata {
-       fn hash<H: std::hash::Hasher>(&self, hasher: &mut H) { hasher.write_u64(self.lock_idx); }
-}
 
 struct LockDep {
        lock: Arc<LockMetadata>,
-       lockdep_trace: Option<Backtrace>,
-}
-impl LockDep {
-       /// Note that `Backtrace::new()` is rather expensive so we rely on the caller to fill in the
-       /// `lockdep_backtrace` field after ensuring we need it.
-       fn new_without_bt(lock: &Arc<LockMetadata>) -> Self {
-               Self { lock: Arc::clone(lock), lockdep_trace: None }
-       }
-}
-impl PartialEq for LockDep {
-       fn eq(&self, o: &LockDep) -> bool { self.lock.lock_idx == o.lock.lock_idx }
-}
-impl Eq for LockDep {}
-impl std::hash::Hash for LockDep {
-       fn hash<H: std::hash::Hasher>(&self, hasher: &mut H) { hasher.write_u64(self.lock.lock_idx); }
+       lockdep_trace: Backtrace,
 }
 
 #[cfg(feature = "backtrace")]
@@ -123,7 +103,7 @@ impl LockMetadata {
                let lock_idx = LOCK_IDX.fetch_add(1, Ordering::Relaxed) as u64;
 
                let res = Arc::new(LockMetadata {
-                       locked_before: StdMutex::new(HashSet::new()),
+                       locked_before: StdMutex::new(HashMap::new()),
                        lock_idx,
                        _lock_construction_bt: backtrace,
                });
@@ -148,20 +128,20 @@ impl LockMetadata {
                        // For each lock which is currently locked, check that no lock's locked-before
                        // set includes the lock we're about to lock, which would imply a lockorder
                        // inversion.
-                       for locked in held.borrow().iter() {
-                               if read && *locked == *this {
+                       for (locked_idx, _locked) in held.borrow().iter() {
+                               if read && *locked_idx == this.lock_idx {
                                        // Recursive read locks are explicitly allowed
                                        return;
                                }
                        }
-                       for locked in held.borrow().iter() {
-                               if !read && *locked == *this {
+                       for (locked_idx, locked) in held.borrow().iter() {
+                               if !read && *locked_idx == this.lock_idx {
                                        // With `feature = "backtrace"` set, we may be looking at different instances
                                        // of the same lock.
                                        debug_assert!(cfg!(feature = "backtrace"), "Tried to acquire a lock while it was held!");
                                }
-                               for locked_dep in locked.locked_before.lock().unwrap().iter() {
-                                       if locked_dep.lock == *this && locked_dep.lock != *locked {
+                               for (locked_dep_idx, locked_dep) in locked.locked_before.lock().unwrap().iter() {
+                                       if *locked_dep_idx == this.lock_idx && *locked_dep_idx != locked.lock_idx {
                                                #[cfg(feature = "backtrace")]
                                                panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n",
                                                        get_construction_location(&this._lock_construction_bt), this.lock_idx, this._lock_construction_bt,
@@ -173,13 +153,12 @@ impl LockMetadata {
                                }
                                // Insert any already-held locks in our locked-before set.
                                let mut locked_before = this.locked_before.lock().unwrap();
-                               let mut lockdep = LockDep::new_without_bt(locked);
-                               if !locked_before.contains(&lockdep) {
-                                       lockdep.lockdep_trace = Some(Backtrace::new());
-                                       locked_before.insert(lockdep);
+                               if !locked_before.contains_key(&locked.lock_idx) {
+                                       let lockdep = LockDep { lock: Arc::clone(locked), lockdep_trace: Backtrace::new() };
+                                       locked_before.insert(lockdep.lock.lock_idx, lockdep);
                                }
                        }
-                       held.borrow_mut().insert(Arc::clone(this));
+                       held.borrow_mut().insert(this.lock_idx, Arc::clone(this));
                        inserted = true;
                });
                inserted
@@ -194,14 +173,13 @@ impl LockMetadata {
                        // consider try-locks to ever generate lockorder inversions. However, if a try-lock
                        // succeeds, we do consider it to have created lockorder dependencies.
                        let mut locked_before = this.locked_before.lock().unwrap();
-                       for locked in held.borrow().iter() {
-                               let mut lockdep = LockDep::new_without_bt(locked);
-                               if !locked_before.contains(&lockdep) {
-                                       lockdep.lockdep_trace = Some(Backtrace::new());
-                                       locked_before.insert(lockdep);
+                       for (locked_idx, locked) in held.borrow().iter() {
+                               if !locked_before.contains_key(locked_idx) {
+                                       let lockdep = LockDep { lock: Arc::clone(locked), lockdep_trace: Backtrace::new() };
+                                       locked_before.insert(*locked_idx, lockdep);
                                }
                        }
-                       held.borrow_mut().insert(Arc::clone(this));
+                       held.borrow_mut().insert(this.lock_idx, Arc::clone(this));
                });
        }
 }
@@ -231,7 +209,7 @@ impl<'a, T: Sized> MutexGuard<'a, T> {
 impl<T: Sized> Drop for MutexGuard<'_, T> {
        fn drop(&mut self) {
                LOCKS_HELD.with(|held| {
-                       held.borrow_mut().remove(&self.mutex.deps);
+                       held.borrow_mut().remove(&self.mutex.deps.lock_idx);
                });
        }
 }
@@ -302,7 +280,7 @@ impl<T: Sized> Drop for RwLockReadGuard<'_, T> {
                        return;
                }
                LOCKS_HELD.with(|held| {
-                       held.borrow_mut().remove(&self.lock.deps);
+                       held.borrow_mut().remove(&self.lock.deps.lock_idx);
                });
        }
 }
@@ -318,7 +296,7 @@ impl<T: Sized> Deref for RwLockWriteGuard<'_, T> {
 impl<T: Sized> Drop for RwLockWriteGuard<'_, T> {
        fn drop(&mut self) {
                LOCKS_HELD.with(|held| {
-                       held.borrow_mut().remove(&self.lock.deps);
+                       held.borrow_mut().remove(&self.lock.deps.lock_idx);
                });
        }
 }