runtime: fix comments on the behavior of SetGCPercent

Fixes for #49680, #49695, #45867, and #49370 all assumed that
SetGCPercent(-1) doesn't block until the GC's mark phase is done, but
it actually does. The cause of 3 of those 4 failures comes from the fact
that at the beginning of the sweep phase, the GC does try to preempt
every P once, and this may run concurrently with test code. In the
fourth case, the issue was likely that only *one* of the debug_test.go
tests was missing a call to SetGCPercent(-1). Just to be safe, leave a
TODO there for now to remove the extraneous runtime.GC calls, but leave
the calls in.

Updates #49680, #49695, #45867, and #49370.

Change-Id: Ibf4e64addfba18312526968bcf40f1f5d54eb3f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/369815
Reviewed-by: Austin Clements <austin@google.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/misc/cgo/test/testdata/issue9400_linux.go b/misc/cgo/test/testdata/issue9400_linux.go
index 47f224dc4fbb6366b815b0251c485b5cb4cf77c5..051b9ab0bbe526baf56dab936680434d4eb740e0 100644 (file)
--- a/misc/cgo/test/testdata/issue9400_linux.go
+++ b/misc/cgo/test/testdata/issue9400_linux.go
@@ -50,7 +50,9 @@ func test9400(t *testing.T) {
        // Disable GC for the duration of the test.
        // This avoids a potential GC deadlock when spinning in uninterruptable ASM below #49695.
        defer debug.SetGCPercent(debug.SetGCPercent(-1))
-       // And finish any pending GC after we pause, if any.
+       // SetGCPercent waits until the mark phase is over, but the runtime
+       // also preempts at the start of the sweep phase, so make sure that's
+       // done too. See #49695.
        runtime.GC()
 
        // Temporarily rewind the stack and trigger SIGSETXID

diff --git a/src/runtime/debug_test.go b/src/runtime/debug_test.go
index 44585b1744af991cd304fbbcc08bbd916bc42c52..5bb0c5cee3be8b122d2dad43ac88a2516324a425 100644 (file)
--- a/src/runtime/debug_test.go
+++ b/src/runtime/debug_test.go
@@ -34,10 +34,17 @@ func startDebugCallWorker(t *testing.T) (g *runtime.G, after func()) {
        skipUnderDebugger(t)
 
        // This can deadlock if there aren't enough threads or if a GC
-       // tries to interrupt an atomic loop (see issue #10958). A GC
-       // could also actively be in progress (see issue #49370), so we
-       // need to call runtime.GC to block until it has complete. We
-       // use 8 Ps so there's room for the debug call worker,
+       // tries to interrupt an atomic loop (see issue #10958). Execute
+       // an extra GC to ensure even the sweep phase is done (out of
+       // caution to prevent #49370 from happening).
+       // TODO(mknyszek): This extra GC cycle is likely unnecessary
+       // because preemption (which may happen during the sweep phase)
+       // isn't much of an issue anymore thanks to asynchronous preemption.
+       // The biggest risk is having a write barrier in the debug call
+       // injection test code fire, because it runs in a signal handler
+       // and may not have a P.
+       //
+       // We use 8 Ps so there's room for the debug call worker,
        // something that's trying to preempt the call worker, and the
        // goroutine that's trying to stop the call worker.
        ogomaxprocs := runtime.GOMAXPROCS(8)
@@ -270,8 +277,14 @@ func TestDebugCallPanic(t *testing.T) {
        // progress. Wait until the current GC is done, and turn it off.
        //
        // See #10958 and #49370.
-       runtime.GC()
        defer debug.SetGCPercent(debug.SetGCPercent(-1))
+       // TODO(mknyszek): This extra GC cycle is likely unnecessary
+       // because preemption (which may happen during the sweep phase)
+       // isn't much of an issue anymore thanks to asynchronous preemption.
+       // The biggest risk is having a write barrier in the debug call
+       // injection test code fire, because it runs in a signal handler
+       // and may not have a P.
+       runtime.GC()
 
        ready := make(chan *runtime.G)
        var stop uint32

diff --git a/src/runtime/proc_test.go b/src/runtime/proc_test.go
index 9198022ace2f04cad4c7a4dcb8520c86a25da345..cc899a24c677acbd83a7cee7a9437f0f58677554 100644 (file)
--- a/src/runtime/proc_test.go
+++ b/src/runtime/proc_test.go
@@ -119,8 +119,9 @@ func TestGoroutineParallelism(t *testing.T) {
        // since the goroutines can't be stopped/preempted.
        // Disable GC for this test (see issue #10958).
        defer debug.SetGCPercent(debug.SetGCPercent(-1))
-       // Now that GCs are disabled, block until any outstanding GCs
-       // are also done.
+       // SetGCPercent waits until the mark phase is over, but the runtime
+       // also preempts at the start of the sweep phase, so make sure that's
+       // done too. See #45867.
        runtime.GC()
        for try := 0; try < N; try++ {
                done := make(chan bool)
@@ -166,8 +167,9 @@ func testGoroutineParallelism2(t *testing.T, load, netpoll bool) {
        // since the goroutines can't be stopped/preempted.
        // Disable GC for this test (see issue #10958).
        defer debug.SetGCPercent(debug.SetGCPercent(-1))
-       // Now that GCs are disabled, block until any outstanding GCs
-       // are also done.
+       // SetGCPercent waits until the mark phase is over, but the runtime
+       // also preempts at the start of the sweep phase, so make sure that's
+       // done too. See #45867.
        runtime.GC()
        for try := 0; try < N; try++ {
                if load {
@@ -629,8 +631,9 @@ func TestSchedLocalQueueEmpty(t *testing.T) {
        // If runtime triggers a forced GC during this test then it will deadlock,
        // since the goroutines can't be stopped/preempted during spin wait.
        defer debug.SetGCPercent(debug.SetGCPercent(-1))
-       // Now that GCs are disabled, block until any outstanding GCs
-       // are also done.
+       // SetGCPercent waits until the mark phase is over, but the runtime
+       // also preempts at the start of the sweep phase, so make sure that's
+       // done too. See #45867.
        runtime.GC()
 
        iters := int(1e5)

diff --git a/src/runtime/rwmutex_test.go b/src/runtime/rwmutex_test.go
index 33ddd7d1d59c09a81ea0a5b1205dec7f34a59869..f15d367b326fb9552f1e7f431cae77b18913afb7 100644 (file)
--- a/src/runtime/rwmutex_test.go
+++ b/src/runtime/rwmutex_test.go
@@ -55,7 +55,9 @@ func TestParallelRWMutexReaders(t *testing.T) {
        // since the goroutines can't be stopped/preempted.
        // Disable GC for this test (see issue #10958).
        defer debug.SetGCPercent(debug.SetGCPercent(-1))
-       // Finish any in-progress GCs and get ourselves to a clean slate.
+       // SetGCPercent waits until the mark phase is over, but the runtime
+       // also preempts at the start of the sweep phase, so make sure that's
+       // done too.
        GC()
 
        doTestParallelReaders(1)

diff --git a/src/runtime/testdata/testprog/preempt.go b/src/runtime/testdata/testprog/preempt.go
index eb9f59053cf170af9b38cf0035a4bd40157beca2..fb6755a37253a90ef3c72f20b943a8db745560dd 100644 (file)
--- a/src/runtime/testdata/testprog/preempt.go
+++ b/src/runtime/testdata/testprog/preempt.go
@@ -21,7 +21,8 @@ func AsyncPreempt() {
        // Disable GC so we have complete control of what we're testing.
        debug.SetGCPercent(-1)
        // Out of an abundance of caution, also make sure that there are
-       // no GCs actively in progress.
+       // no GCs actively in progress. The sweep phase of a GC cycle
+       // for instance tries to preempt Ps at the very beginning.
        runtime.GC()
 
        // Start a goroutine with no sync safe-points.