search

matklad / once_cell

Rust library for single assignment cells and lazy statics without macros
Apache License 2.0
1.87k stars 109 forks source link

Potential to weaken ordering for success case of `compare_exhange`s in `race` module #220

Open Imberflur opened 1 year ago

Imberflur commented 1 year ago

For example, the compare exchange for OnceNonZeroUsize uses AcqRel for the success case: https://github.com/matklad/once_cell/blob/d706539c6f7cd47d0e8037d832c0c95214fb5f91/src/race.rs#L100

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 commented 1 year ago

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

taiki-e commented 1 year ago

Note that CAS failure ordering that is stronger than success ordering requires Rust 1.64+: https://github.com/rust-lang/rust/pull/98383

Imberflur commented 1 year ago

Note that CAS failure ordering that is stronger than success ordering requires Rust 1.64+: https://github.com/rust-lang/rust/pull/98383

Gah! :slightly_frowning_face:

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.