From 7920b5b40a6a90a12cf9ffcc684d364e8c800771 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 11 Nov 2024 15:34:30 -0800 Subject: [PATCH] kernel: improve tcpip.Timer implementation - Move ktime.VariableTimer to kernel.timekeeperTcpipTimer, its only use case. This allows timekeeperTcpipTimer to use concrete types kernel.timekeeperClock and ktime.SampledTimer instead of ktime.Clock and ktime.Timer, saving a tiny amount of memory (interface values consist of two pointers) and CPU (for interface method calls). - Fix a bug where timekeeperTcpipTimer expiration can cancel a racing call to timekeeperTcpipTimer.Reset() (see use of new field timekeeperTcpipTimer.resets). - Define Listener.NotifyTimer directly on timekeeperTcpipTimer (dropping ktime.functionNotifier), and move goroutine spawning from the anonymous function in ktime.AfterFunc() into timekeeperTcpipTimer.NotifyTimer(). This slightly simplifies the control flow and saves an allocation for the anonymous function object. - Use monotonicClock rather than realtimeClock. It doesn't make sense for time-of-day clock adjustments to affect netstack timeouts, and this is consistent with tcpip.stdClock => time.AfterFunc => runtime.timer. PiperOrigin-RevId: 695504159 --- pkg/sentry/kernel/BUILD | 8 ++ pkg/sentry/kernel/kernel.go | 1 + pkg/sentry/kernel/timekeeper.go | 139 ++++++++++++++++++++++++++------ pkg/sentry/ktime/BUILD | 1 - pkg/sentry/ktime/util.go | 127 ----------------------------- 5 files changed, 122 insertions(+), 154 deletions(-) delete mode 100644 pkg/sentry/ktime/util.go diff --git a/pkg/sentry/kernel/BUILD b/pkg/sentry/kernel/BUILD index b40bd63c5..32e146ce8 100644 --- a/pkg/sentry/kernel/BUILD +++ b/pkg/sentry/kernel/BUILD @@ -105,6 +105,13 @@ declare_mutex( prefix = "threadGroupTimer", ) +declare_mutex( + name = "timekeeper_tcpip_timer_mutex", + out = "timekeeper_tcpip_timer_mutex.go", + package = "kernel", + prefix = "timekeeperTcpipTimer", +) + declare_mutex( name = "cgroup_mounts_mutex", out = "cgroup_mounts_mutex.go", @@ -290,6 +297,7 @@ go_library( "threads_impl.go", "timekeeper.go", "timekeeper_state.go", + "timekeeper_tcpip_timer_mutex.go", "tty.go", "user_counters_mutex.go", "uts_namespace.go", diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index b4ba2ca0c..2db357098 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -20,6 +20,7 @@ // // Kernel.extMu // TTY.mu +// timekeeperTcpipTimer.mu // ThreadGroup.timerMu // Locks acquired by ktime.Timer methods // TaskSet.mu diff --git a/pkg/sentry/kernel/timekeeper.go b/pkg/sentry/kernel/timekeeper.go index 339efc33a..75b2986d1 100644 --- a/pkg/sentry/kernel/timekeeper.go +++ b/pkg/sentry/kernel/timekeeper.go @@ -170,32 +170,6 @@ func (t *Timekeeper) SetClocks(c sentrytime.Clocks, params *VDSOParamPage) { } } -var _ tcpip.Clock = (*Timekeeper)(nil) - -// Now implements tcpip.Clock. -func (t *Timekeeper) Now() time.Time { - nsec, err := t.GetTime(sentrytime.Realtime) - if err != nil { - panic("timekeeper.GetTime(sentrytime.Realtime): " + err.Error()) - } - return time.Unix(0, nsec) -} - -// NowMonotonic implements tcpip.Clock. -func (t *Timekeeper) NowMonotonic() tcpip.MonotonicTime { - nsec, err := t.GetTime(sentrytime.Monotonic) - if err != nil { - panic("timekeeper.GetTime(sentrytime.Monotonic): " + err.Error()) - } - var mt tcpip.MonotonicTime - return mt.Add(time.Duration(nsec) * time.Nanosecond) -} - -// AfterFunc implements tcpip.Clock. -func (t *Timekeeper) AfterFunc(d time.Duration, f func()) tcpip.Timer { - return ktime.AfterFunc(t.realtimeClock, d, f) -} - // startUpdater starts an update goroutine that keeps the clocks updated. // // mu must be held. @@ -354,3 +328,116 @@ func (tc *timekeeperClock) Now() ktime.Time { func (tc *timekeeperClock) NewTimer(l ktime.Listener) ktime.Timer { return ktime.NewSampledTimer(tc, l) } + +var _ tcpip.Clock = (*Timekeeper)(nil) + +// Now implements tcpip.Clock. +func (t *Timekeeper) Now() time.Time { + nsec, err := t.GetTime(sentrytime.Realtime) + if err != nil { + panic("timekeeper.GetTime(sentrytime.Realtime): " + err.Error()) + } + return time.Unix(0, nsec) +} + +// NowMonotonic implements tcpip.Clock. +func (t *Timekeeper) NowMonotonic() tcpip.MonotonicTime { + nsec, err := t.GetTime(sentrytime.Monotonic) + if err != nil { + panic("timekeeper.GetTime(sentrytime.Monotonic): " + err.Error()) + } + var mt tcpip.MonotonicTime + return mt.Add(time.Duration(nsec) * time.Nanosecond) +} + +// AfterFunc implements tcpip.Clock. +func (t *Timekeeper) AfterFunc(d time.Duration, f func()) tcpip.Timer { + timer := &timekeeperTcpipTimer{ + clock: t.monotonicClock, + fn: f, + } + timer.Reset(d) + return timer +} + +// timekeeperTcpipTimer implements tcpip.Timer by wrapping a ktime.SampledTimer. +// tcpip.Timer does not define a Destroy method, so each timer expiration and +// each call to Timer.Stop() must release all resources by calling +// ktime.SampledTimer.Destroy(). +type timekeeperTcpipTimer struct { + // immutable + clock *timekeeperClock + fn func() + + // mu protects t. + mu timekeeperTcpipTimerMutex + + // t stores the latest running Timer. This is replaced whenever Reset is + // called since Timer cannot be restarted once it has been Destroyed by Stop. + // + // This field is nil iff Stop has been called. + t *ktime.SampledTimer + + // resets is the number of times Reset has been called. resets is written + // with both mu and ktime.SampledTimer locks held, so it may be read with + // either or both locks held. + resets int +} + +// Stop implements tcpip.Timer.Stop. +func (r *timekeeperTcpipTimer) Stop() bool { + r.mu.Lock() + defer r.mu.Unlock() + + if r.t == nil { + return false + } + _, lastSetting := r.t.Set(ktime.Setting{}, nil) + r.t.Destroy() + r.t = nil + return lastSetting.Enabled +} + +// stopExpired is equivalent to Stop, but is called when the timer expires. +func (r *timekeeperTcpipTimer) stopExpired(reset int) { + r.mu.Lock() + defer r.mu.Unlock() + + if r.t == nil || r.resets != reset { + return + } + r.t.Destroy() + r.t = nil +} + +// Reset implements tcpip.Timer.Reset. +func (r *timekeeperTcpipTimer) Reset(d time.Duration) { + r.mu.Lock() + defer r.mu.Unlock() + + if r.t == nil { + r.t = ktime.NewSampledTimer(r.clock, r) + } + r.t.Set(ktime.Setting{ + Enabled: true, + Next: r.clock.Now().Add(d), + }, r.incResets) +} + +func (r *timekeeperTcpipTimer) incResets() { + r.resets++ +} + +// NotifyTimer implements ktime.Listener.NotifyTimer. +func (r *timekeeperTcpipTimer) NotifyTimer(exp uint64) { + // Implementations of ktime.Listener.NotifyTimer() can't call Timer methods + // due to lock ordering, so we must call r.t.Destroy() from another + // goroutine. We also must call r.stopExpired() rather than r.Stop(), since + // the latter might cancel an unrelated call to r.Reset() that happens + // between now and when this goroutine runs. + thisReset := r.resets + go func() { + r.stopExpired(thisReset) + r.fn() + }() +} diff --git a/pkg/sentry/ktime/BUILD b/pkg/sentry/ktime/BUILD index d16dbe02a..9401f98c7 100644 --- a/pkg/sentry/ktime/BUILD +++ b/pkg/sentry/ktime/BUILD @@ -65,7 +65,6 @@ go_library( "synthetic_timer_list.go", "synthetic_timer_set.go", "uint64_range.go", - "util.go", ], visibility = ["//pkg/sentry:internal"], deps = [ diff --git a/pkg/sentry/ktime/util.go b/pkg/sentry/ktime/util.go deleted file mode 100644 index 1bddb027d..000000000 --- a/pkg/sentry/ktime/util.go +++ /dev/null @@ -1,127 +0,0 @@ -// Copyright 2020 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package ktime - -import ( - "sync" - "time" -) - -// AfterFunc waits for duration to elapse according to clock then runs fn. -// The timer is started immediately and will fire exactly once. -func AfterFunc(clock Clock, duration time.Duration, fn func()) *VariableTimer { - timer := &VariableTimer{ - clock: clock, - } - timer.notifier = functionNotifier{ - fn: func() { - // tcpip.Timer.Stop() explicitly states that the function is called in a - // separate goroutine that Stop() does not synchronize with. - // Timer.Destroy() synchronizes with calls to Listener.NotifyTimer(). - // This is semantically meaningful because, in the former case, it's - // legal to call tcpip.Timer.Stop() while holding locks that may also be - // taken by the function, but this isn't so in the latter case. Most - // immediately, Timer calls Listener.NotifyTimer() while holding - // Timer.mu. A deadlock occurs without spawning a goroutine: - // T1: (Timer expires) - // => Timer.Tick() <- Timer.mu.Lock() called - // => Listener.NotifyTimer() - // => Timer.Stop() - // => Timer.Destroy() <- Timer.mu.Lock() called, deadlock! - // - // Spawning a goroutine avoids the deadlock: - // T1: (Timer expires) - // => Timer.Tick() <- Timer.mu.Lock() called - // => Listener.NotifyTimer() <- Launches T2 - // T2: - // => Timer.Stop() - // => Timer.Destroy() <- Timer.mu.Lock() called, blocks - // T1: - // => (returns) <- Timer.mu.Unlock() called - // T2: - // => (continues) <- No deadlock! - go func() { - timer.Stop() - fn() - }() - }, - } - timer.Reset(duration) - return timer -} - -// VariableTimer is a resettable timer with variable duration expirations. -// Implements tcpip.Timer, which does not define a Destroy method; instead, all -// resources are released after timer expiration and calls to Timer.Stop. -// -// Must be created by AfterFunc. -type VariableTimer struct { - // clock is the time source. clock is immutable. - clock Clock - - // notifier is called when the Timer expires. notifier is immutable. - notifier functionNotifier - - // mu protects t. - mu sync.Mutex - - // t stores the latest running Timer. This is replaced whenever Reset is - // called since Timer cannot be restarted once it has been Destroyed by Stop. - // - // This field is nil iff Stop has been called. - t Timer -} - -// Stop implements tcpip.Timer.Stop. -func (r *VariableTimer) Stop() bool { - r.mu.Lock() - defer r.mu.Unlock() - - if r.t == nil { - return false - } - _, lastSetting := r.t.Set(Setting{}, nil) - r.t.Destroy() - r.t = nil - return lastSetting.Enabled -} - -// Reset implements tcpip.Timer.Reset. -func (r *VariableTimer) Reset(d time.Duration) { - r.mu.Lock() - defer r.mu.Unlock() - - if r.t == nil { - r.t = r.clock.NewTimer(&r.notifier) - } - - r.t.Set(Setting{ - Enabled: true, - Period: 0, - Next: r.clock.Now().Add(d), - }, nil) -} - -// functionNotifier is a TimerListener that runs a function. -// -// functionNotifier cannot be saved or loaded. -type functionNotifier struct { - fn func() -} - -// NotifyTimer implements ktime.TimerListener.NotifyTimer. -func (f *functionNotifier) NotifyTimer(uint64) { - f.fn() -}