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

runtime: spurious cycles in time.Now pprof output (vdsoPC and vdsoSP not WAI?) #47324

Closed
josharian opened this issue Jul 21, 2021 · 11 comments
Closed

runtime: spurious cycles in time.Now pprof output (vdsoPC and vdsoSP not WAI?) #47324

josharian opened this issue Jul 21, 2021 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@josharian
Copy link
Contributor

On linux:

go test -bench=kNow$ -run=NONE -count=3 -cpuprofile=c.p time
go tool pprof -pdf -lines c.p > o.pdf

Result:

o.pdf

Observe the cycles in the graph. The actual call graph doesn't have any cycles in it.
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 23, 2021
@thanm thanm added this to the Backlog milestone Jul 23, 2021
@thanm
Copy link
Contributor

thanm commented Jul 23, 2021

@cherrymui per owners (I am assuming that this is a runtime/pprof problem).

@cherrymui
Copy link
Member

I assume this is Linux/ARM64? (from sys_linux_arm64.s in the profile)
What version of Go is this? I assume this is not tip, where walltime1 is gone.

It appears to me in sys_linux_arm64.s we probably store the wrong value into m.vdsoSP. I think we should store the caller's SP instead of its own SP.

I'll work on a fix. (But would still be good to know the information above. Thanks.)

@josharian
Copy link
Contributor Author

Sorry, yes, linux/arm64. I’ve also seen it on linux/amd64, and it wouldn’t surprise me if it were also present on other platforms. The reproduction above was with 1.16, I believe. (I’m AFK.)

@cherrymui
Copy link
Member

Does CL https://go-review.googlesource.com/c/go/+/337590 help? (It's on tip.) Thanks.

@gopherbot
Copy link

Change https://golang.org/cl/337590 mentions this issue: runtime: set m.vdsoSP correctly on Linux/ARM64

@josharian
Copy link
Contributor Author

Does CL https://go-review.googlesource.com/c/go/+/337590 help? (It's on tip.) Thanks.

Sure does!

@cherrymui
Copy link
Member

Thanks. I'll make it a real CL then.

@mengzhuo
Copy link
Contributor

@cherrymui should we fix other architectures in CL 337590 or submit separate CLs for this issue?

@cherrymui
Copy link
Member

I'll probably do all of them in one CL.

@gopherbot
Copy link

Change https://golang.org/cl/380715 mentions this issue: [release-branch.go1.17] runtime: set vdsoSP to caller's SP consistently

@gopherbot
Copy link

Change https://golang.org/cl/380716 mentions this issue: [release-branch.go1.16] runtime: set vdsoSP to caller's SP consistently

gopherbot pushed a commit that referenced this issue Feb 7, 2022
@cherrymui
[release-branch.go1.16] runtime: set vdsoSP to caller's SP consistently
e2277c8 
m.vdsoSP should be set to the SP of the caller of nanotime1,
instead of the SP of nanotime1 itself, which matches m.vdsoPC.
Otherwise the unmatched vdsoPC and vdsoSP would make the stack
trace look like recursive.

We already do it correctly on AMD64, 386, and RISCV64. This CL
fixes the rest.

Also incorporate CL 352509, skipping a flaky test.

Updates #47324, #50772.
Fixes #50780.

Change-Id: I98b6fcfbe9fc6bdd28b8fe2a1299b7c505371dd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/337590
Trust: Cherry Mui <cherryyz@google.com>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
(cherry picked from commit 217507e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/380716
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
gopherbot pushed a commit that referenced this issue Feb 7, 2022
@cherrymui
[release-branch.go1.17] runtime: set vdsoSP to caller's SP consistently
1952c65 
m.vdsoSP should be set to the SP of the caller of nanotime1,
instead of the SP of nanotime1 itself, which matches m.vdsoPC.
Otherwise the unmatched vdsoPC and vdsoSP would make the stack
trace look like recursive.

We already do it correctly on AMD64, 386, and RISCV64. This CL
fixes the rest.

Also incorporate CL 352509, skipping a flaky test.

Updates #47324, #50772.
Fixes #50781.

Change-Id: I98b6fcfbe9fc6bdd28b8fe2a1299b7c505371dd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/337590
Trust: Cherry Mui <cherryyz@google.com>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
(cherry picked from commit 217507e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/380715
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@golang golang locked and limited conversation to collaborators Jan 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@josharian @mengzhuo @gopherbot @thanm @cherrymui