From 0786d6853236cc68003d2b00f5adcf8e662439eb Mon Sep 17 00:00:00 2001 From: Thomas Schaller Date: Fri, 8 Oct 2021 22:32:48 +0200 Subject: [PATCH 1/3] Use Acq/Rel ordering everywhere Fixes #15 --- examples/fifo.rs | 10 ++++---- examples/simple.rs | 6 ++--- src/lib.rs | 61 ++++++++++++++++++++-------------------------- tests/atom.rs | 60 ++++++++++++++++++++++----------------------- 4 files changed, 65 insertions(+), 72 deletions(-) diff --git a/examples/fifo.rs b/examples/fifo.rs index b10c5d1..0cbd761 100644 --- a/examples/fifo.rs +++ b/examples/fifo.rs @@ -16,7 +16,7 @@ extern crate atom; use atom::*; use std::mem; -use std::sync::{atomic::Ordering, Arc, Barrier}; +use std::sync::{ Arc, Barrier}; use std::thread; const THREADS: usize = 100; @@ -29,7 +29,7 @@ struct Link { impl Drop for Link { fn drop(&mut self) { // This is done to avoid a recusive drop of the List - while let Some(mut h) = self.next.atom().take(Ordering::Acquire) { + while let Some(mut h) = self.next.atom().take() { self.next = mem::replace(&mut h.next, AtomSetOnce::empty()); } } @@ -54,12 +54,12 @@ fn main() { }); loop { - while let Some(h) = hptr.next.get(Ordering::Acquire) { + while let Some(h) = hptr.next.get() { hptr = h; } my_awesome_node = - match hptr.next.set_if_none(my_awesome_node, Ordering::Release) { + match hptr.next.set_if_none(my_awesome_node) { Some(v) => v, None => break, }; @@ -73,7 +73,7 @@ fn main() { let mut hptr = &*head; let mut count = 0; - while let Some(h) = hptr.next.get(Ordering::Acquire) { + while let Some(h) = hptr.next.get() { hptr = h; count += 1; } diff --git a/examples/simple.rs b/examples/simple.rs index feee330..cdf52f1 100644 --- a/examples/simple.rs +++ b/examples/simple.rs @@ -1,7 +1,7 @@ extern crate atom; use atom::*; -use std::sync::{atomic::Ordering, Arc}; +use std::sync::{Arc}; use std::thread; fn main() { @@ -9,7 +9,7 @@ fn main() { let shared_atom = Arc::new(Atom::empty()); // set the value 75 - shared_atom.swap(Box::new(75), Ordering::AcqRel); + shared_atom.swap(Box::new(75)); // Spawn a bunch of thread that will try and take the value let threads: Vec> = (0..8) @@ -17,7 +17,7 @@ fn main() { let shared_atom = shared_atom.clone(); thread::spawn(move || { // Take the contents of the atom, only one will win the race - if let Some(v) = shared_atom.take(Ordering::Acquire) { + if let Some(v) = shared_atom.take() { println!("I got it: {:?} :D", v); } else { println!("I did not get it :("); diff --git a/src/lib.rs b/src/lib.rs index ac0188b..90f7b1d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -63,17 +63,17 @@ where /// Swap a new value into the Atom, This will try multiple /// times until it succeeds. The old value will be returned. - pub fn swap(&self, v: P, order: Ordering) -> Option

