runtime: use timer.lock in moveTimers

Continue using timer.lock to simplify timer operations.

[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: Iaf371315308425d132217eacb20b1e120a6833c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/564127
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

1 files changed, 47 insertions, 48 deletions

diff --git a/src/runtime/time.go b/src/runtime/time.go
index 251fe8eb49..6c9333e55b 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -132,6 +132,8 @@ func (t *timer) lock() (status uint32, mp *m) {
 }
 
 // unlock unlocks the timer.
+// If mp == nil, the caller is responsible for calling
+// releasem(mp) with the mp returned by t.lock.
 func (t *timer) unlock(status uint32, mp *m) {
 	releaseLockRank(lockRankTimer)
 	if t.status.Load() != timerLocked {
@@ -141,7 +143,9 @@ func (t *timer) unlock(status uint32, mp *m) {
 		badTimer()
 	}
 	t.status.Store(status)
-	releasem(mp)
+	if mp != nil {
+		releasem(mp)
+	}
 }
 
 // maxWhen is the maximum value for timer's when field.
@@ -237,7 +241,8 @@ func goroutineReady(arg any, seq uintptr) {
 }
 
 // doaddtimer adds t to the current P's heap.
-// The caller must have locked the timers for pp.
+// The caller must have set t.pp = pp, unlocked t,
+// and then locked the timers for pp.
 func doaddtimer(pp *p, t *timer) {
 	// Timers rely on the network poller, so make sure the poller
 	// has started.
@@ -245,10 +250,9 @@ func doaddtimer(pp *p, t *timer) {
 		netpollGenericInit()
 	}
 
-	if t.pp != 0 {
-		throw("doaddtimer: P already set in timer")
+	if t.pp.ptr() != pp {
+		throw("doaddtimer: P not set in timer")
 	}
-	t.pp.set(pp)
 	i := len(pp.timers)
 	pp.timers = append(pp.timers, t)
 	siftupTimer(pp.timers, i)
@@ -327,12 +331,17 @@ func modtimer(t *timer, when, period int64, f func(any, uintptr), arg any, seq u
 		// 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()
+		t.pp.set(pp)
+		// pass mp=nil to t.unlock to avoid preemption
+		// between t.unlock and lock of timersLock.
+		// releasem done manually below
+		t.unlock(timerWaiting, nil)
+
 		lock(&pp.timersLock)
 		doaddtimer(pp, t)
 		unlock(&pp.timersLock)
+		releasem(mp)
 		wakeNetPoller(when)
 		return false
 	}
@@ -410,14 +419,16 @@ func cleantimers(pp *p) {
 		if t.nextwhen == 0 {
 			pp.deletedTimers.Add(-1)
 			status = timerRemoved
+			t.unlock(status, mp)
 		} else {
 			// Now we can change the when field.
 			t.when = t.nextwhen
+			t.pp.set(pp)
+			status = timerWaiting
+			t.unlock(status, mp)
 			// Move t to the right position.
 			doaddtimer(pp, t)
-			status = timerWaiting
 		}
-		t.unlock(status, mp)
 	}
 }
 
@@ -448,46 +459,32 @@ func adoptTimers(pp *p) {
 // is expected to have locked the timers for pp.
 func moveTimers(pp *p, timers []*timer) {
 	for _, t := range timers {
-	loop:
-		for {
-			switch s := t.status.Load(); s {
-			case timerWaiting:
-				if !t.status.CompareAndSwap(s, timerLocked) {
-					continue
-				}
-				t.pp = 0
+		status, mp := t.lock()
+		switch status {
+		case timerWaiting:
+			t.pp.set(pp)
+			// Unlock before add, to avoid append (allocation)
+			// while holding lock. This would be correct even if the world wasn't
+			// stopped (but it is), and it makes staticlockranking happy.
+			t.unlock(status, mp)
+			doaddtimer(pp, t)
+			continue
+		case timerModified:
+			t.pp = 0
+			if t.nextwhen != 0 {
+				t.when = t.nextwhen
+				status = timerWaiting
+				t.pp.set(pp)
+				t.unlock(status, mp)
 				doaddtimer(pp, t)
-				if !t.status.CompareAndSwap(timerLocked, timerWaiting) {
-					badTimer()
-				}
-				break loop
-			case timerModified:
-				if !t.status.CompareAndSwap(s, timerLocked) {
-					continue
-				}
-				t.pp = 0
-				if t.nextwhen != 0 {
-					t.when = t.nextwhen
-					doaddtimer(pp, t)
-					if !t.status.CompareAndSwap(timerLocked, timerWaiting) {
-						badTimer()
-					}
-				} else {
-					if !t.status.CompareAndSwap(timerLocked, timerRemoved) {
-						continue
-					}
-				}
-				break loop
-			case timerLocked:
-				// Loop until the modification is complete.
-				osyield()
-			case timerRemoved:
-				// We should not see these status values in a timers heap.
-				badTimer()
-			default:
-				badTimer()
+				continue
+			} else {
+				status = timerRemoved
 			}
+		case timerRemoved:
+			badTimer()
 		}
+		t.unlock(status, mp)
 	}
 }
 
@@ -666,12 +663,14 @@ Redo:
 		if t.nextwhen == 0 {
 			status = timerRemoved
 			pp.deletedTimers.Add(-1)
+			t.unlock(status, mp)
 		} else {
 			t.when = t.nextwhen
-			doaddtimer(pp, t)
+			t.pp.set(pp)
 			status = timerWaiting
+			t.unlock(status, mp)
+			doaddtimer(pp, t)
 		}
-		t.unlock(status, mp)
 		goto Redo
 	}