Skip to content

Commit

Permalink
[release-branch.go1.23] runtime: uphold goroutine profile invariants …
Browse files Browse the repository at this point in the history
…in coroswitch

Goroutine profiles require checking in with the profiler before any
goroutine starts running. coroswitch is a place where a goroutine may
start running, but where we do not check in with the profiler, which
leads to crashes. Fix this by checking in with the profiler the same way
execute does.

For #69998.
Fixes #70001.

Change-Id: Idef6dd31b70a73dd1c967b56c307c7a46a26ba73
Reviewed-on: https://go-review.googlesource.com/c/go/+/622016
Reviewed-by: David Chase <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
(cherry picked from commit 2a98a18)
Reviewed-on: https://go-review.googlesource.com/c/go/+/622375
Reviewed-by: Michael Pratt <[email protected]>
  • Loading branch information
mknyszek authored and prattmic committed Oct 30, 2024
1 parent 58babf6 commit cfe0ae0
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 0 deletions.
12 changes: 12 additions & 0 deletions src/runtime/coro.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,18 @@ func coroswitch_m(gp *g) {
// directly if possible.
setGNoWB(&mp.curg, gnext)
setMNoWB(&gnext.m, mp)

// Synchronize with any out-standing goroutine profile. We're about to start
// executing, and an invariant of the profiler is that we tryRecordGoroutineProfile
// whenever a goroutine is about to start running.
//
// N.B. We must do this before transitioning to _Grunning but after installing gnext
// in curg, so that we have a valid curg for allocation (tryRecordGoroutineProfile
// may allocate).
if goroutineProfile.active {
tryRecordGoroutineProfile(gnext, nil, osyield)
}

if !gnext.atomicstatus.CompareAndSwap(_Gwaiting, _Grunning) {
// The CAS failed: use casgstatus, which will take care of
// coordinating with the garbage collector about the state change.
Expand Down
45 changes: 45 additions & 0 deletions src/runtime/pprof/pprof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"internal/syscall/unix"
"internal/testenv"
"io"
"iter"
"math"
"math/big"
"os"
Expand Down Expand Up @@ -1754,6 +1755,50 @@ func TestGoroutineProfileConcurrency(t *testing.T) {
}
}

// Regression test for #69998.
func TestGoroutineProfileCoro(t *testing.T) {
testenv.MustHaveParallelism(t)

goroutineProf := Lookup("goroutine")

// Set up a goroutine to just create and run coroutine goroutines all day.
iterFunc := func() {
p, stop := iter.Pull2(
func(yield func(int, int) bool) {
for i := 0; i < 10000; i++ {
if !yield(i, i) {
return
}
}
},
)
defer stop()
for {
_, _, ok := p()
if !ok {
break
}
}
}
var wg sync.WaitGroup
done := make(chan struct{})
wg.Add(1)
go func() {
defer wg.Done()
for {
iterFunc()
select {
case <-done:
default:
}
}
}()

// Take a goroutine profile. If the bug in #69998 is present, this will crash
// with high probability. We don't care about the output for this bug.
goroutineProf.WriteTo(io.Discard, 1)
}

func BenchmarkGoroutine(b *testing.B) {
withIdle := func(n int, fn func(b *testing.B)) func(b *testing.B) {
return func(b *testing.B) {
Expand Down

0 comments on commit cfe0ae0

Please sign in to comment.