]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: incorporate Gscan acquire/release into lock ranking order
authorDan Scales <danscales@google.com>
Wed, 15 Apr 2020 19:35:24 +0000 (12:35 -0700)
committerDan Scales <danscales@google.com>
Thu, 7 May 2020 20:45:42 +0000 (20:45 +0000)
I added routines that can acquire/release a particular rank without
acquiring/releasing an associated lock. I added lockRankGscan as a rank
for acquiring/releasing the Gscan bit.

castogscanstatus() and casGtoPreemptScan() are acquires of the Gscan
bit. casfrom_Gscanstatus() is a release of the Gscan bit. casgstatus()
is like an acquire and release of the Gscan bit, since it will wait if
Gscan bit is currently set.

We have a cycle between hchan and Gscan. The acquisition of Gscan and
then hchan only happens in syncadjustsudogs() when the G is suspended,
so the main normal ordering (get hchan, then get Gscan) can't be
happening. So, I added a new rank lockRankHchanLeaf that is used when
acquiring hchan locks in syncadjustsudogs. This ranking is set so no
other locks can be acquired except other hchan locks.

Fixes #38922

Change-Id: I58ce526a74ba856cb42078f7b9901f2832e1d45c
Reviewed-on: https://go-review.googlesource.com/c/go/+/228417
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
src/runtime/lock_futex.go
src/runtime/lock_js.go
src/runtime/lock_sema.go
src/runtime/lockrank.go
src/runtime/lockrank_off.go
src/runtime/lockrank_on.go
src/runtime/proc.go
src/runtime/stack.go

