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: lock cycle between malloc and execution tracer #53979

Open
aclements opened this issue Jul 21, 2022 · 5 comments
Open

runtime: lock cycle between malloc and execution tracer #53979

aclements opened this issue Jul 21, 2022 · 5 comments
Assignees
Labels
compiler/runtime NeedsFix
Milestone

Comments

@aclements
Copy link
Member

@aclements aclements commented Jul 21, 2022

What version of Go are you using (go version)?

Current HEAD (2aa473c)

Does this issue reproduce with the latest release?

Yes, though via a different code path.

What did you do?

If the execution tracer is enabled, there's a potential though rare deadlock via a rank cycle on mheap_.lock and trace.lock:

A: setGCPercent or setMemoryLimit acquire mheap_.lock -> gcControllerCommit -> traceHeapGoal -> traceEvent -> traceEventLocked -> traceFlush -> acquires trace.lock
B: traceFlush acquires trace.lock -> triggers stack growth -> stack allocator calls mheap.allocManual -> mheap.allocSpan -> acquires mheap_.lock

Path "A" violates the current lock ranking. I discovered this when I added a "may acquire" annotation on traceEvent. But I think path "B" may be the real problem. Because stack growth can happen while holding trace.lock, it's pretty high in the ranking (has a low rank value). But this means that anything that holds any locks further down in the ranking, like the memory allocator, can't safely create trace events.

I wonder if, like mheap.lock, we should say that trace.lock can only be acquired on the system stack so stack growth can never happen. I think that would push tracing down to the leaves of the rank graph, rather than it being smack in the middle.

/cc @mknyszek @golang/runtime

@aclements aclements added the NeedsFix label Jul 21, 2022
@aclements aclements added this to the Go1.20 milestone Jul 21, 2022
@gopherbot gopherbot added the compiler/runtime label Jul 21, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418716 mentions this issue: runtime: add missing trace lock edges

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418720 mentions this issue: runtime: add mayAcquire annotation for trace.lock

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418957 mentions this issue: runtime: move trace locks to the leaf of the lock graph

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418955 mentions this issue: runtime: don't use trace.lock for trace reader parking

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418956 mentions this issue: runtime: only acquire trace.lock on the system stack

gopherbot pushed a commit that referenced this issue Aug 4, 2022
We're missing lock edges to trace.lock that happen only rarely. Any
trace event can potentially fill up a trace buffer and acquire
trace.lock in order to flush the buffer, but this happens relatively
rarely, so we simply haven't seen some of these lock edges that could
happen.

With this change, we promote "fin, notifyList < traceStackTab" to
"fin, notifyList < trace" and now everything that emits trace events
with a P enters the tracer lock ranks via "trace", rather than some
things entering at "trace" and others at "traceStackTab".

This was found by inspecting the rank graph for things that didn't
make sense.

Ideally we would add a mayAcquire annotation that any trace event can
potentially acquire trace.lock, but there are actually cases that
violate this ranking right now. This is #53979. The chance of a lock
cycle is extremely low given the number of conditions that have to
happen simultaneously.

For #53789.

Change-Id: Ic65947d27dee88d2daf639b21b2c9d37552f0ac0
Reviewed-on: https://go-review.googlesource.com/c/go/+/418716
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime NeedsFix
Projects
Status: In Progress
Development

No branches or pull requests

2 participants