From fab659734c4ca10cc1deee6a53363860ae0fb169 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Wed, 2 Oct 2019 22:42:18 +0200 Subject: [PATCH 1/7] Add documentation to HASHTABLE static --- core/src/parking_lot.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index 82f48fa..2569953 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -17,6 +17,13 @@ use smallvec::SmallVec; use std::time::{Duration, Instant}; static NUM_THREADS: AtomicUsize = AtomicUsize::new(0); + +/// Holds the pointer to the currently active `HashTable`. +/// +/// # Safety +/// +/// Except for the initial value of null, it must always point to a valid `HashTable` instance. +/// Any `HashTable` this global static has ever pointed to must never be freed. static HASHTABLE: AtomicPtr = AtomicPtr::new(ptr::null_mut()); // Even with 3x more buckets than threads, the memory overhead per thread is From 964a923015f7e30bc8b5dc77bb20f1ee8ad3fb65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Tue, 1 Oct 2019 23:54:51 +0200 Subject: [PATCH 2/7] Add safety notice to all bucket mutex unlocks --- core/src/parking_lot.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index 2569953..f4bc560 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -253,6 +253,7 @@ unsafe fn grow_hashtable(num_threads: usize) { // Unlock buckets and try again for b in &(*old_table).entries[..] { + // SAFETY: We hold the lock here, as required b.mutex.unlock(); } @@ -288,6 +289,7 @@ unsafe fn grow_hashtable(num_threads: usize) { // Unlock all buckets in the old table for b in &(*old_table).entries[..] { + // SAFETY: We hold the lock here, as required b.mutex.unlock(); } } @@ -544,6 +546,7 @@ pub unsafe fn park( // If the validation function fails, just return if !validate() { + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); return ParkResult::Invalid; } @@ -560,6 +563,7 @@ pub unsafe fn park( bucket.queue_head.set(thread_data); } bucket.queue_tail.set(thread_data); + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); // Invoke the pre-sleep callback @@ -590,6 +594,7 @@ pub unsafe fn park( // Now we need to check again if we were unparked or timed out. Unlike the // last check this is precise because we hold the bucket lock. if !thread_data.parker.timed_out() { + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); return ParkResult::Unparked(thread_data.unpark_token.get()); } @@ -637,6 +642,7 @@ pub unsafe fn park( debug_assert!(!current.is_null()); // Unlock the bucket, we are done + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); ParkResult::TimedOut }) @@ -708,6 +714,7 @@ pub unsafe fn unpark_one( // the queue locked while we perform a system call. Finally we wake // up the parked thread. let handle = (*current).parker.unpark_lock(); + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); handle.unpark(); @@ -721,6 +728,7 @@ pub unsafe fn unpark_one( // No threads with a matching key were found in the bucket callback(result); + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); result } @@ -771,6 +779,7 @@ pub unsafe fn unpark_all(key: usize, unpark_token: UnparkToken) -> usize { } // Unlock the bucket + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); // Now that we are outside the lock, wake up all the threads that we removed @@ -1003,6 +1012,7 @@ pub unsafe fn unpark_filter( t.1 = Some((*t.0).parker.unpark_lock()); } + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); // Now that we are outside the lock, wake up all the threads that we removed @@ -1195,6 +1205,7 @@ mod deadlock_impl { } current = (*current).next_in_queue.get(); } + // SAFETY: We hold the lock here, as required b.mutex.unlock(); } @@ -1232,6 +1243,7 @@ mod deadlock_impl { // Unlock buckets and try again for b in &(*table).entries[..] { + // SAFETY: We hold the lock here, as required b.mutex.unlock(); } @@ -1264,6 +1276,7 @@ mod deadlock_impl { } for b in &(*table).entries[..] { + // SAFETY: We hold the lock here, as required b.mutex.unlock(); } @@ -1279,6 +1292,7 @@ mod deadlock_impl { (*td).deadlock_data.deadlocked.set(true); *(*td).deadlock_data.backtrace_sender.get() = Some(sender.clone()); let handle = (*td).parker.unpark_lock(); + // SAFETY: We hold the lock here, as required bucket.mutex.unlock(); // unpark the deadlocked thread! // on unpark it'll notice the deadlocked flag and report back From d0e082b7ec2c21a2ead18d54bcc46c4717731d15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Tue, 1 Oct 2019 23:25:34 +0200 Subject: [PATCH 3/7] Make {get,create}_hashtable return static reference --- core/src/parking_lot.rs | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index f4bc560..f8e9c92 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -191,27 +191,31 @@ impl Drop for ThreadData { } } -// Get a pointer to the latest hash table, creating one if it doesn't exist yet. +/// Returns a reference to the latest hash table, creating one if it doesn't exist yet. +/// The reference is valid forever. However, the `HashTable` it references might become stale +/// at any point. Meaning it still exists, but it is not the instance in active use. #[inline] -fn get_hashtable() -> *mut HashTable { +fn get_hashtable() -> &'static HashTable { let table = HASHTABLE.load(Ordering::Acquire); // If there is no table, create one if table.is_null() { create_hashtable() } else { - table + // SAFETY: when not null, `HASHTABLE` always points to a `HashTable` that is never freed. + unsafe { &*table } } } -// Get a pointer to the latest hash table, creating one if it doesn't exist yet. +/// Returns a reference to the latest hash table, creating one if it doesn't exist yet. +/// The reference is valid forever. However, the `HashTable` it references might become stale +/// at any point. Meaning it still exists, but it is not the instance in active use. #[cold] -fn create_hashtable() -> *mut HashTable { +fn create_hashtable() -> &'static HashTable { let new_table = Box::into_raw(HashTable::new(LOAD_FACTOR, ptr::null())); - // If this fails then it means some other thread created the hash - // table first. - match HASHTABLE.compare_exchange( + // If this fails then it means some other thread created the hash table first. + let table = match HASHTABLE.compare_exchange( ptr::null_mut(), new_table, Ordering::Release, @@ -220,12 +224,16 @@ fn create_hashtable() -> *mut HashTable { Ok(_) => new_table, Err(old_table) => { // Free the table we created + // SAFETY: `new_table` is created from `Box::into_raw` above and only freed here. unsafe { Box::from_raw(new_table); } old_table } - } + }; + // SAFETY: The `HashTable` behind `table` is never freed. It is either the table pointer we + // created here, or it is one loaded from `HASHTABLE`. + unsafe { &*table } } // Grow the hash table so that it is big enough for the given number of threads. @@ -247,7 +255,7 @@ unsafe fn grow_hashtable(num_threads: usize) { // Now check if our table is still the latest one. Another thread could // have grown the hash table between us reading HASHTABLE and locking // the buckets. - if HASHTABLE.load(Ordering::Relaxed) == old_table { + if HASHTABLE.load(Ordering::Relaxed) == old_table as *const _ as *mut _ { break; } @@ -257,7 +265,9 @@ unsafe fn grow_hashtable(num_threads: usize) { b.mutex.unlock(); } - old_table = HASHTABLE.load(Ordering::Acquire); + // SAFETY: when not null, `HASHTABLE` always points to a `HashTable` that is never freed. + // Here we know `get_hashtable` above must have initialized `HASHTABLE` + old_table = &*HASHTABLE.load(Ordering::Acquire); } // Create the new table @@ -321,7 +331,7 @@ unsafe fn lock_bucket<'a>(key: usize) -> &'a Bucket { // If no other thread has rehashed the table before we grabbed the lock // then we are good to go! The lock we grabbed prevents any rehashes. - if HASHTABLE.load(Ordering::Relaxed) == hashtable { + if HASHTABLE.load(Ordering::Relaxed) == hashtable as *const _ as *mut _ { return bucket; } @@ -348,7 +358,7 @@ unsafe fn lock_bucket_checked<'a>(key: &AtomicUsize) -> (usize, &'a Bucket) { // Check that both the hash table and key are correct while the bucket // is locked. Note that the key can't change once we locked the proper // bucket for it, so we just keep trying until we have the correct key. - if HASHTABLE.load(Ordering::Relaxed) == hashtable + if HASHTABLE.load(Ordering::Relaxed) == hashtable as *const _ as *mut _ && key.load(Ordering::Relaxed) == current_key { return (current_key, bucket); @@ -380,7 +390,7 @@ unsafe fn lock_bucket_pair<'a>(key1: usize, key2: usize) -> (&'a Bucket, &'a Buc // If no other thread has rehashed the table before we grabbed the lock // then we are good to go! The lock we grabbed prevents any rehashes. - if HASHTABLE.load(Ordering::Relaxed) == hashtable { + if HASHTABLE.load(Ordering::Relaxed) == hashtable as *const _ as *mut _ { // Now lock the second bucket and return the two buckets if hash1 == hash2 { return (bucket1, bucket1); From c0d9ba114eedbdff8abbea5ee776ad8c91f3cfe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Tue, 1 Oct 2019 23:40:38 +0200 Subject: [PATCH 4/7] Make lock_bucket functions not unsafe --- core/src/parking_lot.rs | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index f8e9c92..2ab1969 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -318,13 +318,13 @@ fn hash(key: usize, bits: u32) -> usize { // Lock the bucket for the given key #[inline] -unsafe fn lock_bucket<'a>(key: usize) -> &'a Bucket { +fn lock_bucket(key: usize) -> &'static Bucket { let mut bucket; loop { let hashtable = get_hashtable(); - let hash = hash(key, (*hashtable).hash_bits); - bucket = &(*hashtable).entries[hash]; + let hash = hash(key, hashtable.hash_bits); + bucket = &hashtable.entries[hash]; // Lock the bucket bucket.mutex.lock(); @@ -336,21 +336,22 @@ unsafe fn lock_bucket<'a>(key: usize) -> &'a Bucket { } // Unlock the bucket and try again - bucket.mutex.unlock(); + // SAFETY: We hold the lock here, as required + unsafe { bucket.mutex.unlock() }; } } // Lock the bucket for the given key, but check that the key hasn't been changed // in the meantime due to a requeue. #[inline] -unsafe fn lock_bucket_checked<'a>(key: &AtomicUsize) -> (usize, &'a Bucket) { +fn lock_bucket_checked(key: &AtomicUsize) -> (usize, &'static Bucket) { let mut bucket; loop { let hashtable = get_hashtable(); let current_key = key.load(Ordering::Relaxed); - let hash = hash(current_key, (*hashtable).hash_bits); - bucket = &(*hashtable).entries[hash]; + let hash = hash(current_key, hashtable.hash_bits); + bucket = &hashtable.entries[hash]; // Lock the bucket bucket.mutex.lock(); @@ -365,24 +366,25 @@ unsafe fn lock_bucket_checked<'a>(key: &AtomicUsize) -> (usize, &'a Bucket) { } // Unlock the bucket and try again - bucket.mutex.unlock(); + // SAFETY: We hold the lock here, as required + unsafe { bucket.mutex.unlock() }; } } // Lock the two buckets for the given pair of keys #[inline] -unsafe fn lock_bucket_pair<'a>(key1: usize, key2: usize) -> (&'a Bucket, &'a Bucket) { +fn lock_bucket_pair(key1: usize, key2: usize) -> (&'static Bucket, &'static Bucket) { let mut bucket1; loop { let hashtable = get_hashtable(); // Get the lowest bucket first - let hash1 = hash(key1, (*hashtable).hash_bits); - let hash2 = hash(key2, (*hashtable).hash_bits); + let hash1 = hash(key1, hashtable.hash_bits); + let hash2 = hash(key2, hashtable.hash_bits); if hash1 <= hash2 { - bucket1 = &(*hashtable).entries[hash1]; + bucket1 = &hashtable.entries[hash1]; } else { - bucket1 = &(*hashtable).entries[hash2]; + bucket1 = &hashtable.entries[hash2]; } // Lock the first bucket @@ -395,18 +397,19 @@ unsafe fn lock_bucket_pair<'a>(key1: usize, key2: usize) -> (&'a Bucket, &'a Buc if hash1 == hash2 { return (bucket1, bucket1); } else if hash1 < hash2 { - let bucket2 = &(*hashtable).entries[hash2]; + let bucket2 = &hashtable.entries[hash2]; bucket2.mutex.lock(); return (bucket1, bucket2); } else { - let bucket2 = &(*hashtable).entries[hash1]; + let bucket2 = &hashtable.entries[hash1]; bucket2.mutex.lock(); return (bucket2, bucket1); } } // Unlock the bucket and try again - bucket1.mutex.unlock(); + // SAFETY: We hold the lock here, as required + unsafe { bucket1.mutex.unlock() }; } } From 7016b91efb1a7f8cc41faa04028758bc956a485b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Tue, 1 Oct 2019 23:50:05 +0200 Subject: [PATCH 5/7] Add documentation safety section to unlock_bucket_pair --- core/src/parking_lot.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index 2ab1969..584bee2 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -413,7 +413,11 @@ fn lock_bucket_pair(key1: usize, key2: usize) -> (&'static Bucket, &'static Buck } } -// Unlock a pair of buckets +/// Unlock a pair of buckets +/// +/// # Safety +/// +/// Both buckets must be locked #[inline] unsafe fn unlock_bucket_pair(bucket1: &Bucket, bucket2: &Bucket) { bucket1.mutex.unlock(); @@ -846,6 +850,7 @@ pub unsafe fn unpark_requeue( let mut result = UnparkResult::default(); let op = validate(); if op == RequeueOp::Abort { + // SAFETY: Both buckets are locked, as required. unlock_bucket_pair(bucket_from, bucket_to); return result; } @@ -926,9 +931,11 @@ pub unsafe fn unpark_requeue( if let Some(wakeup_thread) = wakeup_thread { (*wakeup_thread).unpark_token.set(token); let handle = (*wakeup_thread).parker.unpark_lock(); + // SAFETY: Both buckets are locked, as required. unlock_bucket_pair(bucket_from, bucket_to); handle.unpark(); } else { + // SAFETY: Both buckets are locked, as required. unlock_bucket_pair(bucket_from, bucket_to); } From 3a92c14418634c1c5ef979acf647d4e29eb5f9d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Thu, 3 Oct 2019 09:09:05 +0200 Subject: [PATCH 6/7] Add lock_bucket documentation --- core/src/parking_lot.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index 584bee2..63f6a28 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -316,7 +316,8 @@ fn hash(key: usize, bits: u32) -> usize { key.wrapping_mul(0x9E3779B97F4A7C15) >> (64 - bits) } -// Lock the bucket for the given key +/// Locks the bucket for the given key and returns a reference to it. +/// The returned bucket must be unlocked again in order to not cause deadlocks. #[inline] fn lock_bucket(key: usize) -> &'static Bucket { let mut bucket; @@ -341,8 +342,9 @@ fn lock_bucket(key: usize) -> &'static Bucket { } } -// Lock the bucket for the given key, but check that the key hasn't been changed -// in the meantime due to a requeue. +/// Locks the bucket for the given key and returns a reference to it. But checks that the key +/// hasn't been changed in the meantime due to a requeue. +/// The returned bucket must be unlocked again in order to not cause deadlocks. #[inline] fn lock_bucket_checked(key: &AtomicUsize) -> (usize, &'static Bucket) { let mut bucket; @@ -371,7 +373,11 @@ fn lock_bucket_checked(key: &AtomicUsize) -> (usize, &'static Bucket) { } } -// Lock the two buckets for the given pair of keys +/// Locks the two buckets for the given pair of keys and returns references to them. +/// The returned buckets must be unlocked again in order to not cause deadlocks. +/// +/// If both keys hash to the same value, both returned references will be to the same bucket. Be +/// careful to only unlock it once in this case, always use `unlock_bucket_pair`. #[inline] fn lock_bucket_pair(key1: usize, key2: usize) -> (&'static Bucket, &'static Bucket) { let mut bucket1; From 39167068f8a26f31955e6ce204de21e6e01a3b97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Thu, 3 Oct 2019 09:30:08 +0200 Subject: [PATCH 7/7] Fix deadlock detection for new get_hashtable --- core/src/parking_lot.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/parking_lot.rs b/core/src/parking_lot.rs index 63f6a28..5c45dce 100644 --- a/core/src/parking_lot.rs +++ b/core/src/parking_lot.rs @@ -1256,19 +1256,19 @@ mod deadlock_impl { let mut table = get_hashtable(); loop { // Lock all buckets in the old table - for b in &(*table).entries[..] { + for b in &table.entries[..] { b.mutex.lock(); } // Now check if our table is still the latest one. Another thread could // have grown the hash table between us getting and locking the hash table. let new_table = get_hashtable(); - if new_table == table { + if new_table as *const _ == table as *const _ { break; } // Unlock buckets and try again - for b in &(*table).entries[..] { + for b in &table.entries[..] { // SAFETY: We hold the lock here, as required b.mutex.unlock(); } @@ -1280,7 +1280,7 @@ mod deadlock_impl { let mut graph = DiGraphMap::::with_capacity(thread_count * 2, thread_count * 2); - for b in &(*table).entries[..] { + for b in &table.entries[..] { let mut current = b.queue_head.get(); while !current.is_null() { if !(*current).parked_with_timeout.get() @@ -1301,7 +1301,7 @@ mod deadlock_impl { } } - for b in &(*table).entries[..] { + for b in &table.entries[..] { // SAFETY: We hold the lock here, as required b.mutex.unlock(); }