Skip to content

Commit 160577b

Browse files
committed
runtime: always acquire M when acquiring locks by rank
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 renaming acquireLockRank to acquireLockRankAndM and having it include a call to aquirem. Do the same for release. For golang#64706 For golang#66004 Change-Id: Id18e4d8de1036de743d2937fad002c6feebe2faf
1 parent 330bc95 commit 160577b

File tree

4 files changed

+22
-16
lines changed

4 files changed

+22
-16
lines changed

src/runtime/lockrank_off.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ func lockWithRank(l *mutex, rank lockRank) {
2727
// This function may be called in nosplit context and thus must be nosplit.
2828
//
2929
//go:nosplit
30-
func acquireLockRank(rank lockRank) {
30+
func acquireLockRankAndM(rank lockRank) {
31+
acquirem()
3132
}
3233

3334
func unlockWithRank(l *mutex) {
@@ -37,7 +38,8 @@ func unlockWithRank(l *mutex) {
3738
// This function may be called in nosplit context and thus must be nosplit.
3839
//
3940
//go:nosplit
40-
func releaseLockRank(rank lockRank) {
41+
func releaseLockRankAndM(rank lockRank) {
42+
releasem(getg().m)
4143
}
4244

4345
func lockWithRankMayAcquire(l *mutex, rank lockRank) {

src/runtime/lockrank_on.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,16 @@ func printHeldLocks(gp *g) {
104104
}
105105
}
106106

107-
// acquireLockRank acquires a rank which is not associated with a mutex lock
107+
// acquireLockRankAndM acquires a rank which is not associated with a mutex
108+
// lock. To maintain the invariant that an M with m.locks==0 does not hold any
109+
// lock-like resources, it also acquires the M.
108110
//
109111
// This function may be called in nosplit context and thus must be nosplit.
110112
//
111113
//go:nosplit
112-
func acquireLockRank(rank lockRank) {
114+
func acquireLockRankAndM(rank lockRank) {
115+
acquirem()
116+
113117
gp := getg()
114118
// Log the new class. See comment on lockWithRank.
115119
systemstack(func() {
@@ -189,12 +193,14 @@ func unlockWithRank(l *mutex) {
189193
})
190194
}
191195

192-
// releaseLockRank releases a rank which is not associated with a mutex lock
196+
// releaseLockRankAndM releases a rank which is not associated with a mutex
197+
// lock. To maintain the invariant that an M with m.locks==0 does not hold any
198+
// lock-like resources, it also releases the M.
193199
//
194200
// This function may be called in nosplit context and thus must be nosplit.
195201
//
196202
//go:nosplit
197-
func releaseLockRank(rank lockRank) {
203+
func releaseLockRankAndM(rank lockRank) {
198204
gp := getg()
199205
systemstack(func() {
200206
found := false
@@ -211,6 +217,8 @@ func releaseLockRank(rank lockRank) {
211217
throw("lockRank release without matching lockRank acquire")
212218
}
213219
})
220+
221+
releasem(getg().m)
214222
}
215223

216224
// nosplit because it may be called from nosplit contexts.

src/runtime/proc.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1067,7 +1067,7 @@ func casfrom_Gscanstatus(gp *g, oldval, newval uint32) {
10671067
dumpgstatus(gp)
10681068
throw("casfrom_Gscanstatus: gp->status is not in scan state")
10691069
}
1070-
releaseLockRank(lockRankGscan)
1070+
releaseLockRankAndM(lockRankGscan)
10711071
}
10721072

10731073
// This will return false if the gp is not in the expected status and the cas fails.
@@ -1081,7 +1081,7 @@ func castogscanstatus(gp *g, oldval, newval uint32) bool {
10811081
if newval == oldval|_Gscan {
10821082
r := gp.atomicstatus.CompareAndSwap(oldval, newval)
10831083
if r {
1084-
acquireLockRank(lockRankGscan)
1084+
acquireLockRankAndM(lockRankGscan)
10851085
}
10861086
return r
10871087

@@ -1110,8 +1110,7 @@ func casgstatus(gp *g, oldval, newval uint32) {
11101110
})
11111111
}
11121112

1113-
acquireLockRank(lockRankGscan)
1114-
releaseLockRank(lockRankGscan)
1113+
lockWithRankMayAcquire(nil, lockRankGscan)
11151114

11161115
// See https://golang.org/cl/21503 for justification of the yield delay.
11171116
const yieldDelay = 5 * 1000
@@ -1245,7 +1244,7 @@ func casGToPreemptScan(gp *g, old, new uint32) {
12451244
if old != _Grunning || new != _Gscan|_Gpreempted {
12461245
throw("bad g transition")
12471246
}
1248-
acquireLockRank(lockRankGscan)
1247+
acquireLockRankAndM(lockRankGscan)
12491248
for !gp.atomicstatus.CompareAndSwap(_Grunning, _Gscan|_Gpreempted) {
12501249
}
12511250
}

src/runtime/rwmutex.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,7 @@ func (rw *rwmutex) rlock() {
7272
// things blocking on the lock may consume all of the Ps and
7373
// deadlock (issue #20903). Alternatively, we could drop the P
7474
// while sleeping.
75-
acquirem()
76-
77-
acquireLockRank(rw.readRank)
75+
acquireLockRankAndM(rw.readRank)
7876
lockWithRankMayAcquire(&rw.rLock, getLockRank(&rw.rLock))
7977

8078
if rw.readerCount.Add(1) < 0 {
@@ -116,8 +114,7 @@ func (rw *rwmutex) runlock() {
116114
unlock(&rw.rLock)
117115
}
118116
}
119-
releaseLockRank(rw.readRank)
120-
releasem(getg().m)
117+
releaseLockRankAndM(rw.readRank)
121118
}
122119

123120
// lock locks rw for writing.

0 commit comments

Comments
 (0)