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

Dangerously misleading introduction to std::sync::atomic::Ordering #55196

Closed
vorner opened this issue Oct 19, 2018 · 3 comments
Closed

Dangerously misleading introduction to std::sync::atomic::Ordering #55196

vorner opened this issue Oct 19, 2018 · 3 comments

Comments

@vorner
Copy link
Contributor

vorner commented Oct 19, 2018

When looking at https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html, this is in the intro:

At its most restrictive, "sequentially consistent" atomics allow neither reads nor writes to be moved either before or after the atomic operation;

This is not true (even the explanations for variants disprove this, but if someone reads only the intro, they could get to the conclusion that using SeqCst is good enough everywhere, which it actually isn't).

Eg, from the docs below:

SeqCst: Like AcqRel with the additional guarantee that all threads see all sequentially consistent operations in the same order.

AcqRel: For loads it uses Acquire ordering. For stores it uses the Release ordering.

So, atomic.load(Ordering::SeqCst) has Acquire semantics (+ participating in the global timeline of all SeqCst operations), which means reads and writes are allowed to be reordered after the operation. And even failed CAS operation is effectively just a read, so operations are allowed to be reordered after the operation.

I have the vague memory someone was updating it recently to actually point out these gotchas, but I can't find that now.

I can try rewording it, but I'm not really that good at writing good concise documentation.

@nagisa
Copy link
Member

nagisa commented Oct 19, 2018

See also #54962

@ehuss
Copy link
Contributor

ehuss commented Oct 20, 2018

The nightly docs has some updated wording from #53106.

@vorner
Copy link
Contributor Author

vorner commented Oct 20, 2018

It has some updated wording, but in the details below. But the intro wasn't changed there and it is still misleading. And if someone reads „SeqCst is allmighty“, they might just stop there and not read the rest.

But yes, that was the one I was referring to when I wrote about the vague memory.

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 28, 2018
…stjepang

atomic::Ordering: Get rid of misleading parts of intro

Remove the parts of atomic::Ordering's intro that wrongly claimed that
SeqCst prevents all reorderings around it.

Closes rust-lang#55196

This is a (minimal) alternative to rust-lang#55233.

I also wonder if it would be worth adding at least some warnings that atomics are often a footgun/hard to use correctly, similarly like `mem::transmute` or other functions have.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 29, 2018
…stjepang

atomic::Ordering: Get rid of misleading parts of intro

Remove the parts of atomic::Ordering's intro that wrongly claimed that
SeqCst prevents all reorderings around it.

Closes rust-lang#55196

This is a (minimal) alternative to rust-lang#55233.

I also wonder if it would be worth adding at least some warnings that atomics are often a footgun/hard to use correctly, similarly like `mem::transmute` or other functions have.
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