runtime: use timer.lock in modtimer

Continue using timer.lock to simplify timer operations.

Note the removal of a previous potential deadlock.
(Explained at new line 325, there was a lock inversion
between individual timer locks and the 'timers' lock.)

[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]

Change-Id: I8c9be00d13c6acd171a8aa2882a4fc844498f754
Reviewed-on: https://go-review.googlesource.com/c/go/+/564125
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/runtime/time.go b/src/runtime/time.go
index e88c3f9b2ea7aad27aeed21356f15bf2eb15596e..5a8f516ccad2e24ecd55d97bbb14e12b77738423 100644 (file)
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -315,82 +315,49 @@ func modtimer(t *timer, when, period int64, f func(any, uintptr), arg any, seq u
                throw("timer period must be non-negative")
        }
 
-       status := uint32(timerRemoved)
-       wasRemoved := false
-       var pending bool
-       var mp *m
-loop:
-       for {
-               switch status = t.status.Load(); status {
-               case timerWaiting, timerModified, timerRemoved:
-                       // Prevent preemption while the timer is in timerLocked.
-                       // This could lead to a self-deadlock. See #38070.
-                       mp = acquirem()
-                       if !t.status.CompareAndSwap(status, timerLocked) {
-                               releasem(mp)
-                               break
-                       }
-                       switch status {
-                       case timerWaiting, timerModified:
-                               if status == timerModified && t.nextwhen == 0 {
-                                       t.pp.ptr().deletedTimers.Add(-1)
-                                       pending = false // timer already stopped
-                                       break loop
-                               }
-                               pending = true // timer not yet run
-                               break loop
-                       case timerRemoved:
-                               wasRemoved = true
-                               pending = false // timer already run or stopped
-                               break loop
-                       }
-               case timerLocked:
-                       // The timer is being run or modified, by a different P.
-                       // Wait for it to complete.
-                       osyield()
-               default:
-                       badTimer()
-               }
-       }
-
+       status, mp := t.lock()
        t.period = period
        t.f = f
        t.arg = arg
        t.seq = seq
 
-       if wasRemoved {
+       if status == timerRemoved {
+               // Set up t for insertion but unlock first,
+               // to avoid lock inversion with timers lock.
+               // Since t is not in a heap yet, nothing will
+               // find and modify it until after the doaddtimer.
                t.when = when
+               t.unlock(timerWaiting, mp)
+
                pp := getg().m.p.ptr()
                lock(&pp.timersLock)
                doaddtimer(pp, t)
                unlock(&pp.timersLock)
-               if !t.status.CompareAndSwap(timerLocked, timerWaiting) {
-                       badTimer()
-               }
-               releasem(mp)
                wakeNetPoller(when)
-       } else {
-               // The timer is in some other P's heap, so we can't change
-               // the when field. If we did, the other P's heap would
-               // be out of order. So we put the new when value in the
-               // nextwhen field, and let the other P set the when field
-               // when it is prepared to resort the heap.
-               t.nextwhen = when
-               earlier := when < t.when
-               if earlier {
-                       updateTimerModifiedEarliest(t.pp.ptr(), when)
-               }
+               return false
+       }
 
-               // Set the new status of the timer.
-               if !t.status.CompareAndSwap(timerLocked, timerModified) {
-                       badTimer()
-               }
-               releasem(mp)
+       pending := status == timerWaiting || status == timerModified && t.nextwhen != 0
+       if !pending {
+               t.pp.ptr().deletedTimers.Add(-1)
+       }
 
-               // If the new status is earlier, wake up the poller.
-               if earlier {
-                       wakeNetPoller(when)
-               }
+       // The timer is in some other P's heap, so we can't change
+       // the when field. If we did, the other P's heap would
+       // be out of order. So we put the new when value in the
+       // nextwhen field, and let the other P set the when field
+       // when it is prepared to resort the heap.
+       t.nextwhen = when
+       earlier := when < t.when
+       if earlier {
+               updateTimerModifiedEarliest(t.pp.ptr(), when)
+       }
+
+       t.unlock(timerModified, mp)
+
+       // If the new status is earlier, wake up the poller.
+       if earlier {
+               wakeNetPoller(when)
        }
 
        return pending