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.

Sign up for GitHub

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

Jump to bottom

sync/atomic: make load and store fully sequentially consistent on s390x #32428

Closed
mundaym opened this issue Jun 4, 2019 · 2 comments
Closed

sync/atomic: make load and store fully sequentially consistent on s390x #32428

mundaym opened this issue Jun 4, 2019 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.13

Comments

@mundaym
Copy link
Member

mundaym commented Jun 4, 2019

As discussed in https://groups.google.com/forum/#!topic/golang-dev/KKfRLcTeb0E loads and stores are not fully sequentially consistent on s390x since loads may be moved before stores if they access independent memory locations. This is similar to the memory model used by amd64.

We can force full sequential consistency of memory accesses by using fast serialization instructions (SYNC in Go asm). From discussion in #5045 it seems it is probably worth adding serialization between loads and stores in the sync/atomic package at least.

As far as I can tell the runtime doesn't currently require full sequential consistency, so I'm inclined to leave the atomic implementations as they are until a requirement to change them arises. (Does this sound reasonable @aclements?)
@mundaym mundaym added this to the Go1.13 milestone Jun 4, 2019
@mundaym mundaym added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 4, 2019
@mundaym mundaym self-assigned this Jun 4, 2019
@aclements
Copy link
Member

As far as I can tell the runtime doesn't currently require full sequential consistency

I don't know that the runtime requires sequential consistency, but we've always assumed that atomics are sequentially consistent, so I strongly suspect there are parts that do require sequential consistency. We explicitly use atomic.LoadAcq/atomic.StoreRel in the runtime where we've considered it worth the engineering and maintenance effort to reason about using weaker atomics. On a TSO-like model, those can just be regular loads and stores (with the appropriate compiler barriers).

But this isn't just about the runtime. Go atomics on all other platforms are sequentially consistent. That may be not be documented, but there is certainly code out there that relies on that.

So, we do need to fix this.

@gopherbot
Copy link

Change https://golang.org/cl/180439 mentions this issue: cmd/compile, runtime: make atomic loads/stores sequentially consistent on s390x

@zhiqiangxu zhiqiangxu mentioned this issue Dec 14, 2019
Closed
@golang golang locked and limited conversation to collaborators Jun 5, 2020
@rsc rsc unassigned mundaym Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
3 participants
@aclements @gopherbot @mundaym