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: "synchronizes-before" ordering is broken on Load/Store. #67203

Closed
haruyama480 opened this issue May 6, 2024 · 7 comments
Closed

sync/atomic: "synchronizes-before" ordering is broken on Load/Store. #67203

haruyama480 opened this issue May 6, 2024 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.

Comments

@haruyama480
Copy link
Contributor

haruyama480 commented May 6, 2024 •
edited
Loading

Go version

go version go1.22.2 darwin/arm64

Output of go env in your module/workspace:

% go env
GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xxx/Library/Caches/go-build'
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xxx/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/xxx/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/xxx/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.2.darwin-arm64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/xxx/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.2.darwin-arm64/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.2'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/xxx/ghq/github.com/haruyama480/go-litmus-test/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/7c/vn1g1x4n4nqclvb5dnjkf8q80000gn/T/go-build1289181956=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

go func() {
  atomic.StoreInt32(&x, 1) // W1
  atomic.StoreInt32(&y, 1) // W2
}

r1 := atomic.LoadInt32(&x) // R1
r2 := atomic.LoadInt32(&y) // R2

sample

What did you see happen?

r1 == 0 && r2 == 1 was observed.

https://pkg.go.dev/sync/atomic says,

In the terminology of the Go memory model, if the effect of an atomic operation A is observed by atomic operation B, then A “synchronizes before” B. Additionally, all the atomic operations executed in a program behave as though executed in some sequentially consistent order.

but, it seems to be false.

What did you expect to see?

If W1 synchronized before W2, r1 == 0 && r2 == 1 should not be observed.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 6, 2024
@haruyama480
Copy link
Contributor Author

sync/atomic uses AArch64's STLR* instructions.

go/src/internal/runtime/atomic/atomic_arm64.s

Lines 105 to 121 in ac17440

TEXT ·Store(SB), NOSPLIT, $0-12
MOVD ptr+0(FP), R0
MOVW val+8(FP), R1
STLRW R1, (R0)
RET
TEXT ·Store8(SB), NOSPLIT, $0-9
MOVD ptr+0(FP), R0
MOVB val+8(FP), R1
STLRB R1, (R0)
RET
TEXT ·Store64(SB), NOSPLIT, $0-16
MOVD ptr+0(FP), R0
MOVD val+8(FP), R1
STLR R1, (R0)
RET

But, arm's hardware memory model allows memory accesses to different addresses to be "Re-order"ed.
https://developer.arm.com/documentation/102376/0100/Normal-memory

@seankhliao
Copy link
Member

r1 == 0 && r2 == 1

isn't this simply: W1, (the two store ops), W2

@haruyama480
Copy link
Contributor Author

W1, (the two store ops), W2

Sorry, I don't understand. Could you give me some details?

@haruyama480
Copy link
Contributor Author

(I added "R1" and "R2" for convention)

@haruyama480
Copy link
Contributor Author

Sorry, I get it.

You meant R1, W1, W2, R2.

@haruyama480
Copy link
Contributor Author

I fixed my code, and confirmed the test works.

-                       r1 := atomic.LoadInt32(&x)
-                       r2 := atomic.LoadInt32(&y)
-                       if r1 == 0 && r2 == 1 {
+                       r1 := atomic.LoadInt32(&y)
+                       r2 := atomic.LoadInt32(&x)
+                       if r1 == 1 && r2 == 0 {

Thank you for comments!

(Though I'm curious there are correct ordering with arm's STLR/LDAR instructions, I will study it. )

@haruyama480
Copy link
Contributor Author

In my macOS, TSO mode is enabled. So, If I turn it off or use another arm, fixed test may fail possibly.

% sysctl net.inet.tcp.tso
net.inet.tcp.tso: 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gopherbot @seankhliao @haruyama480