runtime: fix a few GC-related bugs

1) Move non-preemption check even earlier in newstack.
This avoids a few priority inversion problems.

2) Always use atomic operations to update bitmap for 1-word objects.
This avoids lost mark bits during concurrent GC.

3) Stop using work.nproc == 1 as a signal for being single-threaded.
The concurrent GC runs with work.nproc == 1 but other procs are
running mutator code.

The use of work.nproc == 1 in getfull *is* safe, but remove it anyway,
since it is saving only a single atomic operation per GC round.

Fixes #9225.

Change-Id: I24134f100ad592ea8cb59efb6a54f5a1311093dc
Reviewed-on: https://go-review.googlesource.com/2745
Reviewed-by: Rick Hudson <rlh@golang.org>

1 files changed, 23 insertions, 15 deletions

diff --git a/src/runtime/stack1.go b/src/runtime/stack1.go
index ed1ff3428d..2c12cd73f3 100644
--- a/src/runtime/stack1.go
+++ b/src/runtime/stack1.go
@@ -634,21 +634,39 @@ func newstack() {
 		throw("runtime: stack split at bad time")
 	}
 
-	// The goroutine must be executing in order to call newstack,
-	// so it must be Grunning or Gscanrunning.
-
 	gp := thisg.m.curg
 	morebuf := thisg.m.morebuf
 	thisg.m.morebuf.pc = 0
 	thisg.m.morebuf.lr = 0
 	thisg.m.morebuf.sp = 0
 	thisg.m.morebuf.g = 0
+	rewindmorestack(&gp.sched)
+
+	// Be conservative about where we preempt.
+	// We are interested in preempting user Go code, not runtime code.
+	// If we're holding locks, mallocing, or GCing, don't preempt.
+	// This check is very early in newstack so that even the status change
+	// from Grunning to Gwaiting and back doesn't happen in this case.
+	// That status change by itself can be viewed as a small preemption,
+	// because the GC might change Gwaiting to Gscanwaiting, and then
+	// this goroutine has to wait for the GC to finish before continuing.
+	// If the GC is in some way dependent on this goroutine (for example,
+	// it needs a lock held by the goroutine), that small preemption turns
+	// into a real deadlock.
+	if gp.stackguard0 == stackPreempt {
+		if thisg.m.locks != 0 || thisg.m.mallocing != 0 || thisg.m.gcing != 0 || thisg.m.p.status != _Prunning {
+			// Let the goroutine keep running for now.
+			// gp->preempt is set, so it will be preempted next time.
+			gp.stackguard0 = gp.stack.lo + _StackGuard
+			gogo(&gp.sched) // never return
+		}
+	}
 
+	// The goroutine must be executing in order to call newstack,
+	// so it must be Grunning (or Gscanrunning).
 	casgstatus(gp, _Grunning, _Gwaiting)
 	gp.waitreason = "stack growth"
 
-	rewindmorestack(&gp.sched)
-
 	if gp.stack.lo == 0 {
 		throw("missing stack in newstack")
 	}
@@ -697,16 +715,6 @@ func newstack() {
 			gogo(&gp.sched)        // never return
 		}
 
-		// Be conservative about where we preempt.
-		// We are interested in preempting user Go code, not runtime code.
-		if thisg.m.locks != 0 || thisg.m.mallocing != 0 || thisg.m.gcing != 0 || thisg.m.p.status != _Prunning {
-			// Let the goroutine keep running for now.
-			// gp->preempt is set, so it will be preempted next time.
-			gp.stackguard0 = gp.stack.lo + _StackGuard
-			casgstatus(gp, _Gwaiting, _Grunning)
-			gogo(&gp.sched) // never return
-		}
-
 		// Act like goroutine called runtime.Gosched.
 		casgstatus(gp, _Gwaiting, _Grunning)
 		gosched_m(gp) // never return