Skip to content

Commit 36a432f

Browse files
aclementsaclements
committed
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 golang#10958, golang#24543. Change-Id: Ia2ac8831b1bfda98d39bb30285e144c4f7eaf9ab Reviewed-on: https://go-review.googlesource.com/c/go/+/172982 Run-TryBot: Austin Clements <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Michael Knyszek <[email protected]>
1 parent 8c58615 commit 36a432f

5 files changed

Lines changed: 58 additions & 46 deletions

File tree

‎src/runtime/chan.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ func chansend(c *hchan, ep unsafe.Pointer, block bool, callerpc uintptr) bool {
249249
gp.waiting = mysg
250250
gp.param = nil
251251
c.sendq.enqueue(mysg)
252-
goparkunlock(&c.lock, waitReasonChanSend, traceEvGoBlockSend, 3)
252+
gopark(chanparkcommit, unsafe.Pointer(&c.lock), waitReasonChanSend, traceEvGoBlockSend, 2)
253253
// Ensure the value being sent is kept alive until the
254254
// receiver copies it out. The sudog has a pointer to the
255255
// 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 {
261261
throw("G waiting list is corrupted")
262262
}
263263
gp.waiting = nil
264+
gp.activeStackChans = false
264265
if gp.param == nil {
265266
if c.closed == 0 {
266267
throw("chansend: spurious wakeup")
@@ -559,13 +560,14 @@ func chanrecv(c *hchan, ep unsafe.Pointer, block bool) (selected, received bool)
559560
mysg.c = c
560561
gp.param = nil
561562
c.recvq.enqueue(mysg)
562-
goparkunlock(&c.lock, waitReasonChanReceive, traceEvGoBlockRecv, 3)
563+
gopark(chanparkcommit, unsafe.Pointer(&c.lock), waitReasonChanReceive, traceEvGoBlockRecv, 2)
563564

564565
// someone woke us up
565566
if mysg != gp.waiting {
566567
throw("G waiting list is corrupted")
567568
}
568569
gp.waiting = nil
570+
gp.activeStackChans = false
569571
if mysg.releasetime > 0 {
570572
blockevent(mysg.releasetime-t0, 2)
571573
}
@@ -632,6 +634,14 @@ func recv(c *hchan, sg *sudog, ep unsafe.Pointer, unlockf func(), skip int) {
632634
goready(gp, skip+1)
633635
}
634636

637+
func chanparkcommit(gp *g, chanLock unsafe.Pointer) bool {
638+
// There are unlocked sudogs that point into gp's stack. Stack
639+
// copying must lock the channels of those sudogs.
640+
gp.activeStackChans = true
641+
unlock((*mutex)(chanLock))
642+
return true
643+
}
644+
635645
// compiler implements
636646
//
637647
// select {

‎src/runtime/runtime2.go‎

Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -404,30 +404,36 @@ type g struct {
404404
stackguard0 uintptr // offset known to liblink
405405
stackguard1 uintptr // offset known to liblink
406406

407-
_panic *_panic // innermost panic - offset known to liblink
408-
_defer *_defer // innermost defer
409-
m *m // current m; offset known to arm liblink
410-
sched gobuf
411-
syscallsp uintptr // if status==Gsyscall, syscallsp = sched.sp to use during gc
412-
syscallpc uintptr // if status==Gsyscall, syscallpc = sched.pc to use during gc
413-
stktopsp uintptr // expected sp at top of stack, to check in traceback
414-
param unsafe.Pointer // passed parameter on wakeup
415-
atomicstatus uint32
416-
stackLock uint32 // sigprof/scang lock; TODO: fold in to atomicstatus
417-
goid int64
418-
schedlink guintptr
419-
waitsince int64 // approx time when the g become blocked
420-
waitreason waitReason // if status==Gwaiting
421-
preempt bool // preemption signal, duplicates stackguard0 = stackpreempt
422-
preemptStop bool // transition to _Gpreempted on preemption; otherwise, just deschedule
423-
paniconfault bool // panic (instead of crash) on unexpected fault address
424-
gcscandone bool // g has scanned stack; protected by _Gscan bit in status
425-
throwsplit bool // must not split stack
426-
raceignore int8 // ignore race detection events
427-
sysblocktraced bool // StartTrace has emitted EvGoInSyscall about this goroutine
428-
sysexitticks int64 // cputicks when syscall has returned (for tracing)
429-
traceseq uint64 // trace event sequencer
430-
tracelastp puintptr // last P emitted an event for this goroutine
407+
_panic *_panic // innermost panic - offset known to liblink
408+
_defer *_defer // innermost defer
409+
m *m // current m; offset known to arm liblink
410+
sched gobuf
411+
syscallsp uintptr // if status==Gsyscall, syscallsp = sched.sp to use during gc
412+
syscallpc uintptr // if status==Gsyscall, syscallpc = sched.pc to use during gc
413+
stktopsp uintptr // expected sp at top of stack, to check in traceback
414+
param unsafe.Pointer // passed parameter on wakeup
415+
atomicstatus uint32
416+
stackLock uint32 // sigprof/scang lock; TODO: fold in to atomicstatus
417+
goid int64
418+
schedlink guintptr
419+
waitsince int64 // approx time when the g become blocked
420+
waitreason waitReason // if status==Gwaiting
421+
preempt bool // preemption signal, duplicates stackguard0 = stackpreempt
422+
preemptStop bool // transition to _Gpreempted on preemption; otherwise, just deschedule
423+
paniconfault bool // panic (instead of crash) on unexpected fault address
424+
gcscandone bool // g has scanned stack; protected by _Gscan bit in status
425+
throwsplit bool // must not split stack
426+
// activeStackChans indicates that there are unlocked channels
427+
// pointing into this goroutine's stack. If true, stack
428+
// copying needs to acquire channel locks to protect these
429+
// areas of the stack.
430+
activeStackChans bool
431+
432+
raceignore int8 // ignore race detection events
433+
sysblocktraced bool // StartTrace has emitted EvGoInSyscall about this goroutine
434+
sysexitticks int64 // cputicks when syscall has returned (for tracing)
435+
traceseq uint64 // trace event sequencer
436+
tracelastp puintptr // last P emitted an event for this goroutine
431437
lockedm muintptr
432438
sig uint32
433439
writebuf []byte

‎src/runtime/select.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ func selunlock(scases []scase, lockorder []uint16) {
7575
}
7676

7777
func selparkcommit(gp *g, _ unsafe.Pointer) bool {
78+
// There are unlocked sudogs that point into gp's stack. Stack
79+
// copying must lock the channels of those sudogs.
80+
gp.activeStackChans = true
7881
// This must not access gp's stack (see gopark). In
7982
// particular, it must not access the *hselect. That's okay,
8083
// because by the time this is called, gp.waiting has all
@@ -311,6 +314,7 @@ loop:
311314
// wait for someone to wake us up
312315
gp.param = nil
313316
gopark(selparkcommit, nil, waitReasonSelect, traceEvGoBlockSelect, 1)
317+
gp.activeStackChans = false
314318

315319
sellock(scases, lockorder)
316320

‎src/runtime/sizeof_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ func TestSizeof(t *testing.T) {
2121
_32bit uintptr // size on 32bit platforms
2222
_64bit uintptr // size on 64bit platforms
2323
}{
24-
{runtime.G{}, 212, 368}, // g, but exported for testing
24+
{runtime.G{}, 216, 376}, // g, but exported for testing
2525
}
2626

2727
for _, tt := range tests {

‎src/runtime/stack.go‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -786,10 +786,6 @@ func syncadjustsudogs(gp *g, used uintptr, adjinfo *adjustinfo) uintptr {
786786
}
787787

788788
// Lock channels to prevent concurrent send/receive.
789-
// It's important that we *only* do this for async
790-
// copystack; otherwise, gp may be in the middle of
791-
// putting itself on wait queues and this would
792-
// self-deadlock.
793789
var lastc *hchan
794790
for sg := gp.waiting; sg != nil; sg = sg.waitlink {
795791
if sg.c != lastc {
@@ -826,12 +822,7 @@ func syncadjustsudogs(gp *g, used uintptr, adjinfo *adjustinfo) uintptr {
826822

827823
// Copies gp's stack to a new stack of a different size.
828824
// Caller must have changed gp status to Gcopystack.
829-
//
830-
// If sync is true, this is a self-triggered stack growth and, in
831-
// particular, no other G may be writing to gp's stack (e.g., via a
832-
// channel operation). If sync is false, copystack protects against
833-
// concurrent channel operations.
834-
func copystack(gp *g, newsize uintptr, sync bool) {
825+
func copystack(gp *g, newsize uintptr) {
835826
if gp.syscallsp != 0 {
836827
throw("stack growth not allowed in system call")
837828
}
@@ -857,15 +848,16 @@ func copystack(gp *g, newsize uintptr, sync bool) {
857848

858849
// Adjust sudogs, synchronizing with channel ops if necessary.
859850
ncopy := used
860-
if sync {
851+
if !gp.activeStackChans {
861852
adjustsudogs(gp, &adjinfo)
862853
} else {
863-
// sudogs can point in to the stack. During concurrent
864-
// shrinking, these areas may be written to. Find the
865-
// highest such pointer so we can handle everything
866-
// there and below carefully. (This shouldn't be far
867-
// from the bottom of the stack, so there's little
868-
// cost in handling everything below it carefully.)
854+
// sudogs may be pointing in to the stack and gp has
855+
// released channel locks, so other goroutines could
856+
// be writing to gp's stack. Find the highest such
857+
// pointer so we can handle everything there and below
858+
// carefully. (This shouldn't be far from the bottom
859+
// of the stack, so there's little cost in handling
860+
// everything below it carefully.)
869861
adjinfo.sghi = findsghi(gp, old)
870862

871863
// Synchronize with channel ops and copy the part of
@@ -1040,7 +1032,7 @@ func newstack() {
10401032

10411033
// The concurrent GC will not scan the stack while we are doing the copy since
10421034
// the gp is in a Gcopystack status.
1043-
copystack(gp, newsize, true)
1035+
copystack(gp, newsize)
10441036
if stackDebug >= 1 {
10451037
print("stack grow done\n")
10461038
}
@@ -1120,7 +1112,7 @@ func shrinkstack(gp *g) {
11201112
print("shrinking stack ", oldsize, "->", newsize, "\n")
11211113
}
11221114

1123-
copystack(gp, newsize, false)
1115+
copystack(gp, newsize)
11241116
}
11251117

11261118
// freeStackSpans frees unused stack spans at the end of GC.

0 commit comments

Comments
 (0)