{ + pub fn swap(&self, v: P) -> Option

{ let new = v.into_raw(); - let old = self.inner.swap(new, order); + let old = self.inner.swap(new, Ordering::AcqRel); unsafe { Self::inner_from_raw(old) } } /// Take the value of the Atom replacing it with null pointer /// Returning the contents. If the contents was a `null` pointer the /// result will be `None`. - pub fn take(&self, order: Ordering) -> Option

{ - let old = self.inner.swap(ptr::null_mut(), order); + pub fn take(&self) -> Option

{ + let old = self.inner.swap(ptr::null_mut(), Ordering::AcqRel); unsafe { Self::inner_from_raw(old) } } @@ -81,10 +81,10 @@ where /// this will return `None` if the value was written, /// otherwise a `Some(v)` will be returned, where the value was /// the same value that you passed into this function - pub fn set_if_none(&self, v: P, order: Ordering) -> Option

{ + pub fn set_if_none(&self, v: P) -> Option

{ let new = v.into_raw(); - let old = self.inner.compare_and_swap(ptr::null_mut(), new, order); - if !old.is_null() { + let result = self.inner.compare_exchange(ptr::null_mut(), new, Ordering::AcqRel, Ordering::Acquire); + if result.is_err() { Some(unsafe { FromRawPtr::from_raw(new) }) } else { None @@ -98,8 +98,6 @@ where pub fn replace_and_set_next( &self, mut value: P, - load_order: Ordering, - cas_order: Ordering, ) -> bool where P: GetNextMut>, @@ -110,12 +108,13 @@ where // assert that it was droppeds unsafe { ptr::drop_in_place(next) }; loop { - let pcurrent = self.inner.load(load_order); + let pcurrent = self.inner.load(Ordering::Acquire); let current = unsafe { Self::inner_from_raw(pcurrent) }; unsafe { ptr::write(next, current) }; - let last = self.inner.compare_and_swap(pcurrent, raw, cas_order); - if last == pcurrent { - return last.is_null(); + let result = self.inner.compare_exchange(pcurrent, raw, Ordering::AcqRel, Ordering::Acquire); + match result { + Ok(replaced_ptr) => return replaced_ptr.is_null(), + _ => {} } } } @@ -163,15 +162,13 @@ where &self, current: Option<&P>, new: Option

, - order: Ordering, ) -> Result, (Option

, *mut P)> { let pcurrent = Self::inner_as_ptr(current); let pnew = Self::inner_into_raw(new); - let pprev = self.inner.compare_and_swap(pcurrent, pnew, order); - if pprev == pcurrent { - Ok(unsafe { Self::inner_from_raw(pprev) }) - } else { - Err((unsafe { Self::inner_from_raw(pnew) }, pprev as *mut P)) + let pprev = self.inner.compare_exchange(pcurrent, pnew, Ordering::AcqRel, Ordering::Acquire); + match pprev { + Ok(pprev) => Ok(unsafe { Self::inner_from_raw(pprev) }), + Err(pprev) => Err((unsafe { Self::inner_from_raw(pnew) }, pprev as *mut P)) } } @@ -192,12 +189,10 @@ where &self, current: Option<&P>, new: Option

, - success: Ordering, - failure: Ordering, ) -> Result, (Option

, *mut P)> { let pnew = Self::inner_into_raw(new); self.inner - .compare_exchange(Self::inner_as_ptr(current), pnew, success, failure) + .compare_exchange(Self::inner_as_ptr(current), pnew, Ordering::AcqRel, Ordering::Acquire) .map(|pprev| unsafe { Self::inner_from_raw(pprev) }) .map_err(|pprev| (unsafe { Self::inner_from_raw(pnew) }, pprev as *mut P)) } @@ -220,12 +215,10 @@ where &self, current: Option<&P>, new: Option

, - success: Ordering, - failure: Ordering, ) -> Result, (Option

, *mut P)> { let pnew = Self::inner_into_raw(new); self.inner - .compare_exchange_weak(Self::inner_as_ptr(current), pnew, success, failure) + .compare_exchange_weak(Self::inner_as_ptr(current), pnew, Ordering::AcqRel, Ordering::Acquire) .map(|pprev| unsafe { Self::inner_from_raw(pprev) }) .map_err(|pprev| (unsafe { Self::inner_from_raw(pnew) }, pprev as *mut P)) } @@ -244,7 +237,7 @@ where P: IntoRawPtr + FromRawPtr, { fn drop(&mut self) { - self.take(Ordering::Relaxed); + self.take(); } } @@ -360,8 +353,8 @@ where /// this will return `OK(())` if the value was written, /// otherwise a `Err(P)` will be returned, where the value was /// the same value that you passed into this function - pub fn set_if_none(&self, v: P, order: Ordering) -> Option

{ - self.inner.set_if_none(v, order) + pub fn set_if_none(&self, v: P) -> Option

{ + self.inner.set_if_none(v) } /// Convert an `AtomSetOnce` into an `Atom` @@ -387,8 +380,8 @@ where P: IntoRawPtr + FromRawPtr + Deref, { /// If the Atom is set, get the value - pub fn get(&self, order: Ordering) -> Option<&T> { - let ptr = self.inner.inner.load(order); + pub fn get(&self) -> Option<&T> { + let ptr = self.inner.inner.load(Ordering::Acquire); let val = unsafe { Atom::inner_from_raw(ptr) }; val.map(|v: P| { // This is safe since ptr cannot be changed once it is set @@ -402,8 +395,8 @@ where impl AtomSetOnce> { /// If the Atom is set, get the value - pub fn get_mut(&mut self, order: Ordering) -> Option<&mut T> { - let ptr = self.inner.inner.load(order); + pub fn get_mut(&mut self) -> Option<&mut T> { + let ptr = self.inner.inner.load(Ordering::Acquire); let val = unsafe { Atom::inner_from_raw(ptr) }; val.map(move |mut v: Box| { // This is safe since ptr cannot be changed once it is set @@ -420,8 +413,8 @@ where T: Clone + IntoRawPtr + FromRawPtr, { /// Duplicate the inner pointer if it is set - pub fn dup(&self, order: Ordering) -> Option { - let ptr = self.inner.inner.load(order); + pub fn dup(&self) -> Option { + let ptr = self.inner.inner.load(Ordering::Acquire); let val = unsafe { Atom::inner_from_raw(ptr) }; val.map(|v: T| { let out = v.clone(); diff --git a/tests/atom.rs b/tests/atom.rs index 17f7e08..4bf9718 100644 --- a/tests/atom.rs +++ b/tests/atom.rs @@ -24,24 +24,24 @@ use std::thread; #[test] fn swap() { let a = Atom::empty(); - assert_eq!(a.swap(Box::new(1u8), Ordering::AcqRel), None); - assert_eq!(a.swap(Box::new(2u8), Ordering::AcqRel), Some(Box::new(1u8))); - assert_eq!(a.swap(Box::new(3u8), Ordering::AcqRel), Some(Box::new(2u8))); + assert_eq!(a.swap(Box::new(1u8)), None); + assert_eq!(a.swap(Box::new(2u8)), Some(Box::new(1u8))); + assert_eq!(a.swap(Box::new(3u8)), Some(Box::new(2u8))); } #[test] fn take() { let a = Atom::new(Box::new(7u8)); - assert_eq!(a.take(Ordering::Acquire), Some(Box::new(7))); - assert_eq!(a.take(Ordering::Acquire), None); + assert_eq!(a.take(), Some(Box::new(7))); + assert_eq!(a.take(), None); } #[test] fn set_if_none() { let a = Atom::empty(); - assert_eq!(a.set_if_none(Box::new(7u8), Ordering::Release), None); + assert_eq!(a.set_if_none(Box::new(7u8)), None); assert_eq!( - a.set_if_none(Box::new(8u8), Ordering::Release), + a.set_if_none(Box::new(8u8)), Some(Box::new(8u8)) ); } @@ -49,42 +49,42 @@ fn set_if_none() { #[test] fn compare_and_swap_basics() { cas_test_basics_helper(|a, cas_val, next_val| { - a.compare_and_swap(cas_val, next_val, Ordering::SeqCst) + a.compare_and_swap(cas_val, next_val) }); } #[test] fn compare_exchange_basics() { cas_test_basics_helper(|a, cas_val, next_val| { - a.compare_exchange(cas_val, next_val, Ordering::SeqCst, Ordering::SeqCst) + a.compare_exchange(cas_val, next_val) }); } #[test] fn compare_exchange_weak_basics() { cas_test_basics_helper(|a, cas_val, next_val| { - a.compare_exchange_weak(cas_val, next_val, Ordering::SeqCst, Ordering::SeqCst) + a.compare_exchange_weak(cas_val, next_val) }); } #[test] fn compare_and_swap_threads() { cas_test_threads_helper(|a, cas_val, next_val| { - a.compare_and_swap(cas_val, next_val, Ordering::SeqCst) + a.compare_and_swap(cas_val, next_val) }); } #[test] fn compare_exchange_threads() { cas_test_threads_helper(|a, cas_val, next_val| { - a.compare_exchange(cas_val, next_val, Ordering::SeqCst, Ordering::SeqCst) + a.compare_exchange(cas_val, next_val) }); } #[test] fn compare_exchange_weak_threads() { cas_test_threads_helper(|a, cas_val, next_val| { - a.compare_exchange_weak(cas_val, next_val, Ordering::SeqCst, Ordering::SeqCst) + a.compare_exchange_weak(cas_val, next_val) }); } @@ -157,7 +157,7 @@ fn cas_test_threads_helper(cas: TestCASFn) { .collect(); assert!(uniq_pprevs.contains(&cur_val.into_raw())); assert!(!uniq_pprevs.contains(&other_val.into_raw())); - assert_eq!(a.take(Ordering::Relaxed), Some(next_val)); + assert_eq!(a.take(), Some(next_val)); } #[derive(Clone)] @@ -197,34 +197,34 @@ fn ensure_send() { let w = wait.clone(); let a = atom.clone(); thread::spawn(move || { - a.swap(Box::new(7u8), Ordering::AcqRel); + a.swap(Box::new(7u8)); w.wait(); }); wait.wait(); - assert_eq!(atom.take(Ordering::Acquire), Some(Box::new(7u8))); + assert_eq!(atom.take(), Some(Box::new(7u8))); } #[test] fn get() { let atom = Arc::new(AtomSetOnce::empty()); - assert_eq!(atom.get(Ordering::Acquire), None); - assert_eq!(atom.set_if_none(Box::new(8u8), Ordering::Release), None); - assert_eq!(atom.get(Ordering::Acquire), Some(&8u8)); + assert_eq!(atom.get(), None); + assert_eq!(atom.set_if_none(Box::new(8u8)), None); + assert_eq!(atom.get(), Some(&8u8)); } #[test] fn get_arc() { let atom = Arc::new(AtomSetOnce::empty()); - assert_eq!(atom.get(Ordering::Acquire), None); - assert_eq!(atom.set_if_none(Arc::new(8u8), Ordering::Release), None); - assert_eq!(atom.get(Ordering::Acquire), Some(&8u8)); + assert_eq!(atom.get(), None); + assert_eq!(atom.set_if_none(Arc::new(8u8)), None); + assert_eq!(atom.get(), Some(&8u8)); let v = Arc::new(AtomicUsize::new(0)); let atom = Arc::new(AtomSetOnce::empty()); - atom.get(Ordering::Acquire); - atom.set_if_none(Arc::new(Canary(v.clone())), Ordering::Release); - atom.get(Ordering::Acquire); + atom.get(); + atom.set_if_none(Arc::new(Canary(v.clone()))); + atom.get(); drop(atom); assert_eq!(v.load(Ordering::SeqCst), 1); @@ -256,13 +256,13 @@ impl GetNextMut for Box { fn lifo() { let atom = Atom::empty(); for i in 0..100 { - let x = atom.replace_and_set_next(Link::new(99 - i), Ordering::Relaxed, Ordering::AcqRel); + let x = atom.replace_and_set_next(Link::new(99 - i)); assert_eq!(x, i == 0); } let expected: Vec = (0..100).collect(); let mut found = Vec::new(); - let mut chain = atom.take(Ordering::Acquire); + let mut chain = atom.take(); while let Some(v) = chain { found.push(v.value); chain = v.next; @@ -300,7 +300,7 @@ fn lifo_drop() { link.next = Some(LinkCanary::new(canary.clone())); let atom = Atom::empty(); - atom.replace_and_set_next(link, Ordering::Relaxed, Ordering::AcqRel); + atom.replace_and_set_next(link); assert_eq!(1, v.load(Ordering::SeqCst)); drop(atom); assert_eq!(2, v.load(Ordering::SeqCst)); @@ -309,6 +309,6 @@ fn lifo_drop() { #[test] fn borrow() { let a = Atom::new(&5); - assert_eq!(a.swap(&7, Ordering::Relaxed), Some(&5)); - assert_eq!(a.take(Ordering::Relaxed), Some(&7)); + assert_eq!(a.swap(&7), Some(&5)); + assert_eq!(a.take(), Some(&7)); } From 23b63415bbdf90492c1bb23b95486c9ab3bf98f4 Mon Sep 17 00:00:00 2001 From: Thomas Schaller Date: Fri, 8 Oct 2021 22:41:45 +0200 Subject: [PATCH 2/3] Bump version to 0.5.0 Also fix docs --- Cargo.toml | 2 +- src/lib.rs | 32 +++++++++++++++----------------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 44e8f71..df999d6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "atom" -version = "0.4.0" +version = "0.5.0" authors = ["Colin Sherratt "] license = "Apache-2.0" homepage = "https://github.com/slide-rs/atom" diff --git a/src/lib.rs b/src/lib.rs index 90f7b1d..58813ef 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,6 +24,21 @@ use std::sync::Arc; /// An Atom wraps an AtomicPtr, it allows for safe mutation of an atomic /// into common Rust Types. +/// +/// All `Ordering` is `AcqRel`. +/// +/// ``` +/// // Create an empty atom +/// use std::sync::Arc; +/// use atom::Atom; +/// +/// let shared_atom = Arc::new(Atom::empty()); +/// +/// shared_atom.set_if_none(Box::new(42)); +/// let old_value = shared_atom.swap(Box::new(75)); +/// +/// assert_eq!(old_value, Some(Box::new(42))); +/// ``` pub struct Atom

where P: IntoRawPtr + FromRawPtr, @@ -155,9 +170,6 @@ where /// returned together with a raw pointer to the Atom's current unchanged /// value, which is **not safe to dereference**, especially if the Atom is /// accessed from multiple threads. - /// - /// `compare_and_swap` also takes an `Ordering` argument which describes - /// the memory ordering of this operation. pub fn compare_and_swap( &self, current: Option<&P>, @@ -178,13 +190,6 @@ where /// The return value is a result indicating whether the new value was /// written and containing the previous value. On success this value is /// guaranteed to be equal to `current`. - /// - /// `compare_exchange` takes two `Ordering` arguments to describe the - /// memory ordering of this operation. The first describes the required - /// ordering if the operation succeeds while the second describes the - /// required ordering when the operation fails. The failure ordering can't - /// be `Release` or `AcqRel` and must be equivalent or weaker than the - /// success ordering. pub fn compare_exchange( &self, current: Option<&P>, @@ -204,13 +209,6 @@ where /// even when the comparison succeeds, which can result in more efficient /// code on some platforms. The return value is a result indicating whether /// the new value was written and containing the previous value. - /// - /// `compare_exchange_weak` takes two `Ordering` arguments to describe the - /// memory ordering of this operation. The first describes the required - /// ordering if the operation succeeds while the second describes the - /// required ordering when the operation fails. The failure ordering can't - /// be `Release` or `AcqRel` and must be equivalent or weaker than the - /// success ordering. pub fn compare_exchange_weak( &self, current: Option<&P>, From 4ed46bf940e6fe35b15063731c638f43e7fbb0e0 Mon Sep 17 00:00:00 2001 From: Thomas Schaller Date: Sat, 9 Oct 2021 11:18:57 +0200 Subject: [PATCH 3/3] Relax some orderings Apply suggestions from code review Co-authored-by: yvt --- examples/fifo.rs | 2 +- src/lib.rs | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/examples/fifo.rs b/examples/fifo.rs index 0cbd761..8a4fbfc 100644 --- a/examples/fifo.rs +++ b/examples/fifo.rs @@ -16,7 +16,7 @@ extern crate atom; use atom::*; use std::mem; -use std::sync::{ Arc, Barrier}; +use std::sync::{Arc, Barrier}; use std::thread; const THREADS: usize = 100; diff --git a/src/lib.rs b/src/lib.rs index 58813ef..7423c24 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,7 +25,10 @@ use std::sync::Arc; /// An Atom wraps an AtomicPtr, it allows for safe mutation of an atomic /// into common Rust Types. /// -/// All `Ordering` is `AcqRel`. +/// All methods of this type perform [acquire-release synchronization][1] as +/// necessary to ensure that the referenced object can be accessed safely. +/// +/// [1]: https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering /// /// ``` /// // Create an empty atom @@ -88,7 +91,7 @@ where /// Returning the contents. If the contents was a `null` pointer the /// result will be `None`. pub fn take(&self) -> Option

{ - let old = self.inner.swap(ptr::null_mut(), Ordering::AcqRel); + let old = self.inner.swap(ptr::null_mut(), Ordering::Acquire); unsafe { Self::inner_from_raw(old) } } @@ -98,7 +101,7 @@ where /// the same value that you passed into this function pub fn set_if_none(&self, v: P) -> Option

{ let new = v.into_raw(); - let result = self.inner.compare_exchange(ptr::null_mut(), new, Ordering::AcqRel, Ordering::Acquire); + let result = self.inner.compare_exchange(ptr::null_mut(), new, Ordering::Release, Ordering::Relaxed); if result.is_err() { Some(unsafe { FromRawPtr::from_raw(new) }) } else { @@ -126,7 +129,7 @@ where let pcurrent = self.inner.load(Ordering::Acquire); let current = unsafe { Self::inner_from_raw(pcurrent) }; unsafe { ptr::write(next, current) }; - let result = self.inner.compare_exchange(pcurrent, raw, Ordering::AcqRel, Ordering::Acquire); + let result = self.inner.compare_exchange(pcurrent, raw, Ordering::Release, Ordering::Relaxed); match result { Ok(replaced_ptr) => return replaced_ptr.is_null(), _ => {} @@ -394,7 +397,7 @@ where impl AtomSetOnce> { /// If the Atom is set, get the value pub fn get_mut(&mut self) -> Option<&mut T> { - let ptr = self.inner.inner.load(Ordering::Acquire); + let ptr = *self.inner.inner.get_mut(); let val = unsafe { Atom::inner_from_raw(ptr) }; val.map(move |mut v: Box| { // This is safe since ptr cannot be changed once it is set