Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Acq/Rel ordering everywhere #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use Acq/Rel ordering everywhere #17

wants to merge 3 commits into from

Conversation

torkleyy
Copy link
Member

@torkleyy torkleyy commented Oct 8, 2021

Fixes #15

Changed all methods to use Acquire, Release or AcqRel, whichever is the strictest.

I had to remove the ordering parameters from all the methods, so it's a breaking change.

Copy link

@yvt yvt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but they are unnecessarily strong in many places. They are are harmless by themselves but can conceal synchronization bugs in user code.

Another possible option is to keep the ordering parameters but enforce a minimum ordering like atomic_ref 0.2.1.

examples/fifo.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@@ -244,7 +235,7 @@ where
P: IntoRawPtr + FromRawPtr,
{
fn drop(&mut self) {
self.take(Ordering::Relaxed);
self.take();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can use the Relaxed ordering or even a non-atomic access (*self.inner.get_mut())

src/lib.rs Outdated Show resolved Hide resolved
pub fn compare_and_swap(
&self,
current: Option<&P>,
new: Option<P>,
order: Ordering,
) -> Result<Option<P>, (Option<P>, *mut P)> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the change, but isn't this *mut P actually *mut <P as Deref>::Target? At least this is true for the IntoRawPtr implementations provided by this crate.

) -> Result<Option<P>, (Option<P>, *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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about the failure ordering.

) -> Result<Option<P>, (Option<P>, *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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about the failure ordering.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Apply suggestions from code review

Co-authored-by: yvt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsound: Atom accepts an unsafe memory ordering
2 participants