runtime: release timersLock while running timer
Dan Scales pointed out a theoretical deadlock in the runtime.
The timer code runs timer functions while holding the timers lock for a P.
The scavenger queues up a timer function that calls wakeScavenger,
which acquires the scavenger lock.
The scavengeSleep function acquires the scavenger lock,
then calls resetTimer which can call addInitializedTimer
which acquires the timers lock for the current P.
So there is a potential deadlock, in that the scavenger lock and
the timers lock for some P may both be acquired in different order.
It's not clear to me whether this deadlock can ever actually occur.
Issue 35532 describes another possible deadlock.
The pollSetDeadline function acquires pd.lock for some poll descriptor,
and in some cases calls resettimer which can in some cases acquire
the timers lock for the current P.
The timer code runs timer functions while holding the timers lock for a P.
The timer function for poll descriptors winds up in netpolldeadlineimpl
which acquires pd.lock.
So again there is a potential deadlock, in that the pd lock for some
poll descriptor and the timers lock for some P may both be acquired in
different order. I think this can happen if we change the deadline
for a network connection exactly as the former deadline expires.
Looking at the code, I don't see any reason why we have to hold
the timers lock while running a timer function.
This CL implements that change.
Updates #6239
Updates #27707
Fixes #35532
Change-Id: I17792f5a0120e01ea07cf1b2de8434d5c10704dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/207348
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 34d5928..c648195 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2631,6 +2631,8 @@
rnow = nanotime()
}
for len(pp.timers) > 0 {
+ // Note that runtimer may temporarily unlock
+ // pp.timersLock.
if tw := runtimer(pp, rnow); tw != 0 {
if tw > 0 {
pollUntil = tw
diff --git a/src/runtime/time.go b/src/runtime/time.go
index 47b3262..ed044b3 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -1011,6 +1011,8 @@
// Returns 0 if it ran a timer, -1 if there are no more timers, or the time
// when the first timer should run.
// The caller must have locked the timers for pp.
+// If a timer is run, this will temporarily unlock the timers.
+//go:systemstack
func runtimer(pp *p, now int64) int64 {
for {
t := pp.timers[0]
@@ -1027,6 +1029,8 @@
if !atomic.Cas(&t.status, s, timerRunning) {
continue
}
+ // Note that runOneTimer may temporarily unlock
+ // pp.timersLock.
runOneTimer(pp, t, now)
return 0
@@ -1081,6 +1085,8 @@
// runOneTimer runs a single timer.
// The caller must have locked the timers for pp.
+// This will temporarily unlock the timers while running the timer function.
+//go:systemstack
func runOneTimer(pp *p, t *timer, now int64) {
if raceenabled {
if pp.timerRaceCtx == 0 {
@@ -1122,11 +1128,12 @@
gp.racectx = pp.timerRaceCtx
}
- // Note that since timers are locked here, f may not call
- // addtimer or resettimer.
+ unlock(&pp.timersLock)
f(arg, seq)
+ lock(&pp.timersLock)
+
if raceenabled {
gp := getg()
gp.racectx = 0
