From e2f6a58f3041248167f4cde3dd51ffe4085168a2 Mon Sep 17 00:00:00 2001 From: lyphyser Date: Thu, 9 Jul 2026 16:13:51 -0700 Subject: [PATCH] kernel: park Timekeeper when idle AI usage: written by Claude with a TON of back and forth and rewrites until we landed on this relatively minimal design; reviewed all the code in detail Currently the Timekeeper unconditionally wakes up every minute even when idle. This PR adds a mechanism to park it and to synchronously calibrate when the timekeeper is parked and either a task starts running or GetTime is called. This is achieved using a reference count mechanism that keeps it alive until tasks running becomes 0 and for the GetTime call duration. The parking mechanism works by first transitioning to idle if no reference are taken when the timer fires, and then if no tasks run and no GetTime calls are made for a whole interval (one second) then it transitions to parked and stops rearming the timer. This design means that there is no behavior change in the non-idle case, and makes everything much simpler. Note that I think there is a pre-existing bug in the codebase, which is that GetTime can be called while the timekeeper is stopped, because it is stopped before netstack during save. Once this is fixed, then the code in this commit can be adjust to panic if anything tries to take a reference to the Timekeeper while stopped instead of silently allowing it with miscalibrated clocks. FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/gvisor/pull/13533 PiperOrigin-RevId: 945368458 --- pkg/sentry/kernel/kernel.go | 9 ++ pkg/sentry/kernel/timekeeper.go | 190 +++++++++++++++++++---- pkg/sentry/kernel/timekeeper_test.go | 4 +- pkg/sentry/time/calibrated_clock.go | 28 ++-- pkg/sentry/time/calibrated_clock_test.go | 12 +- pkg/sentry/time/clocks.go | 4 +- 6 files changed, 196 insertions(+), 51 deletions(-) diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index b2a98d5fa0..dee5ea4c60 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -28,6 +28,7 @@ // Task.mu // FSContext.mu // runningTasksMu +// Timekeeper.updateMu // // Locking SignalHandlers.mu in multiple SignalHandlers requires locking // TaskSet.mu exclusively first. Locking Task.mu in multiple Tasks at the same @@ -1607,6 +1608,11 @@ func (k *Kernel) incRunningTasks() { close(k.taskActivityCh) k.taskActivityCh = make(chan struct{}) + // take a timekeeper reference that says alive until we transition + // back from 1 to 0 tasks so vDSO stays updated; this also updates + // the vDSO before returning if it was parked + k.timekeeper.addRef() + // This store must happen after the increment of k.cpuClock above to ensure // that concurrent calls to Task.accountTaskGoroutineLeave() also observe // the updated k.cpuClock. @@ -1621,6 +1627,9 @@ func (k *Kernel) decRunningTasks() { if tasks < 0 { panic(fmt.Sprintf("Invalid running count %d", tasks)) } + if tasks == 0 { + k.timekeeper.release() + } // Nothing to do. The next CPU clock tick will disable the timer if // there is still nothing running. This provides approximately one tick diff --git a/pkg/sentry/kernel/timekeeper.go b/pkg/sentry/kernel/timekeeper.go index 75b2986d1d..fe27e2248d 100644 --- a/pkg/sentry/kernel/timekeeper.go +++ b/pkg/sentry/kernel/timekeeper.go @@ -26,6 +26,21 @@ import ( "gvisor.dev/gvisor/pkg/tcpip" ) +// Possible values of Timekeeper.updaterState +const ( + // manually stopped + updaterStopped int32 = iota + + // timer not armed due to being idle for a whole interval + updaterParked + + // set when timer triggers if no tasks are running; any task running + // or internal timekeeper usage transitions back to updaterActive + updaterIdle + + updaterActive +) + // Timekeeper manages all of the kernel clocks. // // +stateify savable @@ -86,6 +101,26 @@ type Timekeeper struct { // wg is used to indicate that the update goroutine has exited. wg sync.WaitGroup `state:"nosave"` + + // atomic enum with updater* values + // + // the goroutine moves it active->idle->parked (under updateMu) + // GetTime or 0->1 tasks moves to active if not stopped (lockless) + // startUpdater/stopUpdater move in/out of stopped (under updateMu) + updaterState atomicbitops.Int32 `state:"nosave"` + + // (runningTasks > 0 ? 1 : 0) + number_of_tasks_running_GetTime + refs atomicbitops.Int64 `state:"nosave"` + + // guards updates of the vDSO parameter page and of updaterState + // except for raising updaterState to active on usage + updateMu sync.Mutex `state:"nosave"` + + // timer to periodically update the time calibration + timer *time.Timer `state:"nosave"` + + // vDSO parameter page kept updated by the Timekeeper mechanism + params *VDSOParamPage `state:"nosave"` } // NewTimekeeper returns a Timekeeper that is automatically kept up-to-date. @@ -170,6 +205,35 @@ func (t *Timekeeper) SetClocks(c sentrytime.Clocks, params *VDSOParamPage) { } } +// update samples the backing clocks and writes the new parameters to the VDSO +// parameter page. +// +// Preconditions: updateMu must be held +func (t *Timekeeper) update(parked bool) { + // Call Update within a Write block to prevent the VDSO from using the old + // params between Update and Write. + if err := t.params.Write(func() vdsoParams { + monotonicParams, monotonicOk, realtimeParams, realtimeOk := t.clocks.Update(parked) + + var p vdsoParams + if monotonicOk { + p.monotonicReady = 1 + p.monotonicBaseCycles = int64(monotonicParams.BaseCycles) + p.monotonicBaseRef = int64(monotonicParams.BaseRef) + t.monotonicOffset + p.monotonicFrequency = monotonicParams.Frequency + } + if realtimeOk { + p.realtimeReady = 1 + p.realtimeBaseCycles = int64(realtimeParams.BaseCycles) + p.realtimeBaseRef = int64(realtimeParams.BaseRef) + p.realtimeFrequency = realtimeParams.Frequency + } + return p + }); err != nil { + log.Warningf("Unable to update VDSO parameter page: %v", err) + } +} + // startUpdater starts an update goroutine that keeps the clocks updated. // // mu must be held. @@ -179,6 +243,7 @@ func (t *Timekeeper) startUpdater(params *VDSOParamPage) { return } t.stop = make(chan struct{}) + t.params = params // Keep the clocks up to date. // @@ -186,47 +251,97 @@ func (t *Timekeeper) startUpdater(params *VDSOParamPage) { // timer, so it may run at a *slightly* different rate from the // application CLOCK_MONOTONIC. That is fine, as we only need to update // at approximately this rate. - timer := time.NewTicker(sentrytime.ApproxUpdateInterval) + t.timer = time.NewTimer(sentrytime.ApproxUpdateInterval) + t.timer.Stop() + + t.updateMu.Lock() + // store-then-load to synchronize with addRef + t.updaterState.Store(updaterParked) + if t.refs.Load() != 0 { + t.timer.Reset(sentrytime.ApproxUpdateInterval) + t.update(true) + t.updaterState.Store(updaterActive) + } + t.updateMu.Unlock() + t.wg.Add(1) go func() { // S/R-SAFE: stopped during save. defer t.wg.Done() + defer t.timer.Stop() for { - // Start with an update immediately, so the clocks are - // ready ASAP. - - // Call Update within a Write block to prevent the VDSO - // from using the old params between Update and - // Write. - if err := params.Write(func() vdsoParams { - monotonicParams, monotonicOk, realtimeParams, realtimeOk := t.clocks.Update() - - var p vdsoParams - if monotonicOk { - p.monotonicReady = 1 - p.monotonicBaseCycles = int64(monotonicParams.BaseCycles) - p.monotonicBaseRef = int64(monotonicParams.BaseRef) + t.monotonicOffset - p.monotonicFrequency = monotonicParams.Frequency - } - if realtimeOk { - p.realtimeReady = 1 - p.realtimeBaseCycles = int64(realtimeParams.BaseCycles) - p.realtimeBaseRef = int64(realtimeParams.BaseRef) - p.realtimeFrequency = realtimeParams.Frequency - } - return p - }); err != nil { - log.Warningf("Unable to update VDSO parameter page: %v", err) - } - + // wait until next tick or if parked until addRef is called select { - case <-timer.C: + case <-t.timer.C: case <-t.stop: return } + + t.updateMu.Lock() + + switch t.updaterState.Load() { + case updaterStopped: + t.updateMu.Unlock() + return + + case updaterActive: + // store-then-load to synchronize with addRef + t.updaterState.Store(updaterIdle) + if t.refs.Load() != 0 { + t.updaterState.Store(updaterActive) + } + + case updaterIdle: + // no Timekeeper usage for a whole interval: park + // store-then-load to synchronize with addRef + t.updaterState.Store(updaterParked) + if t.refs.Load() != 0 { + t.updaterState.Store(updaterActive) + } else { + t.updateMu.Unlock() + continue + } + } + + t.timer.Reset(sentrytime.ApproxUpdateInterval) + t.update(false) + t.updateMu.Unlock() } }() } +// add a reference to the Timekeeper: as long as there are references, +// it doesn't park. Call release() to release. +func (t *Timekeeper) addRef() { + // store-then-load to synchronize with the goroutine + t.refs.Add(1) + switch t.updaterState.Load() { + case updaterActive: + case updaterIdle: + // this is enough because the goroutine will + // itself recheck refs after parking + t.updaterState.CompareAndSwap(updaterIdle, updaterActive) + case updaterParked: + t.updateMu.Lock() + cur := t.updaterState.Load() + if cur == updaterParked { + t.timer.Reset(sentrytime.ApproxUpdateInterval) + t.update(true) + } + if cur != updaterStopped { + t.updaterState.Store(updaterActive) + } + t.updateMu.Unlock() + case updaterStopped: + } +} + +// release drops a reference taken by addRef; when refs are 0 the timekeeper can park +func (t *Timekeeper) release() { + if t.refs.Add(-1) < 0 { + panic("Timekeeper.release called with no reference held") + } +} + // stopUpdater stops the update goroutine, blocking until it exits. // // mu must be held. @@ -236,9 +351,17 @@ func (t *Timekeeper) stopUpdater() { return } + // lock to ensure it can't be taken out of stopped by non-startUpdater code + t.updateMu.Lock() + t.updaterState.Store(updaterStopped) + t.updateMu.Unlock() + + // this wakes up the goroutine close(t.stop) t.wg.Wait() t.stop = nil + t.timer = nil + t.params = nil } // Destroy destroys the Timekeeper, freeing all associated resources. @@ -250,7 +373,8 @@ func (t *Timekeeper) Destroy() { } // PauseUpdates stops clock parameter updates. This should only be used when -// Tasks are not running and thus cannot access the clock. +// Tasks are not running and thus cannot access the clock and when +// GetTime is not being called internally func (t *Timekeeper) PauseUpdates() { t.mu.Lock() defer t.mu.Unlock() @@ -272,6 +396,12 @@ func (t *Timekeeper) GetTime(c sentrytime.ClockID) (int64, error) { } <-t.restored } + + // update the calibration if needed and keep the timekeeper calibrated + // during the read + t.addRef() + defer t.release() + now, err := t.clocks.GetTime(c) if err == nil && c == sentrytime.Monotonic { now += t.monotonicOffset diff --git a/pkg/sentry/kernel/timekeeper_test.go b/pkg/sentry/kernel/timekeeper_test.go index e27da18eb8..2014067e2a 100644 --- a/pkg/sentry/kernel/timekeeper_test.go +++ b/pkg/sentry/kernel/timekeeper_test.go @@ -33,11 +33,11 @@ type mockClocks struct { } // Update implements sentrytime.Clocks.Update. It does nothing. -func (*mockClocks) Update() (monotonicParams sentrytime.Parameters, monotonicOk bool, realtimeParam sentrytime.Parameters, realtimeOk bool) { +func (*mockClocks) Update(parked bool) (monotonicParams sentrytime.Parameters, monotonicOk bool, realtimeParam sentrytime.Parameters, realtimeOk bool) { return } -// Update implements sentrytime.Clocks.GetTime. +// GetTime implements sentrytime.Clocks.GetTime. func (c *mockClocks) GetTime(id sentrytime.ClockID) (int64, error) { switch id { case sentrytime.Monotonic: diff --git a/pkg/sentry/time/calibrated_clock.go b/pkg/sentry/time/calibrated_clock.go index 47df5d0a19..a73e1b8f40 100644 --- a/pkg/sentry/time/calibrated_clock.go +++ b/pkg/sentry/time/calibrated_clock.go @@ -107,14 +107,15 @@ func (c *CalibratedClock) resetLocked(str string, v ...any) { // may need to be adjusted slightly from these values to compensate for error. // // Preconditions: c.mu must be held for writing. -func (c *CalibratedClock) updateParams(actual Parameters) { - if !c.ready { - // At initial calibration there is nothing to correct. +func (c *CalibratedClock) updateParams(actual Parameters, parked bool) { + if !c.ready || parked { + // when parked nothing has read the time for a whole interval, so + // assume errors have been compensated and recalibrate from scratch c.params = actual - c.ready = true - - c.Infof("ready") - + if !c.ready { + c.ready = true + c.Infof("ready") + } return } @@ -148,9 +149,12 @@ func (c *CalibratedClock) updateParams(actual Parameters) { // the clock is calibrated. Update should be called regularly to prevent the // clock from getting significantly out of sync from the reference clock. // +// parked indicates that this Update resumes a parked (unobserved) period; see +// updateParams. +// // The returned timekeeping parameters are invalidated on the next call to // Update. -func (c *CalibratedClock) Update() (Parameters, bool) { +func (c *CalibratedClock) Update(parked bool) (Parameters, bool) { c.mu.Lock() defer c.mu.Unlock() @@ -188,7 +192,7 @@ func (c *CalibratedClock) Update() (Parameters, bool) { Frequency: (minHz + maxHz) / 2, BaseRef: newest.ref, BaseCycles: newest.after, - }) + }, parked) return c.params, true } @@ -241,9 +245,9 @@ func NewCalibratedClocks() *CalibratedClocks { } // Update implements Clocks.Update. -func (c *CalibratedClocks) Update() (Parameters, bool, Parameters, bool) { - monotonicParams, monotonicOk := c.monotonic.Update() - realtimeParams, realtimeOk := c.realtime.Update() +func (c *CalibratedClocks) Update(parked bool) (Parameters, bool, Parameters, bool) { + monotonicParams, monotonicOk := c.monotonic.Update(parked) + realtimeParams, realtimeOk := c.realtime.Update(parked) return monotonicParams, monotonicOk, realtimeParams, realtimeOk } diff --git a/pkg/sentry/time/calibrated_clock_test.go b/pkg/sentry/time/calibrated_clock_test.go index f0847191ce..9b671ddec6 100644 --- a/pkg/sentry/time/calibrated_clock_test.go +++ b/pkg/sentry/time/calibrated_clock_test.go @@ -43,7 +43,7 @@ func TestConstantFrequency(t *testing.T) { // Update from all samples. for range samples { - c.Update() + c.Update(false) } c.mu.RLock() @@ -109,12 +109,12 @@ func TestErrorCorrection(t *testing.T) { c := newTestCalibratedClock(tc.samples[:], nil) // Initial calibration takes two updates. - _, ok := c.Update() + _, ok := c.Update(false) if ok { t.Fatalf("Update ready too early") } - params, ok := c.Update() + params, ok := c.Update(false) if !ok { t.Fatalf("Update not ready") } @@ -139,7 +139,7 @@ func TestErrorCorrection(t *testing.T) { } // Update again to see the changed reference clock. - params, ok = c.Update() + params, ok = c.Update(false) if !ok { t.Fatalf("Update not ready") } @@ -165,7 +165,7 @@ func TestErrorCorrection(t *testing.T) { initialErr := c.errorNS t.Logf("initial error: %v ns", initialErr) - _, ok = c.Update() + _, ok = c.Update(false) if !ok { t.Fatalf("Update not ready") } @@ -173,7 +173,7 @@ func TestErrorCorrection(t *testing.T) { t.Errorf("errorNS increased, got %v want |%v| <= |%v|", c.errorNS, c.errorNS, initialErr) } - _, ok = c.Update() + _, ok = c.Update(false) if !ok { t.Fatalf("Update not ready") } diff --git a/pkg/sentry/time/clocks.go b/pkg/sentry/time/clocks.go index 837e860944..ec69cd52a7 100644 --- a/pkg/sentry/time/clocks.go +++ b/pkg/sentry/time/clocks.go @@ -21,7 +21,9 @@ type Clocks interface { // reference host clocks, and returning the new timekeeping parameters. // // Update should be called at approximately ApproxUpdateInterval. - Update() (monotonicParams Parameters, monotonicOk bool, realtimeParam Parameters, realtimeOk bool) + // + // parked indicates that the clock was not read for at least ApproxUpdateInterval + Update(parked bool) (monotonicParams Parameters, monotonicOk bool, realtimeParam Parameters, realtimeOk bool) // GetTime returns the current time in nanoseconds for the given clock. //