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 atomic usage on 32-bit systems #260

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

guillemj
Copy link

The atomic package requires aligned variables (as per the package documentation), but this is not happening for the variables on the stack. Instead change the types for these to 32-bit variables as there should be no need to track 64-bit stats.

This fixes various panics from the unaligned memory accesses, such as the one seen at https://ci.debian.net/data/autopkgtest/testing/i386/g/golang-github-allegro-bigcache/9212366/log.gz.

The atomic package requires aligned variables (as per the package
documentation), but this is not happening for the variables on the
stack. Instead change the types for these to 32-bit variables as
there should be no need to track 64-bit stats.

This fixes various panics from the unaligned memory accesses.
@janisz
Copy link
Collaborator

janisz commented Jan 13, 2021

Just coping the linked panic log just to keep it here

=== RUN   TestRequestMetrics
--- FAIL: TestRequestMetrics (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x804a64c]

goroutine 6 [running]:
testing.tRunner.func1.1(0x81fc4a0, 0x83711b0)
	/usr/lib/go-1.15/src/testing/testing.go:1072 +0x291
testing.tRunner.func1(0x9400ee0)
	/usr/lib/go-1.15/src/testing/testing.go:1075 +0x358
panic(0x81fc4a0, 0x83711b0)
	/usr/lib/go-1.15/src/runtime/panic.go:969 +0x15c
runtime/internal/atomic.Xadd64(0x15afde1c, 0x1, 0x0, 0x904fefb3, 0x838c580)
	/usr/lib/go-1.15/src/runtime/internal/atomic/asm_386.s:105 +0xc
github.com/allegro/bigcache.(*cacheShard).miss(...)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/shard.go:398
github.com/allegro/bigcache.(*cacheShard).getWrappedEntry(0x15afdd90, 0xd01cb2ae, 0x904fefb3, 0x1c, 0x50, 0x0, 0x0, 0x0)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/shard.go:88 +0x10a
github.com/allegro/bigcache.(*cacheShard).get(0x15afdd90, 0x822723b, 0x5, 0xd01cb2ae, 0x904fefb3, 0x80a8a97, 0x2006e4cc, 0x899d1, 0x3ae7c6cc, 0x3ae7c6cc)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/shard.go:64 +0x4d
github.com/allegro/bigcache.(*BigCache).Get(0x94b6180, 0x822723b, 0x5, 0x0, 0x3ae7c6cc, 0x2006e4cc, 0x899d1, 0x0)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/bigcache.go:117 +0x76
github.com/allegro/bigcache/server.getCacheHandler(0x8265b20, 0x1c3b6f00, 0x94b6280)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/server/cache_handlers.go:32 +0x5d
github.com/allegro/bigcache/server.cacheIndexHandler.func1(0x8265b20, 0x1c3b6f00, 0x94b6280)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/server/cache_handlers.go:14 +0x77
net/http.HandlerFunc.ServeHTTP(0x8237164, 0x8265b20, 0x1c3b6f00, 0x94b6280)
	/usr/lib/go-1.15/src/net/http/server.go:2042 +0x34
github.com/allegro/bigcache/server.requestMetrics.func1.1(0x8265b20, 0x1c3b6f00, 0x94b6280)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/server/middleware.go:25 +0x76
net/http.HandlerFunc.ServeHTTP(0x1d0780e0, 0x8265b20, 0x1c3b6f00, 0x94b6280)
	/usr/lib/go-1.15/src/net/http/server.go:2042 +0x34
github.com/allegro/bigcache/server.TestRequestMetrics(0x9400ee0)
	/tmp/autopkgtest-lxc.71tbek_l/downtmp/autopkgtest_tmp/_build/src/github.com/allegro/bigcache/server/middleware_test.go:41 +0x1fe
testing.tRunner(0x9400ee0, 0x823715c)
	/usr/lib/go-1.15/src/testing/testing.go:1123 +0xb7
created by testing.(*T).Run
	/usr/lib/go-1.15/src/testing/testing.go:1168 +0x211
FAIL	github.com/allegro/bigcache/server	0.120s
FAIL
dh_auto_test: error: cd _build && go test -vet=off -v -p 2 github.com/allegro/bigcache github.com/allegro/bigcache/queue github.com/allegro/bigcache/server returned exit code 1
make: *** [debian/rules:4: build] Error 25
autopkgtest [21:39:13]: test dh-golang-autopkgtest: -----------------------]
autopkgtest [21:39:13]: test dh-golang-autopkgtest:  - - - - - - - - - - results - - - - - - - - - -
dh-golang-autopkgtest FAIL non-zero exit status 2
autopkgtest [21:39:13]: @@@@@@@@@@@@@@@@@@@@ summary
dh-golang-autopkgtest FAIL non-zero exit status 2

@janisz
Copy link
Collaborator

janisz commented Jan 13, 2021

"package requires aligned variables (as per the package documentation)"
@guillemj Can you link to doc?

I'm not sure if going 32 bit is good solution. I understand this may solve the problem but it's limiting the statistics maybe we should consider mutex and locking? We can use channel to and additional gorutine to make this asyc to not slow down Read/Write operations.

@guillemj
Copy link
Author

"package requires aligned variables (as per the package documentation)"
@guillemj Can you link to doc?

https://golang.org/pkg/sync/atomic/#pkg-note-BUG

I'm not sure if going 32 bit is good solution. I understand this may solve the problem but it's limiting the statistics maybe we should consider mutex and locking? We can use channel to and additional gorutine to make this asyc to not slow down Read/Write operations.

I pondered about the reduced tracking, but then questioned whether the code would be reaching 2147483648 hits/misses?

Bornholm pushed a commit to Bornholm/bigcache that referenced this pull request Dec 1, 2023
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