[release-branch.go1.16] runtime, time: disable preemption in addtimer

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

For #44868
Fixes #44869

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>
(cherry picked from commit aa26687e457d825fc9c580e8c029b768e0e70d38)
Reviewed-on: https://go-review.googlesource.com/c/go/+/300611

diff --git a/src/runtime/time.go b/src/runtime/time.go
index 8ab2a0343011d20022dcd0eff406bfb42e2d579e..dee6a674e4cdd277a9924abe27dfd1a389ba5759 100644 (file)
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -263,6 +263,9 @@ 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)
@@ -270,6 +273,8 @@ func addtimer(t *timer) {
        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
index 084ac33f51fd5c8046992ad110dcc7493ba99e4c..6ee0631a85564680bf91df0bc8e9d0ab0f3437a2 100644 (file)
--- 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