index b0395d6a6979e1047b2df9833387861d9bf52bea..29b7be0d8f6e412c3207b1caaeac5231e0d97dfb 100644 (file)
@@ -108,7 +108,7 @@ func lock2(l *mutex) {
 }
 
 func unlock(l *mutex) {
-       lockRankRelease(l)
+       unlockWithRank(l)
 }
 
 func unlock2(l *mutex) {
index 7a720f4790ba8a3ff83107275e6b84f98c536f32..429ce639231f74b6fa621de73518b338b3501244 100644 (file)
@@ -44,7 +44,7 @@ func lock2(l *mutex) {
 }
 
 func unlock(l *mutex) {
-       lockRankRelease(l)
+       unlockWithRank(l)
 }
 
 func unlock2(l *mutex) {
index d79520da0778da4c8ade6938a63223d9eefbdee7..bf2584ac929fbe7337850c90637b21afdf3b75d2 100644 (file)
@@ -94,7 +94,7 @@ Loop:
 }
 
 func unlock(l *mutex) {
-       lockRankRelease(l)
+       unlockWithRank(l)
 }
 
 //go:nowritebarrier
index 5174adc8bff633c0692a06d60d47868a61cb42b2..899c4e2e852732ecb46ceb6ae76869e15d87b76b 100644 (file)
@@ -69,6 +69,7 @@ const (
        lockRankMcentral // For !go115NewMCentralImpl
        lockRankSpine    // For !go115NewMCentralImpl
        lockRankSpanSetSpine
+       lockRankGscan
        lockRankStackpool
        lockRankStackLarge
        lockRankDefer
@@ -84,6 +85,14 @@ const (
 
        // Other leaf locks
        lockRankGFree
+       // Generally, hchan must be acquired before gscan. But in one specific
+       // case (in syncadjustsudogs from markroot after the g has been suspended
+       // by suspendG), we allow gscan to be acquired, and then an hchan lock. To
+       // allow this case, we get this lockRankHchanLeaf rank in
+       // syncadjustsudogs(), rather than lockRankHchan. By using this special
+       // rank, we don't allow any further locks to be acquired other than more
+       // hchan locks.
+       lockRankHchanLeaf
 
        // Leaf locks with no dependencies, so these constants are not actually used anywhere.
        // There are other architecture-dependent leaf locks as well.
@@ -141,6 +150,7 @@ var lockNames = []string{
        lockRankMcentral:     "mcentral",
        lockRankSpine:        "spine",
        lockRankSpanSetSpine: "spanSetSpine",
+       lockRankGscan:        "gscan",
        lockRankStackpool:    "stackpool",
        lockRankStackLarge:   "stackLarge",
        lockRankDefer:        "defer",
@@ -152,7 +162,8 @@ var lockNames = []string{
 
        lockRankGlobalAlloc: "globalAlloc.mutex",
 
-       lockRankGFree: "gFree",
+       lockRankGFree:     "gFree",
+       lockRankHchanLeaf: "hchanLeaf",
 
        lockRankNewmHandoff:   "newmHandoff.lock",
        lockRankDebugPtrmask:  "debugPtrmask.lock",
@@ -217,16 +228,18 @@ var lockPartialOrder [][]lockRank = [][]lockRank{
        lockRankMcentral:     {lockRankScavenge, lockRankForcegc, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankHchan},
        lockRankSpine:        {lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSched, lockRankAllg, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankHchan},
        lockRankSpanSetSpine: {lockRankScavenge, lockRankForcegc, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankHchan},
-       lockRankStackpool:    {lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankPollDesc, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankFin, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankTrace, lockRankTraceStackTab, lockRankNetpollInit, lockRankRwmutexR, lockRankMcentral, lockRankSpine, lockRankSpanSetSpine},
-       lockRankStackLarge:   {lockRankAssistQueue, lockRankSched, lockRankItab, lockRankHchan, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankMcentral, lockRankSpanSetSpine},
+       lockRankGscan:        {lockRankScavenge, lockRankForcegc, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankFin, lockRankTraceBuf, lockRankTraceStrings, lockRankRoot, lockRankNotifyList, lockRankProf, lockRankGcBitsArenas, lockRankTrace, lockRankTraceStackTab, lockRankNetpollInit, lockRankMcentral, lockRankSpine, lockRankSpanSetSpine},
+       lockRankStackpool:    {lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankPollDesc, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankFin, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankTrace, lockRankTraceStackTab, lockRankNetpollInit, lockRankRwmutexR, lockRankMcentral, lockRankSpine, lockRankSpanSetSpine, lockRankGscan},
+       lockRankStackLarge:   {lockRankAssistQueue, lockRankSched, lockRankItab, lockRankHchan, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankMcentral, lockRankSpanSetSpine, lockRankGscan},
        lockRankDefer:        {},
        lockRankSudog:        {lockRankNotifyList, lockRankHchan},
-       lockRankWbufSpans:    {lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankSched, lockRankAllg, lockRankPollDesc, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankNotifyList, lockRankTraceStrings, lockRankMspanSpecial, lockRankProf, lockRankRoot, lockRankDefer, lockRankSudog},
-       lockRankMheap:        {lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankPollDesc, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankHchan, lockRankMspanSpecial, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankMcentral, lockRankStackpool, lockRankStackLarge, lockRankDefer, lockRankSudog, lockRankWbufSpans, lockRankSpanSetSpine},
+       lockRankWbufSpans:    {lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankSched, lockRankAllg, lockRankPollDesc, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankNotifyList, lockRankTraceStrings, lockRankMspanSpecial, lockRankProf, lockRankRoot, lockRankGscan, lockRankDefer, lockRankSudog},
+       lockRankMheap:        {lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankPollDesc, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankHchan, lockRankMspanSpecial, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankMcentral, lockRankGscan, lockRankStackpool, lockRankStackLarge, lockRankDefer, lockRankSudog, lockRankWbufSpans, lockRankSpanSetSpine},
        lockRankMheapSpecial: {lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankNotifyList, lockRankTraceBuf, lockRankTraceStrings, lockRankHchan},
        lockRankGlobalAlloc:  {lockRankProf, lockRankSpine, lockRankSpanSetSpine, lockRankMheap, lockRankMheapSpecial},
 
-       lockRankGFree: {lockRankSched},
+       lockRankGFree:     {lockRankSched},
+       lockRankHchanLeaf: {lockRankGscan, lockRankHchanLeaf},
 
        lockRankNewmHandoff:   {},
        lockRankDebugPtrmask:  {},
index fcfcff57a3920fae5a5268cab93d29fdf23d3a3d..891589c0f27772d9d8f3d974a1f5ac92293c25ba 100644 (file)
@@ -14,12 +14,18 @@ func getLockRank(l *mutex) lockRank {
        return 0
 }
 
-func lockRankRelease(l *mutex) {
+func lockWithRank(l *mutex, rank lockRank) {
+       lock2(l)
+}
+
+func acquireLockRank(rank lockRank) {
+}
+
+func unlockWithRank(l *mutex) {
        unlock2(l)
 }
 
-func lockWithRank(l *mutex, rank lockRank) {
-       lock2(l)
+func releaseLockRank(rank lockRank) {
 }
 
 func lockWithRankMayAcquire(l *mutex, rank lockRank) {
index fc72a06f6fd7585b71263ae70db5c59881efbd9d..cf4151ff4622aa01a6c7d62b6062d15aedc8483c 100644 (file)
@@ -46,10 +46,17 @@ func getLockRank(l *mutex) lockRank {
 // when acquiring a non-static lock.
 //go:nosplit
 func lockWithRank(l *mutex, rank lockRank) {
-       if l == &debuglock {
-               // debuglock is only used for println/printlock(). Don't do lock rank
-               // recording for it, since print/println are used when printing
-               // out a lock ordering problem below.
+       if l == &debuglock || l == &paniclk {
+               // debuglock is only used for println/printlock(). Don't do lock
+               // rank recording for it, since print/println are used when
+               // printing out a lock ordering problem below.
+               //
+               // paniclk has an ordering problem, since it can be acquired
+               // during a panic with any other locks held (especially if the
+               // panic is because of a directed segv), and yet also allg is
+               // acquired after paniclk in tracebackothers()). This is a genuine
+               // problem, so for now we don't do lock rank recording for paniclk
+               // either.
                lock2(l)
                return
        }
@@ -75,26 +82,49 @@ func lockWithRank(l *mutex, rank lockRank) {
        })
 }
 
+// acquireLockRank acquires a rank which is not associated with a mutex lock
+//go:nosplit
+func acquireLockRank(rank lockRank) {
+       gp := getg()
+       // Log the new class.
+       systemstack(func() {
+               i := gp.m.locksHeldLen
+               if i >= len(gp.m.locksHeld) {
+                       throw("too many locks held concurrently for rank checking")
+               }
+               gp.m.locksHeld[i].rank = rank
+               gp.m.locksHeld[i].lockAddr = 0
+               gp.m.locksHeldLen++
+
+               // i is the index of the lock being acquired
+               if i > 0 {
+                       checkRanks(gp, gp.m.locksHeld[i-1].rank, rank)
+               }
+       })
+}
+
+// checkRanks checks if goroutine g, which has mostly recently acquired a lock
+// with rank 'prevRank', can now acquire a lock with rank 'rank'.
 func checkRanks(gp *g, prevRank, rank lockRank) {
        rankOK := false
-       // If rank < prevRank, then we definitely have a rank error
-       if prevRank <= rank {
-               if rank == lockRankLeafRank {
-                       // If new lock is a leaf lock, then the preceding lock can
-                       // be anything except another leaf lock.
-                       rankOK = prevRank < lockRankLeafRank
-               } else {
-                       // We've already verified the total lock ranking, but we
-                       // also enforce the partial ordering specified by
-                       // lockPartialOrder as well. Two locks with the same rank
-                       // can only be acquired at the same time if explicitly
-                       // listed in the lockPartialOrder table.
-                       list := lockPartialOrder[rank]
-                       for _, entry := range list {
-                               if entry == prevRank {
-                                       rankOK = true
-                                       break
-                               }
+       if rank < prevRank {
+               // If rank < prevRank, then we definitely have a rank error
+               rankOK = false
+       } else if rank == lockRankLeafRank {
+               // If new lock is a leaf lock, then the preceding lock can
+               // be anything except another leaf lock.
+               rankOK = prevRank < lockRankLeafRank
+       } else {
+               // We've now verified the total lock ranking, but we
+               // also enforce the partial ordering specified by
+               // lockPartialOrder as well. Two locks with the same rank
+               // can only be acquired at the same time if explicitly
+               // listed in the lockPartialOrder table.
+               list := lockPartialOrder[rank]
+               for _, entry := range list {
+                       if entry == prevRank {
+                               rankOK = true
+                               break
                        }
                }
        }
@@ -109,11 +139,9 @@ func checkRanks(gp *g, prevRank, rank lockRank) {
 }
 
 //go:nosplit
-func lockRankRelease(l *mutex) {
-       if l == &debuglock {
-               // debuglock is only used for print/println. Don't do lock rank
-               // recording for it, since print/println are used when printing
-               // out a lock ordering problem below.
+func unlockWithRank(l *mutex) {
+       if l == &debuglock || l == &paniclk {
+               // See comment at beginning of lockWithRank.
                unlock2(l)
                return
        }
@@ -125,6 +153,7 @@ func lockRankRelease(l *mutex) {
                                found = true
                                copy(gp.m.locksHeld[i:gp.m.locksHeldLen-1], gp.m.locksHeld[i+1:gp.m.locksHeldLen])
                                gp.m.locksHeldLen--
+                               break
                        }
                }
                if !found {
@@ -135,6 +164,27 @@ func lockRankRelease(l *mutex) {
        })
 }
 
+// releaseLockRank releases a rank which is not associated with a mutex lock
+//go:nosplit
+func releaseLockRank(rank lockRank) {
+       gp := getg()
+       systemstack(func() {
+               found := false
+               for i := gp.m.locksHeldLen - 1; i >= 0; i-- {
+                       if gp.m.locksHeld[i].rank == rank && gp.m.locksHeld[i].lockAddr == 0 {
+                               found = true
+                               copy(gp.m.locksHeld[i:gp.m.locksHeldLen-1], gp.m.locksHeld[i+1:gp.m.locksHeldLen])
+                               gp.m.locksHeldLen--
+                               break
+                       }
+               }
+               if !found {
+                       println(gp.m.procid, ":", rank.String(), rank)
+                       throw("lockRank release without matching lockRank acquire")
+               }
+       })
+}
+
 //go:nosplit
 func lockWithRankMayAcquire(l *mutex, rank lockRank) {
        gp := getg()
index fe7da0bc871e96c7bebeb8e605c52e69ec4ecfe6..ca998702245f1809a45e5a7e28e1d807e758af84 100644 (file)
@@ -760,6 +760,7 @@ func casfrom_Gscanstatus(gp *g, oldval, newval uint32) {
                dumpgstatus(gp)
                throw("casfrom_Gscanstatus: gp->status is not in scan state")
        }
+       releaseLockRank(lockRankGscan)
 }
 
 // This will return false if the gp is not in the expected status and the cas fails.
@@ -771,7 +772,12 @@ func castogscanstatus(gp *g, oldval, newval uint32) bool {
                _Gwaiting,
                _Gsyscall:
                if newval == oldval|_Gscan {
-                       return atomic.Cas(&gp.atomicstatus, oldval, newval)
+                       r := atomic.Cas(&gp.atomicstatus, oldval, newval)
+                       if r {
+                               acquireLockRank(lockRankGscan)
+                       }
+                       return r
+
                }
        }
        print("runtime: castogscanstatus oldval=", hex(oldval), " newval=", hex(newval), "\n")
@@ -792,6 +798,9 @@ func casgstatus(gp *g, oldval, newval uint32) {
                })
        }
 
+       acquireLockRank(lockRankGscan)
+       releaseLockRank(lockRankGscan)
+
        // See https://golang.org/cl/21503 for justification of the yield delay.
        const yieldDelay = 5 * 1000
        var nextYield int64
@@ -842,6 +851,7 @@ func casGToPreemptScan(gp *g, old, new uint32) {
        if old != _Grunning || new != _Gscan|_Gpreempted {
                throw("bad g transition")
        }
+       acquireLockRank(lockRankGscan)
        for !atomic.Cas(&gp.atomicstatus, _Grunning, _Gscan|_Gpreempted) {
        }
 }
index 6e1f07bf73375ddc8e0b4f4dbb86644de18bf9b7..52e54171cbd595980c42054c771c6293c993513e 100644 (file)
@@ -794,7 +794,16 @@ func syncadjustsudogs(gp *g, used uintptr, adjinfo *adjustinfo) uintptr {
        var lastc *hchan
        for sg := gp.waiting; sg != nil; sg = sg.waitlink {
                if sg.c != lastc {
-                       lock(&sg.c.lock)
+                       // There is a ranking cycle here between gscan bit and
+                       // hchan locks. Normally, we only allow acquiring hchan
+                       // locks and then getting a gscan bit. In this case, we
+                       // already have the gscan bit. We allow acquiring hchan
+                       // locks here as a special case, since a deadlock can't
+                       // happen because the G involved must already be
+                       // suspended. So, we get a special hchan lock rank here
+                       // that is lower than gscan, but doesn't allow acquiring
+                       // any other locks other than hchan.
+                       lockWithRank(&sg.c.lock, lockRankHchanLeaf)
                }
                lastc = sg.c
        }