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

Potential to weaken ordering for success case of compare_exhanges in race module #220

Open
Imberflur opened this issue Feb 14, 2023 · 3 comments

Comments

@Imberflur
Copy link
Contributor

For example, the compare exchange for OnceNonZeroUsize uses AcqRel for the success case:

self.inner.compare_exchange(0, val, Ordering::AcqRel, Ordering::Acquire);

The Acquire portion of the AcqRel here is for the load of 0, however there is no Release store of 0 so this will not synchronize with anything. Note that the construction of OnceNonZeroUsize still has a happens-before relationship to this since we have an &self reference. Thus, I think a Release ordering is sufficient here for the desired synchronization to be achieved (including taking this documentation into account):

 self.inner.compare_exchange(0, val, Ordering::Release, Ordering::Acquire);

I believe this applies to all uses of compare_exchange in this module.


(happy to make a PR if this looks reasonable)

@matklad
Copy link
Owner

matklad commented Feb 14, 2023

This does look reasonable to me, yeah! Although, as usual, I am unnerved by the fact that we don't have any tools to check this (or maybe I am missing them?).

Could you send a PR? You can also adjust the Changelog and bump the patch version in Cargo.toml, so that we release this together with #219

Imberflur added a commit to Imberflur/once_cell that referenced this issue Feb 14, 2023
Since the load during success for compare exchange is `0`/`null` and no
`Release` store of this value exists, using an `Aquire` load for this
case adds no additional synchronization.

Fixes matklad#220
@taiki-e
Copy link

taiki-e commented Feb 14, 2023

Note that CAS failure ordering that is stronger than success ordering requires Rust 1.64+: rust-lang/rust#98383

@Imberflur
Copy link
Contributor Author

Note that CAS failure ordering that is stronger than success ordering requires Rust 1.64+: rust-lang/rust#98383

Gah! 🙁

Although, as usual, I am unnerved by the fact that we don't have any tools to check this (or maybe I am missing them?).

The only thing that comes to my mind is testing with loom. I don't know if it is exactly what you are looking for though.

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

No branches or pull requests

3 participants