From 3f834114ab617eb7b414cb12e7ca8085b5fe3a5c Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Fri, 27 Sep 2019 12:27:51 -0400
Subject: runtime: add general suspendG/resumeG

Currently, the process of suspending a goroutine is tied to stack
scanning. In preparation for non-cooperative preemption, this CL
abstracts this into general purpose suspendG/resumeG functions.

suspendG and resumeG closely follow the existing scang and restartg
functions with one exception: the addition of a _Gpreempted status.
Currently, preemption tasks (stack scanning) are carried out by the
target goroutine if it's in _Grunning. In this new approach, the task
is always carried out by the goroutine that called suspendG. Thus, we
need a reliable way to drive the target goroutine out of _Grunning
until the requesting goroutine is ready to resume it. The new
_Gpreempted state provides the handshake: when a runnable goroutine
responds to a preemption request, it now parks itself and enters
_Gpreempted. The requesting goroutine races to put it in _Gwaiting,
which gives it ownership, but also the responsibility to start it
again.

This CL adds several TODOs about improving the synchronization on the
G status. The existing code already has these problems; we're just
taking note of them.

The next CL will remove the now-dead scang and preemptscan.

For #10958, #24543.

Change-Id: I16dbf87bea9d50399cc86719c156f48e67198f16
Reviewed-on: https://go-review.googlesource.com/c/go/+/201137
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
---
 src/runtime/stack.go | 5 +++++
 1 file changed, 5 insertions(+)

(limited to 'src/runtime/stack.go')

diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 93f9769899..3b92c89ff0 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -1017,6 +1017,11 @@ func newstack() {
 		if thisg.m.p == 0 && thisg.m.locks == 0 {
 			throw("runtime: g is running but p is not")
 		}
+
+		if gp.preemptStop {
+			preemptPark(gp) // never returns
+		}
+
 		// Synchronize with scang.
 		casgstatus(gp, _Grunning, _Gwaiting)
 		if gp.preemptscan {
-- 
cgit v1.3


From 1b79afe460c329c1db75456c3278600a4b451b41 Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Fri, 27 Sep 2019 12:31:33 -0400
Subject: runtime: remove old stack scanning code

This removes scang and preemptscan, since the stack scanning code now
uses suspendG.

For #10958, #24543.

Change-Id: Ic868bf5d6dcce40662a82cb27bb996cb74d0720e
Reviewed-on: https://go-review.googlesource.com/c/go/+/201138
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
---
 src/runtime/proc.go     | 113 ------------------------------------------------
 src/runtime/runtime2.go |   1 -
 src/runtime/stack.go    |  29 +------------
 3 files changed, 1 insertion(+), 142 deletions(-)

(limited to 'src/runtime/stack.go')

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 3964941b9c..9e40bc8c94 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -802,14 +802,6 @@ func casgstatus(gp *g, oldval, newval uint32) {
 		if oldval == _Gwaiting && gp.atomicstatus == _Grunnable {
 			throw("casgstatus: waiting for Gwaiting but is Grunnable")
 		}
-		// Help GC if needed.
-		// if gp.preemptscan && !gp.gcworkdone && (oldval == _Grunning || oldval == _Gsyscall) {
-		// 	gp.preemptscan = false
-		// 	systemstack(func() {
-		// 		gcphasework(gp)
-		// 	})
-		// }
-		// But meanwhile just yield.
 		if i == 0 {
 			nextYield = nanotime() + yieldDelay
 		}
@@ -867,111 +859,6 @@ func casGFromPreempted(gp *g, old, new uint32) bool {
 	return atomic.Cas(&gp.atomicstatus, _Gpreempted, _Gwaiting)
 }
 
-// scang blocks until gp's stack has been scanned.
-// It might be scanned by scang or it might be scanned by the goroutine itself.
-// Either way, the stack scan has completed when scang returns.
-func scang(gp *g, gcw *gcWork) {
-	// Invariant; we (the caller, markroot for a specific goroutine) own gp.gcscandone.
-	// Nothing is racing with us now, but gcscandone might be set to true left over
-	// from an earlier round of stack scanning (we scan twice per GC).
-	// We use gcscandone to record whether the scan has been done during this round.
-
-	gp.gcscandone = false
-
-	// See https://golang.org/cl/21503 for justification of the yield delay.
-	const yieldDelay = 10 * 1000
-	var nextYield int64
-
-	// Endeavor to get gcscandone set to true,
-	// either by doing the stack scan ourselves or by coercing gp to scan itself.
-	// gp.gcscandone can transition from false to true when we're not looking
-	// (if we asked for preemption), so any time we lock the status using
-	// castogscanstatus we have to double-check that the scan is still not done.
-loop:
-	for i := 0; !gp.gcscandone; i++ {
-		switch s := readgstatus(gp); s {
-		default:
-			dumpgstatus(gp)
-			throw("stopg: invalid status")
-
-		case _Gdead:
-			// No stack.
-			gp.gcscandone = true
-			break loop
-
-		case _Gcopystack:
-		// Stack being switched. Go around again.
-
-		case _Grunnable, _Gsyscall, _Gwaiting:
-			// Claim goroutine by setting scan bit.
-			// Racing with execution or readying of gp.
-			// The scan bit keeps them from running
-			// the goroutine until we're done.
-			if castogscanstatus(gp, s, s|_Gscan) {
-				if !gp.gcscandone {
-					scanstack(gp, gcw)
-					gp.gcscandone = true
-				}
-				restartg(gp)
-				break loop
-			}
-
-		case _Gscanwaiting:
-		// newstack is doing a scan for us right now. Wait.
-
-		case _Grunning:
-			// Goroutine running. Try to preempt execution so it can scan itself.
-			// The preemption handler (in newstack) does the actual scan.
-
-			// Optimization: if there is already a pending preemption request
-			// (from the previous loop iteration), don't bother with the atomics.
-			if gp.preemptscan && gp.preempt && gp.stackguard0 == stackPreempt {
-				break
-			}
-
-			// Ask for preemption and self scan.
-			if castogscanstatus(gp, _Grunning, _Gscanrunning) {
-				if !gp.gcscandone {
-					gp.preemptscan = true
-					gp.preempt = true
-					gp.stackguard0 = stackPreempt
-				}
-				casfrom_Gscanstatus(gp, _Gscanrunning, _Grunning)
-			}
-		}
-
-		if i == 0 {
-			nextYield = nanotime() + yieldDelay
-		}
-		if nanotime() < nextYield {
-			procyield(10)
-		} else {
-			osyield()
-			nextYield = nanotime() + yieldDelay/2
-		}
-	}
-
-	gp.preemptscan = false // cancel scan request if no longer needed
-}
-
-// The GC requests that this routine be moved from a scanmumble state to a mumble state.
-func restartg(gp *g) {
-	s := readgstatus(gp)
-	switch s {
-	default:
-		dumpgstatus(gp)
-		throw("restartg: unexpected status")
-
-	case _Gdead:
-	// ok
-
-	case _Gscanrunnable,
-		_Gscanwaiting,
-		_Gscansyscall:
-		casfrom_Gscanstatus(gp, s, s&^_Gscan)
-	}
-}
-
 // stopTheWorld stops all P's from executing goroutines, interrupting
 // all goroutines at GC safe points and records reason as the reason
 // for the stop. On return, only the current goroutine's P is running.
diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index 7eac58eb2c..7630888a3d 100644
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -421,7 +421,6 @@ type g struct {
 	preempt        bool       // preemption signal, duplicates stackguard0 = stackpreempt
 	preemptStop    bool       // transition to _Gpreempted on preemption; otherwise, just deschedule
 	paniconfault   bool       // panic (instead of crash) on unexpected fault address
-	preemptscan    bool       // preempted g does scan for gc
 	gcscandone     bool       // g has scanned stack; protected by _Gscan bit in status
 	gcscanvalid    bool       // false at start of gc cycle, true if G has not run since last scan; TODO: remove?
 	throwsplit     bool       // must not split stack
diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 3b92c89ff0..2c2a88e6e1 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -916,7 +916,7 @@ func round2(x int32) int32 {
 // Stack growth is multiplicative, for constant amortized cost.
 //
 // g->atomicstatus will be Grunning or Gscanrunning upon entry.
-// If the GC is trying to stop this g then it will set preemptscan to true.
+// If the scheduler is trying to stop this g, then it will set preemptStop.
 //
 // This must be nowritebarrierrec because it can be called as part of
 // stack growth from other nowritebarrierrec functions, but the
@@ -1022,34 +1022,7 @@ func newstack() {
 			preemptPark(gp) // never returns
 		}
 
-		// Synchronize with scang.
-		casgstatus(gp, _Grunning, _Gwaiting)
-		if gp.preemptscan {
-			for !castogscanstatus(gp, _Gwaiting, _Gscanwaiting) {
-				// Likely to be racing with the GC as
-				// it sees a _Gwaiting and does the
-				// stack scan. If so, gcworkdone will
-				// be set and gcphasework will simply
-				// return.
-			}
-			if !gp.gcscandone {
-				// gcw is safe because we're on the
-				// system stack.
-				gcw := &gp.m.p.ptr().gcw
-				scanstack(gp, gcw)
-				gp.gcscandone = true
-			}
-			gp.preemptscan = false
-			gp.preempt = false
-			casfrom_Gscanstatus(gp, _Gscanwaiting, _Gwaiting)
-			// This clears gcscanvalid.
-			casgstatus(gp, _Gwaiting, _Grunning)
-			gp.stackguard0 = gp.stack.lo + _StackGuard
-			gogo(&gp.sched) // never return
-		}
-
 		// Act like goroutine called runtime.Gosched.
-		casgstatus(gp, _Gwaiting, _Grunning)
 		gopreempt_m(gp) // never return
 	}
 
-- 
cgit v1.3


From 36a432f27bbcc65fb03845ebe5e4a3db6f4cc189 Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Wed, 3 Apr 2019 14:00:12 -0400
Subject: runtime: make copystack/sudog synchronization more explicit

When we copy a stack of a goroutine blocked in a channel operation, we
have to be very careful because other goroutines may be writing to
that goroutine's stack. To handle this, stack copying acquires the
locks for the channels a goroutine is waiting on.

One complication is that stack growth may happen while a goroutine
holds these locks, in which case stack copying must *not* acquire
these locks because that would self-deadlock.

Currently, stack growth never acquires these locks because stack
growth only happens when a goroutine is running, which means it's
either not blocking on a channel or it's holding the channel locks
already. Stack shrinking always acquires these locks because shrinking
happens asynchronously, so the goroutine is never running, so there
are either no locks or they've been released by the goroutine.

However, we're about to change when stack shrinking can happen, which
is going to break the current rules. Rather than find a new way to
derive whether to acquire these locks or not, this CL simply adds a
flag to the g struct that indicates that stack copying should acquire
channel locks. This flag is set while the goroutine is blocked on a
channel op.

For #10958, #24543.

Change-Id: Ia2ac8831b1bfda98d39bb30285e144c4f7eaf9ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/172982
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
---
 src/runtime/chan.go        | 14 ++++++++++--
 src/runtime/runtime2.go    | 54 +++++++++++++++++++++++++---------------------
 src/runtime/select.go      |  4 ++++
 src/runtime/sizeof_test.go |  2 +-
 src/runtime/stack.go       | 30 ++++++++++----------------
 5 files changed, 58 insertions(+), 46 deletions(-)

(limited to 'src/runtime/stack.go')

diff --git a/src/runtime/chan.go b/src/runtime/chan.go
index 93afe90dad..677af99eac 100644
--- a/src/runtime/chan.go
+++ b/src/runtime/chan.go
@@ -249,7 +249,7 @@ func chansend(c *hchan, ep unsafe.Pointer, block bool, callerpc uintptr) bool {
 	gp.waiting = mysg
 	gp.param = nil
 	c.sendq.enqueue(mysg)
-	goparkunlock(&c.lock, waitReasonChanSend, traceEvGoBlockSend, 3)
+	gopark(chanparkcommit, unsafe.Pointer(&c.lock), waitReasonChanSend, traceEvGoBlockSend, 2)
 	// Ensure the value being sent is kept alive until the
 	// receiver copies it out. The sudog has a pointer to the
 	// stack object, but sudogs aren't considered as roots of the
@@ -261,6 +261,7 @@ func chansend(c *hchan, ep unsafe.Pointer, block bool, callerpc uintptr) bool {
 		throw("G waiting list is corrupted")
 	}
 	gp.waiting = nil
+	gp.activeStackChans = false
 	if gp.param == nil {
 		if c.closed == 0 {
 			throw("chansend: spurious wakeup")
@@ -559,13 +560,14 @@ func chanrecv(c *hchan, ep unsafe.Pointer, block bool) (selected, received bool)
 	mysg.c = c
 	gp.param = nil
 	c.recvq.enqueue(mysg)
-	goparkunlock(&c.lock, waitReasonChanReceive, traceEvGoBlockRecv, 3)
+	gopark(chanparkcommit, unsafe.Pointer(&c.lock), waitReasonChanReceive, traceEvGoBlockRecv, 2)
 
 	// someone woke us up
 	if mysg != gp.waiting {
 		throw("G waiting list is corrupted")
 	}
 	gp.waiting = nil
+	gp.activeStackChans = false
 	if mysg.releasetime > 0 {
 		blockevent(mysg.releasetime-t0, 2)
 	}
@@ -632,6 +634,14 @@ func recv(c *hchan, sg *sudog, ep unsafe.Pointer, unlockf func(), skip int) {
 	goready(gp, skip+1)
 }
 
+func chanparkcommit(gp *g, chanLock unsafe.Pointer) bool {
+	// There are unlocked sudogs that point into gp's stack. Stack
+	// copying must lock the channels of those sudogs.
+	gp.activeStackChans = true
+	unlock((*mutex)(chanLock))
+	return true
+}
+
 // compiler implements
 //
 //	select {
diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index a146f47446..bf56466e08 100644
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -404,30 +404,36 @@ type g struct {
 	stackguard0 uintptr // offset known to liblink
 	stackguard1 uintptr // offset known to liblink
 
-	_panic         *_panic // innermost panic - offset known to liblink
-	_defer         *_defer // innermost defer
-	m              *m      // current m; offset known to arm liblink
-	sched          gobuf
-	syscallsp      uintptr        // if status==Gsyscall, syscallsp = sched.sp to use during gc
-	syscallpc      uintptr        // if status==Gsyscall, syscallpc = sched.pc to use during gc
-	stktopsp       uintptr        // expected sp at top of stack, to check in traceback
-	param          unsafe.Pointer // passed parameter on wakeup
-	atomicstatus   uint32
-	stackLock      uint32 // sigprof/scang lock; TODO: fold in to atomicstatus
-	goid           int64
-	schedlink      guintptr
-	waitsince      int64      // approx time when the g become blocked
-	waitreason     waitReason // if status==Gwaiting
-	preempt        bool       // preemption signal, duplicates stackguard0 = stackpreempt
-	preemptStop    bool       // transition to _Gpreempted on preemption; otherwise, just deschedule
-	paniconfault   bool       // panic (instead of crash) on unexpected fault address
-	gcscandone     bool       // g has scanned stack; protected by _Gscan bit in status
-	throwsplit     bool       // must not split stack
-	raceignore     int8       // ignore race detection events
-	sysblocktraced bool       // StartTrace has emitted EvGoInSyscall about this goroutine
-	sysexitticks   int64      // cputicks when syscall has returned (for tracing)
-	traceseq       uint64     // trace event sequencer
-	tracelastp     puintptr   // last P emitted an event for this goroutine
+	_panic       *_panic // innermost panic - offset known to liblink
+	_defer       *_defer // innermost defer
+	m            *m      // current m; offset known to arm liblink
+	sched        gobuf
+	syscallsp    uintptr        // if status==Gsyscall, syscallsp = sched.sp to use during gc
+	syscallpc    uintptr        // if status==Gsyscall, syscallpc = sched.pc to use during gc
+	stktopsp     uintptr        // expected sp at top of stack, to check in traceback
+	param        unsafe.Pointer // passed parameter on wakeup
+	atomicstatus uint32
+	stackLock    uint32 // sigprof/scang lock; TODO: fold in to atomicstatus
+	goid         int64
+	schedlink    guintptr
+	waitsince    int64      // approx time when the g become blocked
+	waitreason   waitReason // if status==Gwaiting
+	preempt      bool       // preemption signal, duplicates stackguard0 = stackpreempt
+	preemptStop  bool       // transition to _Gpreempted on preemption; otherwise, just deschedule
+	paniconfault bool       // panic (instead of crash) on unexpected fault address
+	gcscandone   bool       // g has scanned stack; protected by _Gscan bit in status
+	throwsplit   bool       // must not split stack
+	// activeStackChans indicates that there are unlocked channels
+	// pointing into this goroutine's stack. If true, stack
+	// copying needs to acquire channel locks to protect these
+	// areas of the stack.
+	activeStackChans bool
+
+	raceignore     int8     // ignore race detection events
+	sysblocktraced bool     // StartTrace has emitted EvGoInSyscall about this goroutine
+	sysexitticks   int64    // cputicks when syscall has returned (for tracing)
+	traceseq       uint64   // trace event sequencer
+	tracelastp     puintptr // last P emitted an event for this goroutine
 	lockedm        muintptr
 	sig            uint32
 	writebuf       []byte
diff --git a/src/runtime/select.go b/src/runtime/select.go
index d2c5a03a1a..8033b6512f 100644
--- a/src/runtime/select.go
+++ b/src/runtime/select.go
@@ -75,6 +75,9 @@ func selunlock(scases []scase, lockorder []uint16) {
 }
 
 func selparkcommit(gp *g, _ unsafe.Pointer) bool {
+	// There are unlocked sudogs that point into gp's stack. Stack
+	// copying must lock the channels of those sudogs.
+	gp.activeStackChans = true
 	// This must not access gp's stack (see gopark). In
 	// particular, it must not access the *hselect. That's okay,
 	// because by the time this is called, gp.waiting has all
@@ -311,6 +314,7 @@ loop:
 	// wait for someone to wake us up
 	gp.param = nil
 	gopark(selparkcommit, nil, waitReasonSelect, traceEvGoBlockSelect, 1)
+	gp.activeStackChans = false
 
 	sellock(scases, lockorder)
 
diff --git a/src/runtime/sizeof_test.go b/src/runtime/sizeof_test.go
index 406a38aad9..852244d425 100644
--- a/src/runtime/sizeof_test.go
+++ b/src/runtime/sizeof_test.go
@@ -21,7 +21,7 @@ func TestSizeof(t *testing.T) {
 		_32bit uintptr     // size on 32bit platforms
 		_64bit uintptr     // size on 64bit platforms
 	}{
-		{runtime.G{}, 212, 368}, // g, but exported for testing
+		{runtime.G{}, 216, 376}, // g, but exported for testing
 	}
 
 	for _, tt := range tests {
diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 2c2a88e6e1..e47f12a8dc 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -786,10 +786,6 @@ func syncadjustsudogs(gp *g, used uintptr, adjinfo *adjustinfo) uintptr {
 	}
 
 	// Lock channels to prevent concurrent send/receive.
-	// It's important that we *only* do this for async
-	// copystack; otherwise, gp may be in the middle of
-	// putting itself on wait queues and this would
-	// self-deadlock.
 	var lastc *hchan
 	for sg := gp.waiting; sg != nil; sg = sg.waitlink {
 		if sg.c != lastc {
@@ -826,12 +822,7 @@ func syncadjustsudogs(gp *g, used uintptr, adjinfo *adjustinfo) uintptr {
 
 // Copies gp's stack to a new stack of a different size.
 // Caller must have changed gp status to Gcopystack.
-//
-// If sync is true, this is a self-triggered stack growth and, in
-// particular, no other G may be writing to gp's stack (e.g., via a
-// channel operation). If sync is false, copystack protects against
-// concurrent channel operations.
-func copystack(gp *g, newsize uintptr, sync bool) {
+func copystack(gp *g, newsize uintptr) {
 	if gp.syscallsp != 0 {
 		throw("stack growth not allowed in system call")
 	}
@@ -857,15 +848,16 @@ func copystack(gp *g, newsize uintptr, sync bool) {
 
 	// Adjust sudogs, synchronizing with channel ops if necessary.
 	ncopy := used
-	if sync {
+	if !gp.activeStackChans {
 		adjustsudogs(gp, &adjinfo)
 	} else {
-		// sudogs can point in to the stack. During concurrent
-		// shrinking, these areas may be written to. Find the
-		// highest such pointer so we can handle everything
-		// there and below carefully. (This shouldn't be far
-		// from the bottom of the stack, so there's little
-		// cost in handling everything below it carefully.)
+		// sudogs may be pointing in to the stack and gp has
+		// released channel locks, so other goroutines could
+		// be writing to gp's stack. Find the highest such
+		// pointer so we can handle everything there and below
+		// carefully. (This shouldn't be far from the bottom
+		// of the stack, so there's little cost in handling
+		// everything below it carefully.)
 		adjinfo.sghi = findsghi(gp, old)
 
 		// Synchronize with channel ops and copy the part of
@@ -1040,7 +1032,7 @@ func newstack() {
 
 	// The concurrent GC will not scan the stack while we are doing the copy since
 	// the gp is in a Gcopystack status.
-	copystack(gp, newsize, true)
+	copystack(gp, newsize)
 	if stackDebug >= 1 {
 		print("stack grow done\n")
 	}
@@ -1120,7 +1112,7 @@ func shrinkstack(gp *g) {
 		print("shrinking stack ", oldsize, "->", newsize, "\n")
 	}
 
-	copystack(gp, newsize, false)
+	copystack(gp, newsize)
 }
 
 // freeStackSpans frees unused stack spans at the end of GC.
-- 
cgit v1.3


From 60586034713cc94477868fb6911f34cfcc6a1ba4 Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Fri, 27 Sep 2019 14:34:05 -0400
Subject: runtime: only shrink stacks at synchronous safe points

We're about to introduce asynchronous safe points, where we won't have
precise pointer maps for all stack frames. That's okay for scanning
the stack (conservatively), but not for shrinking the stack.

Hence, this CL prepares for this by only shrinking the stack as part
of the stack scan if the goroutine is stopped at a synchronous safe
point. Otherwise, it queues up the stack shrink for the next
synchronous safe point.

We already have one condition under which we can't shrink the stack
for very similar reasons: syscalls. Currently, we just give up on
shrinking the stack if it's in a syscall. But with this mechanism, we
defer that stack shrink until the next synchronous safe point.

For #10958, #24543.

Change-Id: Ifa1dec6f33fdf30f9067be2ce3f7ab8a7f62ce38
Reviewed-on: https://go-review.googlesource.com/c/go/+/201438
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
---
 src/runtime/mgcmark.go  | 13 +++++++++++--
 src/runtime/runtime2.go | 13 ++++++++-----
 src/runtime/stack.go    | 43 ++++++++++++++++++++++++++++++++-----------
 3 files changed, 51 insertions(+), 18 deletions(-)

(limited to 'src/runtime/stack.go')

diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go
index 22e70ce157..338983424c 100644
--- a/src/runtime/mgcmark.go
+++ b/src/runtime/mgcmark.go
@@ -667,6 +667,10 @@ func gcFlushBgCredit(scanWork int64) {
 
 // scanstack scans gp's stack, greying all pointers found on the stack.
 //
+// scanstack will also shrink the stack if it is safe to do so. If it
+// is not, it schedules a stack shrink for the next synchronous safe
+// point.
+//
 // scanstack is marked go:systemstack because it must not be preempted
 // while using a workbuf.
 //
@@ -695,8 +699,13 @@ func scanstack(gp *g, gcw *gcWork) {
 		throw("can't scan our own stack")
 	}
 
-	// Shrink the stack if not much of it is being used.
-	shrinkstack(gp)
+	if isShrinkStackSafe(gp) {
+		// Shrink the stack if not much of it is being used.
+		shrinkstack(gp)
+	} else {
+		// Otherwise, shrink the stack at the next sync safe point.
+		gp.preemptShrink = true
+	}
 
 	var state stackScanState
 	state.stack = gp.stack
diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index bf56466e08..eecc6a78ac 100644
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -418,11 +418,14 @@ type g struct {
 	schedlink    guintptr
 	waitsince    int64      // approx time when the g become blocked
 	waitreason   waitReason // if status==Gwaiting
-	preempt      bool       // preemption signal, duplicates stackguard0 = stackpreempt
-	preemptStop  bool       // transition to _Gpreempted on preemption; otherwise, just deschedule
-	paniconfault bool       // panic (instead of crash) on unexpected fault address
-	gcscandone   bool       // g has scanned stack; protected by _Gscan bit in status
-	throwsplit   bool       // must not split stack
+
+	preempt       bool // preemption signal, duplicates stackguard0 = stackpreempt
+	preemptStop   bool // transition to _Gpreempted on preemption; otherwise, just deschedule
+	preemptShrink bool // shrink stack at synchronous safe point
+
+	paniconfault bool // panic (instead of crash) on unexpected fault address
+	gcscandone   bool // g has scanned stack; protected by _Gscan bit in status
+	throwsplit   bool // must not split stack
 	// activeStackChans indicates that there are unlocked channels
 	// pointing into this goroutine's stack. If true, stack
 	// copying needs to acquire channel locks to protect these
diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index e47f12a8dc..825826cacd 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -1010,6 +1010,13 @@ func newstack() {
 			throw("runtime: g is running but p is not")
 		}
 
+		if gp.preemptShrink {
+			// We're at a synchronous safe point now, so
+			// do the pending stack shrink.
+			gp.preemptShrink = false
+			shrinkstack(gp)
+		}
+
 		if gp.preemptStop {
 			preemptPark(gp) // never returns
 		}
@@ -1057,16 +1064,36 @@ func gostartcallfn(gobuf *gobuf, fv *funcval) {
 	gostartcall(gobuf, fn, unsafe.Pointer(fv))
 }
 
+// isShrinkStackSafe returns whether it's safe to attempt to shrink
+// gp's stack. Shrinking the stack is only safe when we have precise
+// pointer maps for all frames on the stack.
+func isShrinkStackSafe(gp *g) bool {
+	// We can't copy the stack if we're in a syscall.
+	// The syscall might have pointers into the stack and
+	// often we don't have precise pointer maps for the innermost
+	// frames.
+	return gp.syscallsp == 0
+}
+
 // Maybe shrink the stack being used by gp.
-// Called at garbage collection time.
-// gp must be stopped, but the world need not be.
+//
+// gp must be stopped and we must own its stack. It may be in
+// _Grunning, but only if this is our own user G.
 func shrinkstack(gp *g) {
-	gstatus := readgstatus(gp)
 	if gp.stack.lo == 0 {
 		throw("missing stack in shrinkstack")
 	}
-	if gstatus&_Gscan == 0 {
-		throw("bad status in shrinkstack")
+	if s := readgstatus(gp); s&_Gscan == 0 {
+		// We don't own the stack via _Gscan. We could still
+		// own it if this is our own user G and we're on the
+		// system stack.
+		if !(gp == getg().m.curg && getg() != getg().m.curg && s == _Grunning) {
+			// We don't own the stack.
+			throw("bad status in shrinkstack")
+		}
+	}
+	if !isShrinkStackSafe(gp) {
+		throw("shrinkstack at bad time")
 	}
 	// Check for self-shrinks while in a libcall. These may have
 	// pointers into the stack disguised as uintptrs, but these
@@ -1102,12 +1129,6 @@ func shrinkstack(gp *g) {
 		return
 	}
 
-	// We can't copy the stack if we're in a syscall.
-	// The syscall might have pointers into the stack.
-	if gp.syscallsp != 0 {
-		return
-	}
-
 	if stackDebug > 0 {
 		print("shrinking stack ", oldsize, "->", newsize, "\n")
 	}
-- 
cgit v1.3


From d1969015b4ac29be4f518b94817d3f525380639d Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Fri, 4 Oct 2019 18:54:00 -0400
Subject: runtime: abstract M preemption check into a function

We check whether an M is preemptible in a surprising number of places.
Put it in one function.

For #10958, #24543.

Change-Id: I305090fdb1ea7f7a55ffe25851c1e35012d0d06c
Reviewed-on: https://go-review.googlesource.com/c/go/+/201439
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
---
 src/runtime/mgcwork.go | 10 +++++-----
 src/runtime/preempt.go |  9 +++++++++
 src/runtime/proc.go    |  2 +-
 src/runtime/stack.go   |  2 +-
 4 files changed, 16 insertions(+), 7 deletions(-)

(limited to 'src/runtime/stack.go')

diff --git a/src/runtime/mgcwork.go b/src/runtime/mgcwork.go
index f2c16d7d8c..927b06c3f9 100644
--- a/src/runtime/mgcwork.go
+++ b/src/runtime/mgcwork.go
@@ -126,12 +126,12 @@ func (w *gcWork) checkPut(ptr uintptr, ptrs []uintptr) {
 	if debugCachedWork {
 		alreadyFailed := w.putGen == w.pauseGen
 		w.putGen = w.pauseGen
-		if m := getg().m; m.locks > 0 || m.mallocing != 0 || m.preemptoff != "" || m.p.ptr().status != _Prunning {
+		if !canPreemptM(getg().m) {
 			// If we were to spin, the runtime may
-			// deadlock: the condition above prevents
-			// preemption (see newstack), which could
-			// prevent gcMarkDone from finishing the
-			// ragged barrier and releasing the spin.
+			// deadlock. Since we can't be preempted, the
+			// spin could prevent gcMarkDone from
+			// finishing the ragged barrier, which is what
+			// releases us from the spin.
 			return
 		}
 		for atomic.Load(&gcWorkPauseGen) == w.pauseGen {
diff --git a/src/runtime/preempt.go b/src/runtime/preempt.go
index 0565fd6360..96eaa3488b 100644
--- a/src/runtime/preempt.go
+++ b/src/runtime/preempt.go
@@ -223,3 +223,12 @@ func resumeG(state suspendGState) {
 		ready(gp, 0, true)
 	}
 }
+
+// canPreemptM reports whether mp is in a state that is safe to preempt.
+//
+// It is nosplit because it has nosplit callers.
+//
+//go:nosplit
+func canPreemptM(mp *m) bool {
+	return mp.locks == 0 && mp.mallocing == 0 && mp.preemptoff == "" && mp.p.ptr().status == _Prunning
+}
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 9a553a5f88..60a15c1e9c 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2703,7 +2703,7 @@ func gosched_m(gp *g) {
 // goschedguarded is a forbidden-states-avoided version of gosched_m
 func goschedguarded_m(gp *g) {
 
-	if gp.m.locks != 0 || gp.m.mallocing != 0 || gp.m.preemptoff != "" || gp.m.p.ptr().status != _Prunning {
+	if !canPreemptM(gp.m) {
 		gogo(&gp.sched) // never return
 	}
 
diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 825826cacd..ecefce1e32 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -975,7 +975,7 @@ func newstack() {
 	// it needs a lock held by the goroutine), that small preemption turns
 	// into a real deadlock.
 	if preempt {
-		if thisg.m.locks != 0 || thisg.m.mallocing != 0 || thisg.m.preemptoff != "" || thisg.m.p.ptr().status != _Prunning {
+		if !canPreemptM(thisg.m) {
 			// Let the goroutine keep running for now.
 			// gp->preempt is set, so it will be preempted next time.
 			gp.stackguard0 = gp.stack.lo + _StackGuard
-- 
cgit v1.3


From 7de15e362b0bc4ba83c8ca4d7cadc319c99db65a Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Wed, 23 Oct 2019 11:25:38 -0400
Subject: runtime: atomically set span state and use as publication barrier

When everything is working correctly, any pointer the garbage
collector encounters can only point into a fully initialized heap
span, since the span must have been initialized before that pointer
could escape the heap allocator and become visible to the GC.

However, in various cases, we try to be defensive against bad
pointers. In findObject, this is just a sanity check: we never expect
to find a bad pointer, but programming errors can lead to them. In
spanOfHeap, we don't necessarily trust the pointer and we're trying to
check if it really does point to the heap, though it should always
point to something. Conservative scanning takes this to a new level,
since it can only guess that a word may be a pointer and verify this.

In all of these cases, we have a problem that the span lookup and
check can race with span initialization, since the span becomes
visible to lookups before it's fully initialized.

Furthermore, we're about to start initializing the span without the
heap lock held, which is going to introduce races where accesses were
previously protected by the heap lock.

To address this, this CL makes accesses to mspan.state atomic, and
ensures that the span is fully initialized before setting the state to
mSpanInUse. All loads are now atomic, and in any case where we don't
trust the pointer, it first atomically loads the span state and checks
that it's mSpanInUse, after which it will have synchronized with span
initialization and can safely check the other span fields.

For #10958, #24543, but a good fix in general.

Change-Id: I518b7c63555b02064b98aa5f802c92b758fef853
Reviewed-on: https://go-review.googlesource.com/c/go/+/203286
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
---
 src/runtime/cgocheck.go    |  2 +-
 src/runtime/export_test.go |  6 +--
 src/runtime/heapdump.go    |  8 ++--
 src/runtime/mbitmap.go     | 18 ++++++---
 src/runtime/mgcmark.go     | 16 ++++----
 src/runtime/mgcsweep.go    | 12 +++---
 src/runtime/mheap.go       | 93 ++++++++++++++++++++++++++++++++--------------
 src/runtime/signal_unix.go |  2 +-
 src/runtime/stack.go       |  4 +-
 9 files changed, 104 insertions(+), 57 deletions(-)

(limited to 'src/runtime/stack.go')

diff --git a/src/runtime/cgocheck.go b/src/runtime/cgocheck.go
index ed854e5e2b..9c5b26e4f3 100644
--- a/src/runtime/cgocheck.go
+++ b/src/runtime/cgocheck.go
@@ -133,7 +133,7 @@ func cgoCheckTypedBlock(typ *_type, src unsafe.Pointer, off, size uintptr) {
 	}
 
 	s := spanOfUnchecked(uintptr(src))
-	if s.state == mSpanManual {
+	if s.state.get() == mSpanManual {
 		// There are no heap bits for value stored on the stack.
 		// For a channel receive src might be on the stack of some
 		// other goroutine, so we can't unwind the stack even if
diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go
index 0bd5c902e8..831f3f13d4 100644
--- a/src/runtime/export_test.go
+++ b/src/runtime/export_test.go
@@ -256,7 +256,7 @@ func CountPagesInUse() (pagesInUse, counted uintptr) {
 	pagesInUse = uintptr(mheap_.pagesInUse)
 
 	for _, s := range mheap_.allspans {
-		if s.state == mSpanInUse {
+		if s.state.get() == mSpanInUse {
 			counted += s.npages
 		}
 	}
@@ -318,7 +318,7 @@ func ReadMemStatsSlow() (base, slow MemStats) {
 
 		// Add up current allocations in spans.
 		for _, s := range mheap_.allspans {
-			if s.state != mSpanInUse {
+			if s.state.get() != mSpanInUse {
 				continue
 			}
 			if sizeclass := s.spanclass.sizeclass(); sizeclass == 0 {
@@ -542,7 +542,7 @@ func UnscavHugePagesSlow() (uintptr, uintptr) {
 		lock(&mheap_.lock)
 		base = mheap_.free.unscavHugePages
 		for _, s := range mheap_.allspans {
-			if s.state == mSpanFree && !s.scavenged {
+			if s.state.get() == mSpanFree && !s.scavenged {
 				slow += s.hugePages()
 			}
 		}
diff --git a/src/runtime/heapdump.go b/src/runtime/heapdump.go
index 4d55b316f7..cfd5c251b4 100644
--- a/src/runtime/heapdump.go
+++ b/src/runtime/heapdump.go
@@ -435,7 +435,7 @@ func dumproots() {
 
 	// mspan.types
 	for _, s := range mheap_.allspans {
-		if s.state == mSpanInUse {
+		if s.state.get() == mSpanInUse {
 			// Finalizers
 			for sp := s.specials; sp != nil; sp = sp.next {
 				if sp.kind != _KindSpecialFinalizer {
@@ -458,7 +458,7 @@ var freemark [_PageSize / 8]bool
 
 func dumpobjs() {
 	for _, s := range mheap_.allspans {
-		if s.state != mSpanInUse {
+		if s.state.get() != mSpanInUse {
 			continue
 		}
 		p := s.base()
@@ -621,7 +621,7 @@ func dumpmemprof_callback(b *bucket, nstk uintptr, pstk *uintptr, size, allocs,
 func dumpmemprof() {
 	iterate_memprof(dumpmemprof_callback)
 	for _, s := range mheap_.allspans {
-		if s.state != mSpanInUse {
+		if s.state.get() != mSpanInUse {
 			continue
 		}
 		for sp := s.specials; sp != nil; sp = sp.next {
@@ -642,7 +642,7 @@ var dumphdr = []byte("go1.7 heap dump\n")
 func mdump() {
 	// make sure we're done sweeping
 	for _, s := range mheap_.allspans {
-		if s.state == mSpanInUse {
+		if s.state.get() == mSpanInUse {
 			s.ensureSwept()
 		}
 	}
diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go
index 9600cddac8..55c0282403 100644
--- a/src/runtime/mbitmap.go
+++ b/src/runtime/mbitmap.go
@@ -243,6 +243,10 @@ func (s *mspan) nextFreeIndex() uintptr {
 }
 
 // isFree reports whether the index'th object in s is unallocated.
+//
+// The caller must ensure s.state is mSpanInUse, and there must have
+// been no preemption points since ensuring this (which could allow a
+// GC transition, which would allow the state to change).
 func (s *mspan) isFree(index uintptr) bool {
 	if index < s.freeindex {
 		return false
@@ -361,12 +365,13 @@ func badPointer(s *mspan, p, refBase, refOff uintptr) {
 	// in allocated spans.
 	printlock()
 	print("runtime: pointer ", hex(p))
-	if s.state != mSpanInUse {
+	state := s.state.get()
+	if state != mSpanInUse {
 		print(" to unallocated span")
 	} else {
 		print(" to unused region of span")
 	}
-	print(" span.base()=", hex(s.base()), " span.limit=", hex(s.limit), " span.state=", s.state, "\n")
+	print(" span.base()=", hex(s.base()), " span.limit=", hex(s.limit), " span.state=", state, "\n")
 	if refBase != 0 {
 		print("runtime: found in object at *(", hex(refBase), "+", hex(refOff), ")\n")
 		gcDumpObject("object", refBase, refOff)
@@ -397,9 +402,12 @@ func findObject(p, refBase, refOff uintptr) (base uintptr, s *mspan, objIndex ui
 		return
 	}
 	// If p is a bad pointer, it may not be in s's bounds.
-	if p < s.base() || p >= s.limit || s.state != mSpanInUse {
+	//
+	// Check s.state to synchronize with span initialization
+	// before checking other fields. See also spanOfHeap.
+	if state := s.state.get(); state != mSpanInUse || p < s.base() || p >= s.limit {
 		// Pointers into stacks are also ok, the runtime manages these explicitly.
-		if s.state == mSpanManual {
+		if state == mSpanManual {
 			return
 		}
 		// The following ensures that we are rigorous about what data
@@ -620,7 +628,7 @@ func bulkBarrierPreWrite(dst, src, size uintptr) {
 			}
 		}
 		return
-	} else if s.state != mSpanInUse || dst < s.base() || s.limit <= dst {
+	} else if s.state.get() != mSpanInUse || dst < s.base() || s.limit <= dst {
 		// dst was heap memory at some point, but isn't now.
 		// It can't be a global. It must be either our stack,
 		// or in the case of direct channel sends, it could be
diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go
index 338983424c..2987d3572b 100644
--- a/src/runtime/mgcmark.go
+++ b/src/runtime/mgcmark.go
@@ -321,7 +321,9 @@ func markrootSpans(gcw *gcWork, shard int) {
 	// entered the scan phase, so addfinalizer will have ensured
 	// the above invariants for them.
 	for _, s := range spans {
-		if s.state != mSpanInUse {
+		// This is racing with spans being initialized, so
+		// check the state carefully.
+		if s.state.get() != mSpanInUse {
 			continue
 		}
 		// Check that this span was swept (it may be cached or uncached).
@@ -1310,15 +1312,15 @@ func gcDumpObject(label string, obj, off uintptr) {
 		return
 	}
 	print(" s.base()=", hex(s.base()), " s.limit=", hex(s.limit), " s.spanclass=", s.spanclass, " s.elemsize=", s.elemsize, " s.state=")
-	if 0 <= s.state && int(s.state) < len(mSpanStateNames) {
-		print(mSpanStateNames[s.state], "\n")
+	if state := s.state.get(); 0 <= state && int(state) < len(mSpanStateNames) {
+		print(mSpanStateNames[state], "\n")
 	} else {
-		print("unknown(", s.state, ")\n")
+		print("unknown(", state, ")\n")
 	}
 
 	skipped := false
 	size := s.elemsize
-	if s.state == mSpanManual && size == 0 {
+	if s.state.get() == mSpanManual && size == 0 {
 		// We're printing something from a stack frame. We
 		// don't know how big it is, so just show up to an
 		// including off.
@@ -1406,7 +1408,7 @@ var useCheckmark = false
 func initCheckmarks() {
 	useCheckmark = true
 	for _, s := range mheap_.allspans {
-		if s.state == mSpanInUse {
+		if s.state.get() == mSpanInUse {
 			heapBitsForAddr(s.base()).initCheckmarkSpan(s.layout())
 		}
 	}
@@ -1415,7 +1417,7 @@ func initCheckmarks() {
 func clearCheckmarks() {
 	useCheckmark = false
 	for _, s := range mheap_.allspans {
-		if s.state == mSpanInUse {
+		if s.state.get() == mSpanInUse {
 			heapBitsForAddr(s.base()).clearCheckmarkSpan(s.layout())
 		}
 	}
diff --git a/src/runtime/mgcsweep.go b/src/runtime/mgcsweep.go
index 5f1c90bfe0..580de7a715 100644
--- a/src/runtime/mgcsweep.go
+++ b/src/runtime/mgcsweep.go
@@ -114,12 +114,12 @@ func sweepone() uintptr {
 			atomic.Store(&mheap_.sweepdone, 1)
 			break
 		}
-		if s.state != mSpanInUse {
+		if state := s.state.get(); state != mSpanInUse {
 			// This can happen if direct sweeping already
 			// swept this span, but in that case the sweep
 			// generation should always be up-to-date.
 			if !(s.sweepgen == sg || s.sweepgen == sg+3) {
-				print("runtime: bad span s.state=", s.state, " s.sweepgen=", s.sweepgen, " sweepgen=", sg, "\n")
+				print("runtime: bad span s.state=", state, " s.sweepgen=", s.sweepgen, " sweepgen=", sg, "\n")
 				throw("non in-use span in unswept list")
 			}
 			continue
@@ -211,8 +211,8 @@ func (s *mspan) sweep(preserve bool) bool {
 		throw("mspan.sweep: m is not locked")
 	}
 	sweepgen := mheap_.sweepgen
-	if s.state != mSpanInUse || s.sweepgen != sweepgen-1 {
-		print("mspan.sweep: state=", s.state, " sweepgen=", s.sweepgen, " mheap.sweepgen=", sweepgen, "\n")
+	if state := s.state.get(); state != mSpanInUse || s.sweepgen != sweepgen-1 {
+		print("mspan.sweep: state=", state, " sweepgen=", s.sweepgen, " mheap.sweepgen=", sweepgen, "\n")
 		throw("mspan.sweep: bad span state")
 	}
 
@@ -351,8 +351,8 @@ func (s *mspan) sweep(preserve bool) bool {
 	if freeToHeap || nfreed == 0 {
 		// The span must be in our exclusive ownership until we update sweepgen,
 		// check for potential races.
-		if s.state != mSpanInUse || s.sweepgen != sweepgen-1 {
-			print("mspan.sweep: state=", s.state, " sweepgen=", s.sweepgen, " mheap.sweepgen=", sweepgen, "\n")
+		if state := s.state.get(); state != mSpanInUse || s.sweepgen != sweepgen-1 {
+			print("mspan.sweep: state=", state, " sweepgen=", s.sweepgen, " mheap.sweepgen=", sweepgen, "\n")
 			throw("mspan.sweep: bad span state after sweep")
 		}
 		// Serialization point.
diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go
index d9c8bbae7e..83ee310cda 100644
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -305,6 +305,14 @@ type arenaHint struct {
 // * During GC (gcphase != _GCoff), a span *must not* transition from
 //   manual or in-use to free. Because concurrent GC may read a pointer
 //   and then look up its span, the span state must be monotonic.
+//
+// Setting mspan.state to mSpanInUse or mSpanManual must be done
+// atomically and only after all other span fields are valid.
+// Likewise, if inspecting a span is contingent on it being
+// mSpanInUse, the state should be loaded atomically and checked
+// before depending on other fields. This allows the garbage collector
+// to safely deal with potentially invalid pointers, since resolving
+// such pointers may race with a span being allocated.
 type mSpanState uint8
 
 const (
@@ -323,6 +331,21 @@ var mSpanStateNames = []string{
 	"mSpanFree",
 }
 
+// mSpanStateBox holds an mSpanState and provides atomic operations on
+// it. This is a separate type to disallow accidental comparison or
+// assignment with mSpanState.
+type mSpanStateBox struct {
+	s mSpanState
+}
+
+func (b *mSpanStateBox) set(s mSpanState) {
+	atomic.Store8((*uint8)(&b.s), uint8(s))
+}
+
+func (b *mSpanStateBox) get() mSpanState {
+	return mSpanState(atomic.Load8((*uint8)(&b.s)))
+}
+
 // mSpanList heads a linked list of spans.
 //
 //go:notinheap
@@ -404,19 +427,19 @@ type mspan struct {
 	// h->sweepgen is incremented by 2 after every GC
 
 	sweepgen    uint32
-	divMul      uint16     // for divide by elemsize - divMagic.mul
-	baseMask    uint16     // if non-0, elemsize is a power of 2, & this will get object allocation base
-	allocCount  uint16     // number of allocated objects
-	spanclass   spanClass  // size class and noscan (uint8)
-	state       mSpanState // mspaninuse etc
-	needzero    uint8      // needs to be zeroed before allocation
-	divShift    uint8      // for divide by elemsize - divMagic.shift
-	divShift2   uint8      // for divide by elemsize - divMagic.shift2
-	scavenged   bool       // whether this span has had its pages released to the OS
-	elemsize    uintptr    // computed from sizeclass or from npages
-	limit       uintptr    // end of data in span
-	speciallock mutex      // guards specials list
-	specials    *special   // linked list of special records sorted by offset.
+	divMul      uint16        // for divide by elemsize - divMagic.mul
+	baseMask    uint16        // if non-0, elemsize is a power of 2, & this will get object allocation base
+	allocCount  uint16        // number of allocated objects
+	spanclass   spanClass     // size class and noscan (uint8)
+	state       mSpanStateBox // mSpanInUse etc; accessed atomically (get/set methods)
+	needzero    uint8         // needs to be zeroed before allocation
+	divShift    uint8         // for divide by elemsize - divMagic.shift
+	divShift2   uint8         // for divide by elemsize - divMagic.shift2
+	scavenged   bool          // whether this span has had its pages released to the OS
+	elemsize    uintptr       // computed from sizeclass or from npages
+	limit       uintptr       // end of data in span
+	speciallock mutex         // guards specials list
+	specials    *special      // linked list of special records sorted by offset.
 }
 
 func (s *mspan) base() uintptr {
@@ -483,7 +506,7 @@ func (h *mheap) coalesce(s *mspan) {
 		// The size is potentially changing so the treap needs to delete adjacent nodes and
 		// insert back as a combined node.
 		h.free.removeSpan(other)
-		other.state = mSpanDead
+		other.state.set(mSpanDead)
 		h.spanalloc.free(unsafe.Pointer(other))
 	}
 
@@ -525,7 +548,7 @@ func (h *mheap) coalesce(s *mspan) {
 
 	// Coalesce with earlier, later spans.
 	var hpBefore uintptr
-	if before := spanOf(s.base() - 1); before != nil && before.state == mSpanFree {
+	if before := spanOf(s.base() - 1); before != nil && before.state.get() == mSpanFree {
 		if s.scavenged == before.scavenged {
 			hpBefore = before.hugePages()
 			merge(before, s, before)
@@ -536,7 +559,7 @@ func (h *mheap) coalesce(s *mspan) {
 
 	// Now check to see if next (greater addresses) span is free and can be coalesced.
 	var hpAfter uintptr
-	if after := spanOf(s.base() + s.npages*pageSize); after != nil && after.state == mSpanFree {
+	if after := spanOf(s.base() + s.npages*pageSize); after != nil && after.state.get() == mSpanFree {
 		if s.scavenged == after.scavenged {
 			hpAfter = after.hugePages()
 			merge(s, after, after)
@@ -733,7 +756,7 @@ func inHeapOrStack(b uintptr) bool {
 	if s == nil || b < s.base() {
 		return false
 	}
-	switch s.state {
+	switch s.state.get() {
 	case mSpanInUse, mSpanManual:
 		return b < s.limit
 	default:
@@ -800,9 +823,12 @@ func spanOfUnchecked(p uintptr) *mspan {
 //go:nosplit
 func spanOfHeap(p uintptr) *mspan {
 	s := spanOf(p)
-	// If p is not allocated, it may point to a stale span, so we
-	// have to check the span's bounds and state.
-	if s == nil || p < s.base() || p >= s.limit || s.state != mSpanInUse {
+	// s is nil if it's never been allocated. Otherwise, we check
+	// its state first because we don't trust this pointer, so we
+	// have to synchronize with span initialization. Then, it's
+	// still possible we picked up a stale span pointer, so we
+	// have to check the span's bounds.
+	if s == nil || s.state.get() != mSpanInUse || p < s.base() || p >= s.limit {
 		return nil
 	}
 	return s
@@ -1042,7 +1068,6 @@ func (h *mheap) alloc_m(npage uintptr, spanclass spanClass, large bool) *mspan {
 		// able to map interior pointer to containing span.
 		atomic.Store(&s.sweepgen, h.sweepgen)
 		h.sweepSpans[h.sweepgen/2%2].push(s) // Add to swept in-use list.
-		s.state = mSpanInUse
 		s.allocCount = 0
 		s.spanclass = spanclass
 		s.elemsize = elemSize
@@ -1066,6 +1091,18 @@ func (h *mheap) alloc_m(npage uintptr, spanclass spanClass, large bool) *mspan {
 		s.gcmarkBits = gcmarkBits
 		s.allocBits = allocBits
 
+		// Now that the span is filled in, set its state. This
+		// is a publication barrier for the other fields in
+		// the span. While valid pointers into this span
+		// should never be visible until the span is returned,
+		// if the garbage collector finds an invalid pointer,
+		// access to the span may race with initialization of
+		// the span. We resolve this race by atomically
+		// setting the state after the span is fully
+		// initialized, and atomically checking the state in
+		// any situation where a pointer is suspect.
+		s.state.set(mSpanInUse)
+
 		// Mark in-use span in arena page bitmap.
 		arena, pageIdx, pageMask := pageIndexOf(s.base())
 		arena.pageInUse[pageIdx] |= pageMask
@@ -1143,13 +1180,13 @@ func (h *mheap) allocManual(npage uintptr, stat *uint64) *mspan {
 	lock(&h.lock)
 	s := h.allocSpanLocked(npage, stat)
 	if s != nil {
-		s.state = mSpanManual
 		s.manualFreeList = 0
 		s.allocCount = 0
 		s.spanclass = 0
 		s.nelems = 0
 		s.elemsize = 0
 		s.limit = s.base() + s.npages<<_PageShift
+		s.state.set(mSpanManual) // Publish the span
 		// Manually managed memory doesn't count toward heap_sys.
 		memstats.heap_sys -= uint64(s.npages << _PageShift)
 	}
@@ -1201,7 +1238,7 @@ func (h *mheap) allocSpanLocked(npage uintptr, stat *uint64) *mspan {
 
 HaveSpan:
 	s := t.span()
-	if s.state != mSpanFree {
+	if s.state.get() != mSpanFree {
 		throw("candidate mspan for allocation is not free")
 	}
 
@@ -1332,7 +1369,7 @@ func (h *mheap) growAddSpan(v unsafe.Pointer, size uintptr) {
 	s := (*mspan)(h.spanalloc.alloc())
 	s.init(uintptr(v), size/pageSize)
 	h.setSpans(s.base(), s.npages, s)
-	s.state = mSpanFree
+	s.state.set(mSpanFree)
 	// [v, v+size) is always in the Prepared state. The new span
 	// must be marked scavenged so the allocator transitions it to
 	// Ready when allocating from it.
@@ -1395,7 +1432,7 @@ func (h *mheap) freeManual(s *mspan, stat *uint64) {
 }
 
 func (h *mheap) freeSpanLocked(s *mspan, acctinuse, acctidle bool) {
-	switch s.state {
+	switch s.state.get() {
 	case mSpanManual:
 		if s.allocCount != 0 {
 			throw("mheap.freeSpanLocked - invalid stack free")
@@ -1420,7 +1457,7 @@ func (h *mheap) freeSpanLocked(s *mspan, acctinuse, acctidle bool) {
 	if acctidle {
 		memstats.heap_idle += uint64(s.npages << _PageShift)
 	}
-	s.state = mSpanFree
+	s.state.set(mSpanFree)
 
 	// Coalesce span with neighbors.
 	h.coalesce(s)
@@ -1481,7 +1518,7 @@ func (h *mheap) scavengeSplit(t treapIter, size uintptr) *mspan {
 		h.setSpan(n.base(), n)
 		h.setSpan(n.base()+nbytes-1, n)
 		n.needzero = s.needzero
-		n.state = s.state
+		n.state.set(s.state.get())
 	})
 	return n
 }
@@ -1580,7 +1617,6 @@ func (span *mspan) init(base uintptr, npages uintptr) {
 	span.allocCount = 0
 	span.spanclass = 0
 	span.elemsize = 0
-	span.state = mSpanDead
 	span.scavenged = false
 	span.speciallock.key = 0
 	span.specials = nil
@@ -1588,6 +1624,7 @@ func (span *mspan) init(base uintptr, npages uintptr) {
 	span.freeindex = 0
 	span.allocBits = nil
 	span.gcmarkBits = nil
+	span.state.set(mSpanDead)
 }
 
 func (span *mspan) inList() bool {
diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index 27552c9f33..e0757acbed 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -305,7 +305,7 @@ func sigFetchG(c *sigctxt) *g {
 			// work.
 			sp := getcallersp()
 			s := spanOf(sp)
-			if s != nil && s.state == mSpanManual && s.base() < sp && sp < s.limit {
+			if s != nil && s.state.get() == mSpanManual && s.base() < sp && sp < s.limit {
 				gp := *(**g)(unsafe.Pointer(s.base()))
 				return gp
 			}
diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index ecefce1e32..b87aa0d61b 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -219,7 +219,7 @@ func stackpoolalloc(order uint8) gclinkptr {
 // Adds stack x to the free pool. Must be called with stackpool[order].item.mu held.
 func stackpoolfree(x gclinkptr, order uint8) {
 	s := spanOfUnchecked(uintptr(x))
-	if s.state != mSpanManual {
+	if s.state.get() != mSpanManual {
 		throw("freeing stack not in a stack span")
 	}
 	if s.manualFreeList.ptr() == nil {
@@ -467,7 +467,7 @@ func stackfree(stk stack) {
 		}
 	} else {
 		s := spanOfUnchecked(uintptr(v))
-		if s.state != mSpanManual {
+		if s.state.get() != mSpanManual {
 			println(hex(s.base()), v)
 			throw("bad span state")
 		}
-- 
cgit v1.3