runtime: disable preemption in startTemplateThread

When a locked M wants to start a new M, it hands off to the template
thread to actually call clone and start the thread. The template thread
is lazily created the first time a thread is locked (or if cgo is in
use).

stoplockedm will release the P (_Pidle), then call handoffp to give the
P to another M. In the case of a pending STW, one of two things can
happen:

1. handoffp starts an M, which does acquirep followed by schedule, which
will finally enter _Pgcstop.

2. handoffp immediately enters _Pgcstop. This only occurs if the P has
no local work, GC work, and no spinning M is required.

If handoffp starts an M, and must create a new M to do so, then newm
will simply queue the M on newmHandoff for the template thread to do the
clone.

When a stop-the-world is required, stopTheWorldWithSema will start the
stop and then wait for all Ps to enter _Pgcstop. If the template thread
is not fully created because startTemplateThread gets stopped, then
another stoplockedm may queue an M that will never get created, and the
handoff P will never leave _Pidle. Thus stopTheWorldWithSema will wait
forever.

A sequence to trigger this hang when STW occurs can be visualized with
two threads:

  T1                                 T2
-------------------------------   -----------------------------

LockOSThread                      LockOSThread
  haveTemplateThread == 0
  startTemplateThread
    haveTemplateThread = 1
    newm                            haveTemplateThread == 1
      preempt -> schedule           g.m.lockedExt++
        gcstopm -> _Pgcstop         g.m.lockedg = ...
        park                        g.lockedm = ...
                                    return

                                 ... (any code)
                                   preempt -> schedule
                                     stoplockedm
                                       releasep -> _Pidle
                                       handoffp
                                         startm (first 3 handoffp cases)
                                          newm
                                            g.m.lockedExt != 0
                                            Add to newmHandoff, return
                                       park

Note that the P in T2 is stuck sitting in _Pidle. Since the template
thread isn't running, the new M will not be started complete the
transition to _Pgcstop.

To resolve this, we disable preemption around the assignment of
haveTemplateThread and the creation of the template thread in order to
guarantee that if handTemplateThread is set then the template thread
will eventually exist, in the presence of stops.

Fixes #38931

Change-Id: I50535fbbe2f328f47b18e24d9030136719274191
Reviewed-on: https://go-review.googlesource.com/c/go/+/232978
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go
index 5333b6064635cdc982c778657afcd98825ae9b36..34f30c9a372c94b36a476c75c15c3cd2c232a4d8 100644 (file)
--- a/src/runtime/crash_test.go
+++ b/src/runtime/crash_test.go
@@ -55,6 +55,16 @@ func runTestProg(t *testing.T, binary, name string, env ...string) string {
                t.Fatal(err)
        }
 
