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

Fix alignment bug on 32-bit platforms in new rackAwareRR code #1666

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

satta
Copy link
Contributor

@satta satta commented Dec 12, 2022

When updating Debian packaging to 1.3.0, we noticed that tests were failing on 32-bit systems e.g. on 32-bit ARM [1] or i386 [2]:

=== RUN   TestHostPolicy_RackAwareRR
--- FAIL: TestHostPolicy_RackAwareRR (0.00s)
panic: unaligned 64-bit atomic operation [recovered]
	panic: unaligned 64-bit atomic operation

goroutine 96 [running]:
testing.tRunner.func1.2({0x27b5b8, 0x30a408})
	/usr/lib/go-1.19/src/testing/testing.go:1396 +0x27c
testing.tRunner.func1()
	/usr/lib/go-1.19/src/testing/testing.go:1399 +0x3f4
panic({0x27b5b8, 0x30a408})
	/usr/lib/go-1.19/src/runtime/panic.go:884 +0x23c
runtime/internal/atomic.panicUnaligned()
	/usr/lib/go-1.19/src/runtime/internal/atomic/unaligned.go:8 +0x24
runtime/internal/atomic.Xadd64(0x181267c, 0x1)
	/usr/lib/go-1.19/src/runtime/internal/atomic/atomic_arm.s:258 +0x14
github.com/gocql/gocql.(*rackAwareRR).Pick(0x1812660, {0x0, 0x0})
	/tmp/autopkgtest-lxc.7ok3wgk4/downtmp/autopkgtest_tmp/obj-arm-linux-gnueabihf/src/github.com/gocql/gocql/policies.go:956 +0x34
github.com/gocql/gocql.TestHostPolicy_RackAwareRR(0x11a052c0)
	/tmp/autopkgtest-lxc.7ok3wgk4/downtmp/autopkgtest_tmp/obj-arm-linux-gnueabihf/src/github.com/gocql/gocql/policies_test.go:682 +0x64c
testing.tRunner(0x11a052c0, 0x2c02f0)
	/usr/lib/go-1.19/src/testing/testing.go:1446 +0x118
created by testing.(*T).Run
	/usr/lib/go-1.19/src/testing/testing.go:1493 +0x3a0
FAIL	github.com/gocql/gocql	1.787s

Further investigation on affected platforms showed that this issue was related to a Go problem regarding the use of atomic.*64* functions [3]:

On ARM, 386, and 32-bit MIPS, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically via the primitive atomic functions (types Int64 and Uint64 are automatically aligned). The first word in an allocated struct, array, or slice; in a global variable; or in a local variable (because the subject of all atomic operations will escape to the heap) can be relied upon to be 64-bit aligned.

This problem surfaces in https://github.com/gocql/gocql/blob/fe55d80b186e16ef11ea79c0cd98065d85886cbe/policies.go#L956 where the last item in a struct is accessed, triggering the unaligned access.

An obvious workaround is moving lastUsedHostIdx to the beginning of the rackAwareRR struct (also see [4]), which indeed solves the problem for me. Tests complete on such these platforms now as well. Hence I am proposing this PR as a low-impact solution for that issue.

My email address is already in the AUTHORS file as I apparently have contributed before :)

[1] https://ci.debian.net/data/autopkgtest/testing/armhf/g/golang-github-gocql-gocql/29281221/log.gz
[2] https://ci.debian.net/data/autopkgtest/testing/i386/g/golang-github-gocql-gocql/29271590/log.gz
[3] https://pkg.go.dev/sync/atomic#pkg-note-BUG
[4] golang/go#11891 (comment)

Copy link
Contributor

@martin-sucha martin-sucha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the report!

Could you please rebase the commit and add the information from the pull request description to the commit message? The description is nicely written, it would be great to also include it in the commit message so that it's easier to find in IDEs etcetera.

policies.go Show resolved Hide resolved
On 32-bit platforms, use of atomic.* 64-bit functions needs to be
careful to ensure 64-bit alignment. This was a problem in the
rackAwareRR struct, causing panics on 32-bit platforms.
https://pkg.go.dev/sync/atomic#pkg-note-BUG notes that the first word in
a struct is always 64-bit aligned, to moving the struct member that
causes the issue to the beginning of the struct solves the problem.
See apache#1666.
@satta
Copy link
Contributor Author

satta commented Dec 13, 2022

Thanks for the quick reply. Just added the requested information.

@martin-sucha martin-sucha merged commit 8b1acc3 into apache:master Dec 13, 2022
@satta satta deleted the 32bit-align branch December 13, 2022 12:51
@martin-sucha
Copy link
Contributor

Thanks! I've released this in 1.3.1 🎉

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

Successfully merging this pull request may close these issues.

2 participants