Skip to content

Commit

Permalink
runtime: always acquire M when acquiring locks by rank
Browse files Browse the repository at this point in the history
Profiling of runtime-internal locks checks gp.m.locks to see if it's
safe to add a new record to the profile, but direct use of
acquireLockRank can change the list of the M's active lock ranks without
updating gp.m.locks to match. The runtime's internal rwmutex
implementation makes a point of calling acquirem/releasem when
manipulating the lock rank list, but the other user of acquireLockRank
(the GC's Gscan bit) relied on the GC's invariants to avoid deadlocks.

Codify the rwmutex approach by adding a variant of acquireLockRank,
acquireLockRankAndM, include a call to aquirem. Do the same for release.

Leave runtime/time.go's use of the old variants intact for the moment.

For golang#64706
For golang#66004

Change-Id: Id18e4d8de1036de743d2937fad002c6feebe2faf
  • Loading branch information
rhysh committed Mar 18, 2024
1 parent 34d28ba commit 1d02685
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 10 deletions.
20 changes: 20 additions & 0 deletions src/runtime/lockrank_off.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ func lockWithRank(l *mutex, rank lockRank) {
// This function may be called in nosplit context and thus must be nosplit.
//
//go:nosplit
func acquireLockRankAndM(rank lockRank) {
acquirem()
}

// This function may be called in nosplit context and thus must be nosplit.
//
// Deprecated: use acquireLockRankAndM instead to ensure that m.locks!=0 while
// appearing (to the static lock rank checker) to hold locks.
//
//go:nosplit
func acquireLockRank(rank lockRank) {
}

Expand All @@ -37,6 +47,16 @@ func unlockWithRank(l *mutex) {
// This function may be called in nosplit context and thus must be nosplit.
//
//go:nosplit
func releaseLockRankAndM(rank lockRank) {
releasem(getg().m)
}

// This function may be called in nosplit context and thus must be nosplit.
//
// Deprecated: use releaseLockRankAndM instead to ensure that m.locks!=0 while
// appearing (to the static lock rank checker) to hold locks.
//
//go:nosplit
func releaseLockRank(rank lockRank) {
}

Expand Down
30 changes: 30 additions & 0 deletions src/runtime/lockrank_on.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,23 @@ func printHeldLocks(gp *g) {
}
}

// acquireLockRankAndM acquires a rank which is not associated with a mutex
// lock. To maintain the invariant that an M with m.locks==0 does not hold any
// lock-like resources, it also acquires the M.
//
// This function may be called in nosplit context and thus must be nosplit.
//
//go:nosplit
func acquireLockRankAndM(rank lockRank) {
acquirem()
acquireLockRank(rank)
}

// acquireLockRank acquires a rank which is not associated with a mutex lock
//
// Deprecated: use acquireLockRankAndM instead to ensure that m.locks!=0 while
// appearing (to the static lock rank checker) to hold locks.
//
// This function may be called in nosplit context and thus must be nosplit.
//
//go:nosplit
Expand Down Expand Up @@ -189,8 +204,23 @@ func unlockWithRank(l *mutex) {
})
}

// releaseLockRankAndM releases a rank which is not associated with a mutex
// lock. To maintain the invariant that an M with m.locks==0 does not hold any
// lock-like resources, it also releases the M.
//
// This function may be called in nosplit context and thus must be nosplit.
//
//go:nosplit
func releaseLockRankAndM(rank lockRank) {
releaseLockRank(rank)
releasem(getg().m)
}

// releaseLockRank releases a rank which is not associated with a mutex lock
//
// Deprecated: use releaseLockRankAndM instead to ensure that m.locks!=0 while
// appearing (to the static lock rank checker) to hold locks.
//
// This function may be called in nosplit context and thus must be nosplit.
//
//go:nosplit
Expand Down
10 changes: 5 additions & 5 deletions src/runtime/proc.go
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,7 @@ func casfrom_Gscanstatus(gp *g, oldval, newval uint32) {
dumpgstatus(gp)
throw("casfrom_Gscanstatus: gp->status is not in scan state")
}
releaseLockRank(lockRankGscan)
releaseLockRankAndM(lockRankGscan)
}

// This will return false if the gp is not in the expected status and the cas fails.
Expand All @@ -1082,7 +1082,7 @@ func castogscanstatus(gp *g, oldval, newval uint32) bool {
if newval == oldval|_Gscan {
r := gp.atomicstatus.CompareAndSwap(oldval, newval)
if r {
acquireLockRank(lockRankGscan)
acquireLockRankAndM(lockRankGscan)
}
return r

Expand Down Expand Up @@ -1111,8 +1111,8 @@ func casgstatus(gp *g, oldval, newval uint32) {
})
}

acquireLockRank(lockRankGscan)
releaseLockRank(lockRankGscan)
acquireLockRankAndM(lockRankGscan)
releaseLockRankAndM(lockRankGscan)

// See https://golang.org/cl/21503 for justification of the yield delay.
const yieldDelay = 5 * 1000
Expand Down Expand Up @@ -1235,7 +1235,7 @@ func casGToPreemptScan(gp *g, old, new uint32) {
if old != _Grunning || new != _Gscan|_Gpreempted {
throw("bad g transition")
}
acquireLockRank(lockRankGscan)
acquireLockRankAndM(lockRankGscan)
for !gp.atomicstatus.CompareAndSwap(_Grunning, _Gscan|_Gpreempted) {
}
}
Expand Down
7 changes: 2 additions & 5 deletions src/runtime/rwmutex.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ func (rw *rwmutex) rlock() {
// things blocking on the lock may consume all of the Ps and
// deadlock (issue #20903). Alternatively, we could drop the P
// while sleeping.
acquirem()

acquireLockRank(rw.readRank)
acquireLockRankAndM(rw.readRank)
lockWithRankMayAcquire(&rw.rLock, getLockRank(&rw.rLock))

if rw.readerCount.Add(1) < 0 {
Expand Down Expand Up @@ -116,8 +114,7 @@ func (rw *rwmutex) runlock() {
unlock(&rw.rLock)
}
}
releaseLockRank(rw.readRank)
releasem(getg().m)
releaseLockRankAndM(rw.readRank)
}

// lock locks rw for writing.
Expand Down

0 comments on commit 1d02685

Please sign in to comment.