+       return runBuiltTestProg(t, exe, name, env...)
+}
+
+func runBuiltTestProg(t *testing.T, exe, name string, env ...string) string {
+       if *flagQuick {
+               t.Skip("-quick")
+       }
+
+       testenv.MustHaveGoBuild(t)
+
        cmd := testenv.CleanCmdEnv(exec.Command(exe, name))
        cmd.Env = append(cmd.Env, env...)
        if testing.Short() {
@@ -64,7 +74,7 @@ func runTestProg(t *testing.T, binary, name string, env ...string) string {
        cmd.Stdout = &b
        cmd.Stderr = &b
        if err := cmd.Start(); err != nil {
-               t.Fatalf("starting %s %s: %v", binary, name, err)
+               t.Fatalf("starting %s %s: %v", exe, name, err)
        }
 
        // If the process doesn't complete within 1 minute,
@@ -92,7 +102,7 @@ func runTestProg(t *testing.T, binary, name string, env ...string) string {
        }()
 
        if err := cmd.Wait(); err != nil {
-               t.Logf("%s %s exit status: %v", binary, name, err)
+               t.Logf("%s %s exit status: %v", exe, name, err)
        }
        close(done)
 

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index b423026c0e53bf763760ecac7cb0fb1f12c9e383..e5823dd804740b871b328026f166943f09dd2ccc 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -1820,10 +1820,16 @@ func startTemplateThread() {
        if GOARCH == "wasm" { // no threads on wasm yet
                return
        }
+
+       // Disable preemption to guarantee that the template thread will be
+       // created before a park once haveTemplateThread is set.
+       mp := acquirem()
        if !atomic.Cas(&newmHandoff.haveTemplateThread, 0, 1) {
+               releasem(mp)
                return
        }
        newm(templateThread, nil)
+       releasem(mp)
 }
 
 // templateThread is a thread in a known-good state that exists solely

diff --git a/src/runtime/proc_test.go b/src/runtime/proc_test.go
index 764a279fcaa8d62d717a445d05354862120eb183..de4dec36ce4d763cdb1792e4e9c3b448d5217822 100644 (file)
--- a/src/runtime/proc_test.go
+++ b/src/runtime/proc_test.go
@@ -7,6 +7,7 @@ package runtime_test
 import (
        "fmt"
        "internal/race"
+       "internal/testenv"
        "math"
        "net"
        "runtime"
@@ -929,6 +930,29 @@ func TestLockOSThreadAvoidsStatePropagation(t *testing.T) {
        }
 }
 
+func TestLockOSThreadTemplateThreadRace(t *testing.T) {
+       testenv.MustHaveGoRun(t)
+
+       exe, err := buildTestProg(t, "testprog")
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       iterations := 100
+       if testing.Short() {
+               // Reduce run time to ~100ms, with much lower probability of
+               // catching issues.
+               iterations = 5
+       }
+       for i := 0; i < iterations; i++ {
+               want := "OK\n"
+               output := runBuiltTestProg(t, exe, "LockOSThreadTemplateThreadRace")
+               if output != want {
+                       t.Fatalf("run %d: want %q, got %q", i, want, output)
+               }
+       }
+}
+
 // fakeSyscall emulates a system call.
 //go:nosplit
 func fakeSyscall(duration time.Duration) {

diff --git a/src/runtime/testdata/testprog/lockosthread.go b/src/runtime/testdata/testprog/lockosthread.go
index fd3123e64743fbd8a13e1bb65e872634769ea493..098cc4dd722e659d5d97d0ea6a326d2efc893e8d 100644 (file)
--- a/src/runtime/testdata/testprog/lockosthread.go
+++ b/src/runtime/testdata/testprog/lockosthread.go
@@ -7,6 +7,7 @@ package main
 import (
        "os"
        "runtime"
+       "sync"
        "time"
 )
 
@@ -30,6 +31,7 @@ func init() {
                runtime.LockOSThread()
        })
        register("LockOSThreadAvoidsStatePropagation", LockOSThreadAvoidsStatePropagation)
+       register("LockOSThreadTemplateThreadRace", LockOSThreadTemplateThreadRace)
 }
 
 func LockOSThreadMain() {
@@ -195,3 +197,50 @@ func LockOSThreadAvoidsStatePropagation() {
        runtime.UnlockOSThread()
        println("OK")
 }
+
+func LockOSThreadTemplateThreadRace() {
+       // This test attempts to reproduce the race described in
+       // golang.org/issue/38931. To do so, we must have a stop-the-world
+       // (achieved via ReadMemStats) racing with two LockOSThread calls.
+       //
+       // While this test attempts to line up the timing, it is only expected
+       // to fail (and thus hang) around 2% of the time if the race is
+       // present.
+
+       // Ensure enough Ps to actually run everything in parallel. Though on
+       // <4 core machines, we are still at the whim of the kernel scheduler.
+       runtime.GOMAXPROCS(4)
+
+       go func() {
+               // Stop the world; race with LockOSThread below.
+               var m runtime.MemStats
+               for {
+                       runtime.ReadMemStats(&m)
+               }
+       }()
+
+       // Try to synchronize both LockOSThreads.
+       start := time.Now().Add(10*time.Millisecond)
+
+       var wg sync.WaitGroup
+       wg.Add(2)
+
+       for i := 0; i < 2; i++ {
+               go func() {
+                       for time.Now().Before(start) {
+                       }
+
+                       // Add work to the local runq to trigger early startm
+                       // in handoffp.
+                       go func(){}()
+
+                       runtime.LockOSThread()
+                       runtime.Gosched()  // add a preemption point.
+                       wg.Done()
+               }()
+       }
+
+       wg.Wait()
+       // If both LockOSThreads completed then we did not hit the race.
+       println("OK")
+}