From 785be453f64c79ef054021a4c8b376d0b6a98fd1 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 14 Jun 2023 15:23:12 +0100 Subject: [PATCH 1/3] remove a race from the hashing write() Calling load() and store() separately leaves a lot of opportunity for a race to occur. This changes the logic to use compare_exchange() and will only return if the previous value is absolutely not 1 at the point of comparison. I also don't think this needs to be SeqCst, so relaxing that requirement a little. --- src/config/hashing.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/config/hashing.rs b/src/config/hashing.rs index 2ac191993..986ed8369 100644 --- a/src/config/hashing.rs +++ b/src/config/hashing.rs @@ -50,14 +50,17 @@ impl HokmaLock { pub fn write(&'static self) -> WhenTheHokmaSuppression { loop { - let previous = self.lock.load(Ordering::SeqCst); - self.lock.store(1, Ordering::SeqCst); - - if previous != 1 { - return WhenTheHokmaSuppression { - hokma: self, - state: previous, - }; + // We are only interested in error results + if let Err(previous) = self + .lock + .compare_exchange(1, 1, Ordering::Acquire, Ordering::Relaxed) + { + if previous != 1 { + return WhenTheHokmaSuppression { + hokma: self, + state: previous, + }; + } } } } From f7cc4c93d6b10a4ff214a9346d27f1823e8b436d Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 14 Jun 2023 15:29:51 +0100 Subject: [PATCH 2/3] remove redunant compare of previous and 1 since that's what the compare_exchange does... --- src/config/hashing.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/config/hashing.rs b/src/config/hashing.rs index 986ed8369..46fd14026 100644 --- a/src/config/hashing.rs +++ b/src/config/hashing.rs @@ -55,12 +55,11 @@ impl HokmaLock { .lock .compare_exchange(1, 1, Ordering::Acquire, Ordering::Relaxed) { - if previous != 1 { - return WhenTheHokmaSuppression { - hokma: self, - state: previous, - }; - } + // If we failed, previous cannot be 1 + return WhenTheHokmaSuppression { + hokma: self, + state: previous, + }; } } } From ee6b6d69ddad52e53c4dc90137969b4a400e88b1 Mon Sep 17 00:00:00 2001 From: Gary Pennington Date: Wed, 14 Jun 2023 17:03:56 +0100 Subject: [PATCH 3/3] persist in using SeqCst We probably don't need to, but let's keep the change minimal... --- src/config/hashing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/hashing.rs b/src/config/hashing.rs index 46fd14026..95005eab6 100644 --- a/src/config/hashing.rs +++ b/src/config/hashing.rs @@ -53,7 +53,7 @@ impl HokmaLock { // We are only interested in error results if let Err(previous) = self .lock - .compare_exchange(1, 1, Ordering::Acquire, Ordering::Relaxed) + .compare_exchange(1, 1, Ordering::SeqCst, Ordering::SeqCst) { // If we failed, previous cannot be 1 return WhenTheHokmaSuppression {