Relax SeqCst ordering in standard library.

[m-ou-se]

Every single SeqCst in the standard library is unnecessary. In all cases, Relaxed or Release+Acquire was sufficient.

As I wrote in my book on atomics:

[..] when reading code, SeqCst basically tells the reader: "this operation depends on the total order of every single SeqCst operation in the program," which is an incredibly far-reaching claim. The same code would likely be easier to review and verify if it used weaker memory ordering instead, if possible. For example, Release effectively tells the reader: "this relates to an acquire operation on the same variable," which involves far fewer considerations when forming an understanding of the code.

It is advisable to see SeqCst as a warning sign. Seeing it in the wild often means that either something complicated is going on, or simply that the author did not take the time to analyze their memory ordering related assumptions, both of which are reasons for extra scrutiny.

r? @Amanieu @joboet

[Amanieu]

Could we run the testsuite on miri just in case something was missed? It's easy to make mistakes with insufficient memory orderings.

Other than that, I strongly agree with the premise that SeqCst is an anti-pattern (except for fence(SeqCst) which has valid use cases).

[m-ou-se]

@RalfJung how do we run Miri on the std test suite in this PR?

[RalfJung]

You can use rustup-toolchain-install-master to install the toolchain for the commit this PR is based on, adding all the components required for Miri. (Something like -c cargo -c rust-src -c miri should work.)

Then check out https://github.com/rust-lang/miri-test-libstd/, in there add an override for the above toolchain, and run something like

MIRI_LIB_SRC=<path to rustc checkout with this PR>/library ./run-test.sh alloc -- sync

The first argument is the crate to test. The rest (-- sync) is passed to cargo test (so if you have only a single filter the -- is not needed).

The std crate has some areas that don't work with Miri so a bunch of filtering will be needed. See here for the filtering we do in our daily runs.

---

It is advisable to see SeqCst as a warning sign. Seeing it in the wild often means that either something complicated is going on, or simply that the author did not take the time to analyze their memory ordering related assumptions, both of which are reasons for extra scrutiny.

I agree with that. However, I also would say the same about "Relaxed". These accesses are incredibly poorly behaved. (See llvm/llvm-project#64188 for a recent example of trouble they are causing for standard compiler optimizations.) IMO one should always use the release/acquire modes unless there is a strong reason to use anything else, and any deviation from this requires a justifying comment.

So I'm not happy to see a proliferation of "Relaxed" in this PR, in particular in docs.

[joboet]

Looks good to me, modulo the comment!

[m-ou-se]

I also would say the same about "Relaxed". These accesses are incredibly poorly behaved. (See llvm/llvm-project#64188 for a recent example of trouble they are causing for standard compiler optimizations.) IMO one should always use the release/acquire modes unless there is a strong reason to use anything else, and any deviation from this requires a justifying comment.

That example you link is not just about a single atomic variable with relaxed operations, it's about an interaction of pointers+acquire-release+relaxed (and some variable whose address hasn't escaped/been exposed at the time of the release operation). Such a complex situation has very little to do with the simple cases we have in the standard library.

This PR is about very simple patterns, such as using fetch_add(1) for generating unique integers (ignoring overflow). Relaxed is perfectly fine for these.

[RalfJung]

@m-ou-se btw, given your stance on SeqCst (which I agree with), I wonder if you'd be open to reviving rust-lang/rfcs#2503 in one shape or another? IOW, providing some way to say "give me release and/or acquire, whatever fits for this operation", without the risk of accidentally using Acquire/Release for the success case of an RMW operation (which subtly introduces relaxed semantics)?

[m-ou-se]

@RalfJung I've seen a lot of cases where people want (or think they want ^^') an "acquire store" or "release load" (which don't exist). With something like Ordering::AutoAcqOrRel, it becomes a lot less clear which operations are acquire vs which ones are release. So I don't think it's a good idea to hide that difference.

[RalfJung]

Hm, yes that is fair. I think I am mostly worried about accidentally using Acquire/Release as the success ordering of an RMW, which introduces Relaxed semantics without Relaxed appearing in the code.

[m-ou-se]

I imagine we could have fetch_add(1, Ordering::AcquireRelaxed), with a lint to discourage Ordering::Acquire.. But AcquireRelaxed sounds almost like AcqRel. 😬 Anyway, let's move this discussion to Zulip or something. ^^'

Edit: Zulip: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Explicit.20Relaxed.20ordering.20in.20RMW.20operations/near/428083271