runtime: eliminate _Psyscall

This change eliminates the _Psyscall state by using synchronization on
the G status _Gsyscall to make syscalls work instead. This removes an
atomic Store and an atomic CAS on the syscall path, which reduces
syscall and cgo overheads. It also simplifies the syscall paths quite a
bit.

The one danger with this change is that we have a new combination of
states that was previously impossible. There are brief windows where
it's possible to observe a goroutine in _Grunning but without a P. This
change is careful to hide this detail from the execution tracer, but it
may have unexpected effects in the rest of the runtime, making this
change somewhat risky.

goos: linux
goarch: amd64
pkg: internal/runtime/cgobench
cpu: AMD EPYC 7B13
                   │ before.out  │             after.out              │
                   │   sec/op    │   sec/op     vs base               │
CgoCall-64           43.69n ± 1%   35.83n ± 1%  -17.99% (p=0.002 n=6)
CgoCallParallel-64   5.306n ± 1%   5.338n ± 1%        ~ (p=0.132 n=6)

Change-Id: I4551afc1eea0c1b67a0b2dd26b0d49aa47bf1fb8
Reviewed-on: https://go-review.googlesource.com/c/go/+/646198
Auto-Submit: Michael Knyszek <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>

Author: mknyszek

src/runtime/crash_test.go

@@ -788,6 +788,9 @@ func TestPanicInlined(t *testing.T) {
 // Test for issues #3934 and #20018.
 // We want to delay exiting until a panic print is complete.
 func TestPanicRace(t *testing.T) {
+	if race.Enabled {
+		t.Skip("racefini exits program before main can delay exiting")
+	}
 	testenv.MustHaveGoRun(t)
 
 	exe, err := buildTestProg(t, "testprog")

src/runtime/heapdump.go

@@ -412,6 +412,14 @@ func dumpgs() {
 	forEachG(func(gp *g) {
 		status := readgstatus(gp) // The world is stopped so gp will not be in a scan state.
 		switch status {
+		case _Grunning:
+			// Dump goroutine if it's _Grunning only during a syscall. This is safe
+			// because the goroutine will just park without mutating its stack, since
+			// the world is stopped.
+			if gp.syscallsp != 0 {
+				dumpgoroutine(gp)
+			}
+			fallthrough
 		default:
 			print("runtime: unexpected G.status ", hex(status), "\n")
 			throw("dumpgs in STW - bad status")

src/runtime/metrics.go

@@ -818,9 +818,12 @@ func (a *schedStatsAggregate) compute() {
 		a.gCreated += p.goroutinesCreated
 		switch p.status {
 		case _Prunning:
-			a.gRunning++
-		case _Psyscall:
-			a.gNonGo++
+			if thread, ok := setBlockOnExitSyscall(p); ok {
+				thread.resume()
+				a.gNonGo++
+			} else {
+				a.gRunning++
+			}
 		case _Pgcstop:
 			// The world is stopping or stopped.
 			// This is fine. The results will be
@@ -847,7 +850,7 @@ func (a *schedStatsAggregate) compute() {
 	// Global run queue.
 	a.gRunnable += uint64(sched.runq.size)
 
-	// Account for Gs that are in _Gsyscall without a P in _Psyscall.
+	// Account for Gs that are in _Gsyscall without a P.
 	nGsyscallNoP := sched.nGsyscallNoP.Load()
 
 	// nGsyscallNoP can go negative during temporary races.

src/runtime/mprof.go

@@ -1535,7 +1535,18 @@ func doRecordGoroutineProfile(gp1 *g, pcbuf []uintptr) {
 		// everything else, we just don't record the stack in the profile.
 		return
 	}
-	if readgstatus(gp1) == _Grunning {
+	// Double-check that we didn't make a grave mistake. If the G is running then in
+	// general, we cannot safely read its stack.
+	//
+	// However, there is one case where it's OK. There's a small window of time in
+	// exitsyscall where a goroutine could be in _Grunning as it's exiting a syscall.
+	// This is OK because goroutine will not exit the syscall until it passes through
+	// a call to tryRecordGoroutineProfile. (An explicit one on the fast path, an
+	// implicit one via the scheduler on the slow path.)
+	//
+	// This is also why it's safe to check syscallsp here. The syscall path mutates
+	// syscallsp only after passing through tryRecordGoroutineProfile.
+	if readgstatus(gp1) == _Grunning && gp1.syscallsp == 0 {
 		print("doRecordGoroutineProfile gp1=", gp1.goid, "\n")
 		throw("cannot read stack of running goroutine")
 	}

src/runtime/mstats.go

@@ -914,7 +914,7 @@ type cpuStats struct {
 	ScavengeTotalTime  int64
 
 	IdleTime int64 // Time Ps spent in _Pidle.
-	UserTime int64 // Time Ps spent in _Prunning or _Psyscall that's not any of the above.
+	UserTime int64 // Time Ps spent in _Prunning that's not any of the above.
 
 	TotalTime int64 // GOMAXPROCS * (monotonic wall clock time elapsed)
 }
@@ -976,7 +976,7 @@ func (s *cpuStats) accumulate(now int64, gcMarkPhase bool) {
 	// Compute userTime. We compute this indirectly as everything that's not the above.
 	//
 	// Since time spent in _Pgcstop is covered by gcPauseTime, and time spent in _Pidle
-	// is covered by idleTime, what we're left with is time spent in _Prunning and _Psyscall,
+	// is covered by idleTime, what we're left with is time spent in _Prunning,
 	// the latter of which is fine because the P will either go idle or get used for something
 	// else via sysmon. Meanwhile if we subtract GC time from whatever's left, we get non-GC
 	// _Prunning time. Note that this still leaves time spent in sweeping and in the scheduler,

src/runtime/os_windows.go

@@ -191,8 +191,8 @@ type mOS struct {
 	// complete.
 	//
 	// TODO(austin): We may not need this if preemption were more
-	// tightly synchronized on the G/P status and preemption
-	// blocked transition into _Gsyscall/_Psyscall.
+	// tightly synchronized on the G status and preemption
+	// blocked transition into _Gsyscall.
 	preemptExtLock uint32
 }
 

src/runtime/preempt.go

@@ -286,7 +286,7 @@ func resumeG(state suspendGState) {
 //
 //go:nosplit
 func canPreemptM(mp *m) bool {
-	return mp.locks == 0 && mp.mallocing == 0 && mp.preemptoff == "" && mp.p.ptr().status == _Prunning
+	return mp.locks == 0 && mp.mallocing == 0 && mp.preemptoff == "" && mp.p.ptr().status == _Prunning && mp.curg != nil && readgstatus(mp.curg)&^_Gscan != _Gsyscall
 }
 
 //go:generate go run mkpreempt.go