From 5a65189253bfe6f0f6c702cd0b2f5537f3686feb Mon Sep 17 00:00:00 2001 From: Adin Scannell Date: Tue, 9 Nov 2021 18:16:20 -0800 Subject: [PATCH] Standardize time.Listener API definitions. These definitions used nomenclature that did not mirror other notify APIs. Prior to building on these, standardize the definitions to align with other internal interfaces. Note that the Destroy interface method is dropped, as it was unused outside the channel notifier (which did not need it), and this will be removed soon. PiperOrigin-RevId: 408753196 --- pkg/sentry/fs/timerfd/timerfd.go | 7 +-- pkg/sentry/fsimpl/timerfd/timerfd.go | 9 ++-- pkg/sentry/kernel/posixtimer.go | 9 +--- pkg/sentry/kernel/task_sched.go | 8 +--- pkg/sentry/kernel/thread_group.go | 8 +--- pkg/sentry/kernel/time/BUILD | 2 +- pkg/sentry/kernel/time/time.go | 49 +++++++------------- pkg/sentry/kernel/time/{tcpip.go => util.go} | 29 ++++++------ pkg/sentry/kernel/timekeeper.go | 2 +- 9 files changed, 44 insertions(+), 79 deletions(-) rename pkg/sentry/kernel/time/{tcpip.go => util.go} (79%) diff --git a/pkg/sentry/fs/timerfd/timerfd.go b/pkg/sentry/fs/timerfd/timerfd.go index ca8be8683..457227954 100644 --- a/pkg/sentry/fs/timerfd/timerfd.go +++ b/pkg/sentry/fs/timerfd/timerfd.go @@ -141,12 +141,9 @@ func (t *TimerOperations) Write(context.Context, *fs.File, usermem.IOSequence, i return 0, linuxerr.EINVAL } -// Notify implements ktime.TimerListener.Notify. -func (t *TimerOperations) Notify(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { +// NotifyTimer implements ktime.TimerListener.NotifyTimer. +func (t *TimerOperations) NotifyTimer(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { atomic.AddUint64(&t.val, exp) t.events.Notify(waiter.ReadableEvents) return ktime.Setting{}, false } - -// Destroy implements ktime.TimerListener.Destroy. -func (t *TimerOperations) Destroy() {} diff --git a/pkg/sentry/fsimpl/timerfd/timerfd.go b/pkg/sentry/fsimpl/timerfd/timerfd.go index 68b785791..565dfa6a0 100644 --- a/pkg/sentry/fsimpl/timerfd/timerfd.go +++ b/pkg/sentry/fsimpl/timerfd/timerfd.go @@ -47,7 +47,7 @@ type TimerFileDescription struct { } var _ vfs.FileDescriptionImpl = (*TimerFileDescription)(nil) -var _ ktime.TimerListener = (*TimerFileDescription)(nil) +var _ ktime.Listener = (*TimerFileDescription)(nil) // New returns a new timer fd. func New(ctx context.Context, vfsObj *vfs.VirtualFilesystem, clock ktime.Clock, flags uint32) (*vfs.FileDescription, error) { @@ -136,12 +136,9 @@ func (tfd *TimerFileDescription) Release(context.Context) { tfd.timer.Destroy() } -// Notify implements ktime.TimerListener.Notify. -func (tfd *TimerFileDescription) Notify(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { +// NotifyTimer implements ktime.TimerListener.NotifyTimer. +func (tfd *TimerFileDescription) NotifyTimer(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { atomic.AddUint64(&tfd.val, exp) tfd.events.Notify(waiter.ReadableEvents) return ktime.Setting{}, false } - -// Destroy implements ktime.TimerListener.Destroy. -func (tfd *TimerFileDescription) Destroy() {} diff --git a/pkg/sentry/kernel/posixtimer.go b/pkg/sentry/kernel/posixtimer.go index 319754a42..07205cc68 100644 --- a/pkg/sentry/kernel/posixtimer.go +++ b/pkg/sentry/kernel/posixtimer.go @@ -115,8 +115,8 @@ func (it *IntervalTimer) signalRejectedLocked() { it.overrunCur++ } -// Notify implements ktime.TimerListener.Notify. -func (it *IntervalTimer) Notify(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { +// NotifyTimer implements ktime.TimerListener.NotifyTimer. +func (it *IntervalTimer) NotifyTimer(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { if it.target == nil { return ktime.Setting{}, false } @@ -151,11 +151,6 @@ func (it *IntervalTimer) Notify(exp uint64, setting ktime.Setting) (ktime.Settin return ktime.Setting{}, false } -// Destroy implements ktime.TimerListener.Destroy. Users of Timer should call -// DestroyTimer instead. -func (it *IntervalTimer) Destroy() { -} - // IntervalTimerCreate implements timer_create(2). func (t *Task) IntervalTimerCreate(c ktime.Clock, sigev *linux.Sigevent) (linux.TimerID, error) { t.tg.timerMu.Lock() diff --git a/pkg/sentry/kernel/task_sched.go b/pkg/sentry/kernel/task_sched.go index 9d9fa76a6..9882f1c12 100644 --- a/pkg/sentry/kernel/task_sched.go +++ b/pkg/sentry/kernel/task_sched.go @@ -352,8 +352,8 @@ func newKernelCPUClockTicker(k *Kernel) *kernelCPUClockTicker { } } -// Notify implements ktime.TimerListener.Notify. -func (ticker *kernelCPUClockTicker) Notify(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { +// NotifyTimer implements ktime.TimerListener.NotifyTimer. +func (ticker *kernelCPUClockTicker) NotifyTimer(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { // Only increment cpuClock by 1 regardless of the number of expirations. // This approximately compensates for cases where thread throttling or bad // Go runtime scheduling prevents the kernelCPUClockTicker goroutine, and @@ -472,10 +472,6 @@ func (ticker *kernelCPUClockTicker) Notify(exp uint64, setting ktime.Setting) (k return setting, false } -// Destroy implements ktime.TimerListener.Destroy. -func (ticker *kernelCPUClockTicker) Destroy() { -} - // randInt31n returns a random integer in [0, n). // // randInt31n is equivalent to math/rand.Rand.int31n(), which is unexported. diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 5814a4eca..64659d2ac 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -532,12 +532,8 @@ type itimerRealListener struct { tg *ThreadGroup } -// Notify implements ktime.TimerListener.Notify. -func (l *itimerRealListener) Notify(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { +// NotifyTimer implements ktime.TimerListener.NotifyTimer. +func (l *itimerRealListener) NotifyTimer(exp uint64, setting ktime.Setting) (ktime.Setting, bool) { l.tg.SendSignal(SignalInfoPriv(linux.SIGALRM)) return ktime.Setting{}, false } - -// Destroy implements ktime.TimerListener.Destroy. -func (l *itimerRealListener) Destroy() { -} diff --git a/pkg/sentry/kernel/time/BUILD b/pkg/sentry/kernel/time/BUILD index e293d9a0f..59fbcd9d6 100644 --- a/pkg/sentry/kernel/time/BUILD +++ b/pkg/sentry/kernel/time/BUILD @@ -6,8 +6,8 @@ go_library( name = "time", srcs = [ "context.go", - "tcpip.go", "time.go", + "util.go", ], visibility = ["//pkg/sentry:internal"], deps = [ diff --git a/pkg/sentry/kernel/time/time.go b/pkg/sentry/kernel/time/time.go index 191b92811..e7652282a 100644 --- a/pkg/sentry/kernel/time/time.go +++ b/pkg/sentry/kernel/time/time.go @@ -277,10 +277,10 @@ func (*ClockEventsQueue) Readiness(mask waiter.EventMask) waiter.EventMask { return 0 } -// A TimerListener receives expirations from a Timer. -type TimerListener interface { - // Notify is called when its associated Timer expires. exp is the number of - // expirations. setting is the next timer Setting. +// Listener receives expirations from a Timer. +type Listener interface { + // NotifyTimer is called when its associated Timer expires. exp is the number + // of expirations. setting is the next timer Setting. // // Notify is called with the associated Timer's mutex locked, so Notify // must not take any locks that precede Timer.mu in lock order. @@ -289,10 +289,7 @@ type TimerListener interface { // rather than the passed one. // // Preconditions: exp > 0. - Notify(exp uint64, setting Setting) (newSetting Setting, update bool) - - // Destroy is called when the timer is destroyed. - Destroy() + NotifyTimer(exp uint64, setting Setting) (newSetting Setting, update bool) } // Setting contains user-controlled mutable Timer properties. @@ -415,7 +412,7 @@ type Timer struct { clock Clock // listener is notified of expirations. listener is immutable. - listener TimerListener + listener Listener // mu protects the following mutable fields. mu sync.Mutex `state:"nosave"` @@ -449,7 +446,7 @@ const timerTickEvents = ClockEventSet | ClockEventRateIncrease // NewTimer returns a new Timer that will obtain time from clock and send // expirations to listener. The Timer is initially stopped and has no first // expiration or period configured. -func NewTimer(clock Clock, listener TimerListener) *Timer { +func NewTimer(clock Clock, listener Listener) *Timer { t := &Timer{ clock: clock, listener: listener, @@ -488,7 +485,6 @@ func (t *Timer) Destroy() { // before closing t.events to instruct the Timer goroutine to exit. t.clock.EventUnregister(&t.entry) close(t.events) - t.listener.Destroy() } func (t *Timer) runGoroutine() { @@ -517,7 +513,7 @@ func (t *Timer) Tick() { s, exp := t.setting.At(now) t.setting = s if exp > 0 { - if newS, ok := t.listener.Notify(exp, t.setting); ok { + if newS, ok := t.listener.NotifyTimer(exp, t.setting); ok { t.setting = newS } } @@ -574,7 +570,7 @@ func (t *Timer) Get() (Time, Setting) { s, exp := t.setting.At(now) t.setting = s if exp > 0 { - if newS, ok := t.listener.Notify(exp, t.setting); ok { + if newS, ok := t.listener.NotifyTimer(exp, t.setting); ok { t.setting = newS } } @@ -610,7 +606,7 @@ func (t *Timer) SwapAnd(s Setting, f func()) (Time, Setting) { } oldS, oldExp := t.setting.At(now) if oldExp > 0 { - t.listener.Notify(oldExp, oldS) + t.listener.NotifyTimer(oldExp, oldS) // N.B. The returned Setting doesn't matter because we're about // to overwrite. } @@ -620,7 +616,7 @@ func (t *Timer) SwapAnd(s Setting, f func()) (Time, Setting) { newS, newExp := s.At(now) t.setting = newS if newExp > 0 { - if newS, ok := t.listener.Notify(newExp, t.setting); ok { + if newS, ok := t.listener.NotifyTimer(newExp, t.setting); ok { t.setting = newS } } @@ -658,35 +654,26 @@ func (t *Timer) Clock() Clock { return t.clock } -// ChannelNotifier is a TimerListener that sends a message on an empty struct -// channel. +// ChannelNotifier is a Listener that sends on a channel. // // ChannelNotifier cannot be saved or loaded. -type ChannelNotifier struct { - // tchan must be a buffered channel. - tchan chan struct{} -} +type ChannelNotifier chan struct{} // NewChannelNotifier creates a new channel notifier. // // If the notifier is used with a timer, Timer.Destroy will close the channel // returned here. -func NewChannelNotifier() (TimerListener, <-chan struct{}) { +func NewChannelNotifier() (Listener, <-chan struct{}) { tchan := make(chan struct{}, 1) - return &ChannelNotifier{tchan}, tchan + return ChannelNotifier(tchan), tchan } -// Notify implements ktime.TimerListener.Notify. -func (c *ChannelNotifier) Notify(uint64, Setting) (Setting, bool) { +// NotifyTimer implements Listener.NotifyTimer. +func (c ChannelNotifier) NotifyTimer(uint64, Setting) (Setting, bool) { select { - case c.tchan <- struct{}{}: + case c <- struct{}{}: default: } return Setting{}, false } - -// Destroy implements ktime.TimerListener.Destroy and will close the channel. -func (c *ChannelNotifier) Destroy() { - close(c.tchan) -} diff --git a/pkg/sentry/kernel/time/tcpip.go b/pkg/sentry/kernel/time/util.go similarity index 79% rename from pkg/sentry/kernel/time/tcpip.go rename to pkg/sentry/kernel/time/util.go index c4474c0cf..3e487235f 100644 --- a/pkg/sentry/kernel/time/tcpip.go +++ b/pkg/sentry/kernel/time/util.go @@ -19,32 +19,32 @@ import ( "time" ) -// TcpipAfterFunc waits for duration to elapse according to clock then runs fn. +// AfterFunc waits for duration to elapse according to clock then runs fn. // The timer is started immediately and will fire exactly once. -func TcpipAfterFunc(clock Clock, duration time.Duration, fn func()) *TcpipTimer { - timer := &TcpipTimer{ +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 TimerListener.Notify(). + // 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 TimerListener.Notify() while holding + // 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 - // => TimerListener.Notify() + // => 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 - // => TimerListener.Notify() <- Launches T2 + // => Listener.NotifyTimer() <- Launches T2 // T2: // => Timer.Stop() // => Timer.Destroy() <- Timer.mu.Lock() called, blocks @@ -62,12 +62,12 @@ func TcpipAfterFunc(clock Clock, duration time.Duration, fn func()) *TcpipTimer return timer } -// TcpipTimer is a resettable timer with variable duration expirations. +// 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 TcpipTimer struct { +type VariableTimer struct { // clock is the time source. clock is immutable. clock Clock @@ -85,7 +85,7 @@ type TcpipTimer struct { } // Stop implements tcpip.Timer.Stop. -func (r *TcpipTimer) Stop() bool { +func (r *VariableTimer) Stop() bool { r.mu.Lock() defer r.mu.Unlock() @@ -99,7 +99,7 @@ func (r *TcpipTimer) Stop() bool { } // Reset implements tcpip.Timer.Reset. -func (r *TcpipTimer) Reset(d time.Duration) { +func (r *VariableTimer) Reset(d time.Duration) { r.mu.Lock() defer r.mu.Unlock() @@ -121,11 +121,8 @@ type functionNotifier struct { fn func() } -// Notify implements ktime.TimerListener.Notify. -func (f *functionNotifier) Notify(uint64, Setting) (Setting, bool) { +// NotifyTimer implements ktime.TimerListener.NotifyTimer. +func (f *functionNotifier) NotifyTimer(uint64, Setting) (Setting, bool) { f.fn() return Setting{}, false } - -// Destroy implements ktime.TimerListener.Destroy. -func (f *functionNotifier) Destroy() {} diff --git a/pkg/sentry/kernel/timekeeper.go b/pkg/sentry/kernel/timekeeper.go index 6255bae7a..a68e495f3 100644 --- a/pkg/sentry/kernel/timekeeper.go +++ b/pkg/sentry/kernel/timekeeper.go @@ -200,7 +200,7 @@ func (t *Timekeeper) NowMonotonic() tcpip.MonotonicTime { // AfterFunc implements tcpip.Clock. func (t *Timekeeper) AfterFunc(d time.Duration, f func()) tcpip.Timer { - return ktime.TcpipAfterFunc(t.realtimeClock, d, f) + return ktime.AfterFunc(t.realtimeClock, d, f) } // startUpdater starts an update goroutine that keeps the clocks updated.