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

runtime: total allocation stats are managed in a uintptr which can quickly wrap around on 32-bit architectures [1.18 backport] #52689

Closed
gopherbot opened this issue May 3, 2022 · 10 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@gopherbot
Copy link
Contributor

@mknyszek requested issue #52680 to be considered for backport to the next 1.18 minor release.

@gopherbot Please open backport issues for Go 1.17 and Go 1.18.

@mknyszek
Copy link
Contributor

mknyszek commented May 3, 2022

Basically, on 32-bit platforms, allocation stats can end up completely wrong. I'm surprised this flew under the radar for so long. There's no workaround.

@dr2chase dr2chase added the CherryPickApproved Used during the release process for point releases label May 4, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label May 4, 2022
@dr2chase
Copy link
Contributor

dr2chase commented May 5, 2022

Ahoy, can you do the backport CLs here? (for this and 1.17)
It may require a little cleanup work, I tried and had merge errors.
See https://github.com/golang/go/wiki/MinorReleases

@mknyszek
Copy link
Contributor

mknyszek commented May 5, 2022

Thanks for trying David. Yeah, it's not going to be that easy because a bunch of things moved. I suspect 1.17 and 1.18 can use the same fix, but I need to basically create a separate CL for them. It'll be very similar to what landed for 1.19, but not quite the same.

@dr2chase
Copy link
Contributor

dr2chase commented May 5, 2022

Also not allowed to mail the cherry-pick if I do it by hand, oh well.
Some other time.

@heschi heschi modified the milestones: Go1.18.2, Go1.18.3 May 10, 2022
@toothrot
Copy link
Contributor

Just checking in. The next minor release is coming up soon, and this issue has already been bumped once. Is there any new progress?

@dmitshur dmitshur modified the milestones: Go1.18.3, Go1.18.4 Jun 1, 2022
@hallgren
Copy link

hallgren commented Jun 9, 2022

We bump into this issue today where the value of MemStats.Alloc was a really high value. The reason as we understands it is that totalAlloc has rotated but not the totalFree in the updatememstats() function.

memstats.alloc = totalAlloc - totalFree

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/411495 mentions this issue: [release-branch.go1.18] runtime: store consistent total allocation stats as uint64

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/411494 mentions this issue: [release-branch.go1.18] runtime: store consistent total allocation stats as uint64

@mknyszek
Copy link
Contributor

mknyszek commented Jun 9, 2022

I created the cherry-picks, and the changes are straightforward, so this should end up in the next minor release. Sorry for the delay, got caught up working on stuff for the current release.

@gopherbot
Copy link
Contributor Author

Closed by merging f0eca66 to release-branch.go1.18.

gopherbot pushed a commit that referenced this issue Jul 6, 2022
…ats as uint64

Currently the consistent total allocation stats are managed as uintptrs,
which means they can easily overflow on 32-bit systems. Fix this by
storing these stats as uint64s. This will cause some minor performance
degradation on 32-bit systems, but there really isn't a way around this,
and it affects the correctness of the metrics we export.

For #52680.
Fixes #52689.

Change-Id: I8b1926116e899ae9f03d58e0320bcb9264945b3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/411495
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 14, 2022
…ats as uint64

Currently the consistent total allocation stats are managed as uintptrs,
which means they can easily overflow on 32-bit systems. Fix this by
storing these stats as uint64s. This will cause some minor performance
degradation on 32-bit systems, but there really isn't a way around this,
and it affects the correctness of the metrics we export.

For golang#52680.
Fixes golang#52689.

Change-Id: I8b1926116e899ae9f03d58e0320bcb9264945b3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/411495
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
@golang golang locked and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

7 participants