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

compiler_fence SeqCst docs are incorrect #54962

Open
parched opened this issue Oct 10, 2018 · 5 comments
Open

compiler_fence SeqCst docs are incorrect #54962

parched opened this issue Oct 10, 2018 · 5 comments
Labels
A-concurrency Area: Concurrency related issues. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@parched
Copy link
Contributor

parched commented Oct 10, 2018

The documentation for compiler_fence with SeqCst say "no re-ordering of reads and writes across this point is allowed". As I understand it, SeqCst for a single thread doesn't give anything more than AcqRel. E.g., a store before the fence could be moved past a load after then fence. Is that correct?

cc @jonhoo @RalfJung

@jonhoo
Copy link
Contributor

jonhoo commented Oct 10, 2018

Hmm, that's a good point, though the details are a little subtle. From the C++ reference:

memory_order_seq_cst: A load operation with this memory order performs an acquire operation, a store performs a release operation.

memory_order_acquire: A load operation with this memory order performs the acquire operation on the affected memory location: no reads or writes in the current thread can be reordered before this load.

memory_order_release: A store operation with this memory order performs the release operation: no reads or writes in the current thread can be reordered after this store.

I think what this translates into is that SeqCst guarantees that: no subsequent operations can be moved before this operation if it is a load, and no operations can be moved after this operation if it is a store. The italic parts there aren't present in the current documentation, so that should probably be rectified. I think some annotated source code would really help here!

@estebank estebank added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Oct 10, 2018
@RalfJung
Copy link
Member

It actually depends on the other operation, does it not? Release stores after the fence can be moved up, but SeqCst stores cannot.

TBH I have a hard time thinking in terms of reorderings; I usually think in terms of what actually happens in the underlying memory model. Some of these models can be fully characterized by the allowed reorderings, but not all of them can (as far as we know). And anyway, SC fences are broken in the C11 memory model and probably going to change in the next version, so...^^

memory_order_seq_cst: A load operation with this memory order performs an acquire operation, a store performs a release operation.

On top of that, all SeqCst are in a total order, and that is the entire point of SeqCst. (The website says it but you left it away in your quote. Just making sure nobody thinks that a SeqCst load is the same as an Acquire load. It is not.)

@jonhoo
Copy link
Contributor

jonhoo commented Oct 12, 2018

@RalfJung I'm actually not entirely sure about that. Another store to the same memory location shouldn't be ordered before, but I don't see from the reference text why an unrelated store with SeqCst couldn't be.

I'd be happy for this to be rephrased this in terms of the memory model, but I'm not entirely sure how we'd even do that. Proposals welcome. I think talking about what can and cannot happen, with examples, is probably the most understandable way to present what the different orderings do.

I left off the total order part because it didn't seem directly related to the original question. But you're right that it's a vital part of SeqCst that shouldn't get lost in the docs!

@RalfJung
Copy link
Member

RalfJung commented Oct 13, 2018

I don't see from the reference text why an unrelated store with SeqCst couldn't be.

It says

a single total order exists in which all threads observe all modifications in the same order (see Sequentially-consistent ordering below)

The sequentially-consistent ordering consists of all SeqCst operation, across all locations, and the order in which they are observed must be consistent across all threads. In particular, if one thread performs two SeqCst operations (i.e., they are ordered by program-order), then the order in which it does them must match the order in which every other thread observes them. That rules out reordering them.

This is the sole justification for the existence of SeqCst.

@parched
Copy link
Contributor Author

parched commented Oct 14, 2018

It actually depends on the other operation, does it not?

Yes, good point. So how about we move the SeqCst bullet point to bottom and change it to say

with SeqCst, all of the above rules are enforced and no re-ordering of any SeqCst reads or writes across this point are allowed.

@steveklabnik steveklabnik added the P-medium Medium priority label Dec 27, 2018
@jonas-schievink jonas-schievink added A-concurrency Area: Concurrency related issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency related issues. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants