runtime, time: disable preemption in addtimer

prattmic · prattmic · commit aa26687e457d · 2021-03-10T21:49:24.000Z
The timerpMask optimization updates a mask of Ps (potentially) containing timers in pidleget / pidleput. For correctness, it depends on the assumption that new timers can only be added to a P's own heap. addtimer violates this assumption if it is preempted after computing pp. That G may then run on a different P, but adding a timer to the original P's heap. Avoid this by disabling preemption while pp is in use. Other uses of doaddtimer should be OK: * moveTimers: always moves to the current P's heap * modtimer, cleantimers, addAdjustedTimers, runtimer: does not add net new timers to the heap while locked Fixes #44868 Change-Id: I4a5d080865e854931d0a3a09a51ca36879101d72 Reviewed-on: https://go-review.googlesource.com/c/go/+/300610 Trust: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Pratt <mpratt@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org>
diff --git a/src/runtime/time.go b/src/runtime/time.go
@@ -263,13 +263,18 @@ func addtimer(t *timer) {
 
 	when := t.when
 
+	// Disable preemption while using pp to avoid changing another P's heap.
+	mp := acquirem()
+
 	pp := getg().m.p.ptr()
 	lock(&pp.timersLock)
 	cleantimers(pp)
 	doaddtimer(pp, t)
 	unlock(&pp.timersLock)
 
 	wakeNetPoller(when)
+
+	releasem(mp)
 }
 
 // doaddtimer adds t to the current P's heap.
diff --git a/src/time/sleep_test.go b/src/time/sleep_test.go
@@ -511,6 +511,22 @@ func TestZeroTimerStopPanics(t *testing.T) {
 	tr.Stop()
 }
 
+// Test that zero duration timers aren't missed by the scheduler. Regression test for issue 44868.
+func TestZeroTimer(t *testing.T) {
+	if testing.Short() {
+		t.Skip("-short")
+	}
+
+	for i := 0; i < 1000000; i++ {
+		s := Now()
+		ti := NewTimer(0)
+		<-ti.C
+		if diff := Since(s); diff > 2*Second {
+			t.Errorf("Expected time to get value from Timer channel in less than 2 sec, took %v", diff)
+		}
+	}
+}
+
 // Benchmark timer latency when the thread that creates the timer is busy with
 // other work and the timers must be serviced by other threads.
 // https://golang.org/issue/38860
