runtime: fix timer race introduced in CL 573455

There is a short window when timers.adjust could miss a
timer update. Close that window. Does not change benchmark.

goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
                                   │ s7base.txt  │               s7.txt                │
                                   │   sec/op    │    sec/op     vs base               │
AdjustTimers10000-32                 239.9µ ± 5%   237.6µ ±  8%       ~ (p=0.631 n=10)
AdjustTimers10000SingleThread-32     1.686m ± 8%   1.710m ±  5%       ~ (p=0.481 n=10)
AdjustTimers10000NoReset-32          194.1µ ± 1%   190.8µ ±  2%  -1.69% (p=0.023 n=10)
AdjustTimers10000NoSleep-32          226.2µ ± 3%   222.9µ ±  3%       ~ (p=0.143 n=10)
AdjustTimers10000NoResetNoSleep-32   182.9µ ± 1%   180.9µ ±  2%       ~ (p=0.165 n=10)

goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
                                   │ m3base.txt  │               m3.txt               │
                                   │   sec/op    │   sec/op     vs base               │
AdjustTimers10000-12                 269.3µ ± 2%   267.0µ ± 2%       ~ (p=0.529 n=10)
AdjustTimers10000SingleThread-12     1.176m ± 1%   1.213m ± 1%  +3.15% (p=0.000 n=10)
AdjustTimers10000NoReset-12          262.6µ ± 2%   261.4µ ± 3%       ~ (p=0.123 n=10)
AdjustTimers10000NoSleep-12          247.8µ ± 1%   246.5µ ± 1%       ~ (p=0.393 n=10)
AdjustTimers10000NoResetNoSleep-12   231.0µ ± 1%   232.3µ ± 1%       ~ (p=0.684 n=10)

Change-Id: Ifdfcdd5a25046027912a8b306644bde7ec2d3214
Reviewed-on: https://go-review.googlesource.com/c/go/+/574741
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>

1 files changed, 12 insertions, 0 deletions

diff --git a/src/runtime/time.go b/src/runtime/time.go
index b696c837ab..f9335c95f8 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -526,6 +526,9 @@ func (t *timer) modify(when, period int64, f func(arg any, seq uintptr, delay in
 		// See comment in type timer above and in timers.adjust below.
 		if when < t.whenHeap {
 			wake = true
+			// Force timerModified bit out to t.astate before updating t.minWhenModified,
+			// to synchronize with t.ts.adjust. See comment in adjust.
+			t.astate.Store(t.state)
 			t.ts.updateMinWhenModified(when)
 		}
 	}
@@ -785,6 +788,15 @@ func (ts *timers) adjust(now int64, force bool) {
 	// The wakeTime method implementation reads minWhenModified *before* minWhenHeap,
 	// so that if the minWhenModified is observed to be 0, that means the minWhenHeap that
 	// follows will include the information that was zeroed out of it.
+	//
+	// Originally Step 3 locked every timer, which made sure any timer update that was
+	// already in progress during Steps 1+2 completed and was observed by Step 3.
+	// All that locking was too expensive, so now we do an atomic load of t.astate to
+	// decide whether we need to do a full lock. To make sure that we still observe any
+	// timer update already in progress during Steps 1+2, t.modify sets timerModified
+	// in t.astate *before* calling t.updateMinWhenModified. That ensures that the
+	// overwrite in Step 2 cannot lose an update: if it does overwrite an update, Step 3
+	// will see the timerModified and do a full lock.
 	ts.minWhenHeap.Store(ts.wakeTime())
 	ts.minWhenModified.Store(0)