From 6803cc637f4af7e05ca9c64d1844c60a55c89d11 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 28 Jan 2024 11:42:49 -0800 Subject: [PATCH 01/10] feat(maitake): add `time::Clock` (#475) This commit adds a new type to the `maitake::time` module, called `Clock`. A `Clock` bundles together a base duration and a `now()` function, making it easier for libraries to define time sources to be used with `maitake::time`. --- maitake/src/time.rs | 8 +- maitake/src/time/clock.rs | 279 ++++++++++++++++++++ maitake/src/time/timer.rs | 69 +++-- maitake/src/time/timer/sleep.rs | 2 +- maitake/src/time/timer/tests.rs | 85 ++++++ maitake/src/time/timer/tests/concurrent.rs | 85 +++--- maitake/src/time/timer/tests/wheel_tests.rs | 61 +++-- 7 files changed, 489 insertions(+), 100 deletions(-) create mode 100644 maitake/src/time/clock.rs diff --git a/maitake/src/time.rs b/maitake/src/time.rs index 9008563e..d087d663 100644 --- a/maitake/src/time.rs +++ b/maitake/src/time.rs @@ -43,14 +43,18 @@ //! [wheel]: http://www.cs.columbia.edu/~nahum/w6998/papers/sosp87-timing-wheels.pdf //! [driving-timers]: Timer#driving-timers #![warn(missing_docs, missing_debug_implementations)] +mod clock; pub mod timeout; mod timer; use crate::util; #[doc(inline)] -pub use self::timeout::Timeout; -pub use self::timer::{set_global_timer, sleep::Sleep, AlreadyInitialized, Timer, TimerError, Turn}; +pub use self::{ + clock::{Clock, Instant}, + timeout::Timeout, + timer::{set_global_timer, sleep::Sleep, AlreadyInitialized, Timer, TimerError, Turn}, +}; pub use core::time::Duration; use core::future::Future; diff --git a/maitake/src/time/clock.rs b/maitake/src/time/clock.rs new file mode 100644 index 00000000..15a604d4 --- /dev/null +++ b/maitake/src/time/clock.rs @@ -0,0 +1,279 @@ +use super::{ + timer::{self, TimerError}, + Duration, +}; +use core::ops::{Add, AddAssign, Sub, SubAssign}; + +// /// A hardware clock. +// pub trait Clock: Send + Sync + 'static { +// /// Returns the current hardware timestamp, in ticks of this `Clock`. +// /// +// /// # Monotonicity +// /// +// /// Implementations of this method MUST ensure that timestampss returned by +// /// `now()` MUST be [monontonically non-decreasing]. This means that a call +// /// to `now()` MUST NOT ever return a value less than the value returned by +// /// a previous call to`now()`. +// /// +// /// Note that this means that timestamps returned by `now()` are expected +// /// not to overflow. Of course, all integers *will* overflow eventually, so +// /// this requirement can reasonably be weakened to expecting that timestamps +// /// returned by `now()` will not overflow unless the system has been running +// /// for a duration substantially longer than the system is expected to run +// /// for. For example, if a system is expected to run for as long as a year +// /// without being restarted, it's not unreasonable for timestamps returned +// /// by `now()` to overflow after, say, 100 years. Ideally, a general-purpose +// /// `Clock` implementation would not overflow for, say, 1,000 years. +// /// +// /// The implication of this is that if the timestamp counters provided by +// /// the hardware platform are less than 64 bits wide (e.g., 16- or 32-bit +// /// timestamps), the `Clock` implementation is responsible for ensuring that +// /// they are extended to 64 bits, such as by counting overflows in the +// /// `Clock` implementation. +// /// +// /// [monotonic]: https://en.wikipedia.org/wiki/Monotonic_function +// #[must_use] +// fn now_ticks(&self) -> Ticks; + +// #[must_use] +// fn tick_duration(&self) -> Duration; + +// #[must_use] +// fn now(&self) -> Instant { +// let now = self.now_ticks(); +// let tick_duration = self.tick_duration(); +// Instant { now, tick_duration } +// } + +// fn max_duration(&self) -> Duration { +// max_duration(self.tick_duration()) +// } +// } + +/// A hardware clock definition. +/// +/// A `Clock` consists of a function that returns the hardware clock's current +/// timestamp in [`Ticks`], and a [`Duration`] that defines the amount of time +/// represented by a single tick of the clock. +#[derive(Clone, Debug)] +pub struct Clock { + now: fn() -> Ticks, + tick_duration: Duration, +} + +/// A measurement of a monotonically nondecreasing clock. +/// Opaque and useful only with [`Duration`]. +/// +/// Provided that the [`Clock` implementation is correct, `Instant`s are always +/// guaranteed to be no less than any previously measured instant when created, +/// and are often useful for tasks such as measuring benchmarks or timing how +/// long an operation takes. +/// +/// Note, however, that instants are **not** guaranteed to be **steady**. In other +/// words, each tick of the underlying clock might not be the same length (e.g. +/// some seconds may be longer than others). An instant may jump forwards or +/// experience time dilation (slow down or speed up), but it will never go +/// backwards. +/// As part of this non-guarantee it is also not specified whether system suspends count as +/// elapsed time or not. The behavior varies across platforms and rust versions. +/// +/// Instants are opaque types that can only be compared to one another. There is +/// no method to get "the number of seconds" from an instant. Instead, it only +/// allows measuring the duration between two instants (or comparing two +/// instants). +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)] +pub struct Instant { + now: Ticks, + tick_duration: Duration, +} + +/// Timer ticks are always counted by a 64-bit unsigned integer. +pub type Ticks = u64; + +impl Clock { + pub const fn new(now: fn() -> Ticks, tick_duration: Duration) -> Self { + Self { now, tick_duration } + } + + #[must_use] + pub(crate) fn now_ticks(&self) -> Ticks { + (self.now)() + } + + #[must_use] + pub fn tick_duration(&self) -> Duration { + self.tick_duration + } + + #[must_use] + pub fn now(&self) -> Instant { + let now = self.now_ticks(); + let tick_duration = self.tick_duration(); + Instant { now, tick_duration } + } + + pub fn max_duration(&self) -> Duration { + max_duration(self.tick_duration()) + } +} + +#[track_caller] +#[inline] +#[must_use] +pub(in crate::time) fn ticks_to_dur(tick_duration: Duration, ticks: Ticks) -> Duration { + let nanos = tick_duration.subsec_nanos() as u64 * ticks; + let secs = tick_duration.as_secs() * ticks; + Duration::new(secs, nanos as u32) +} + +#[track_caller] +#[inline] +#[must_use] +pub(in crate::time) fn dur_to_ticks( + tick_duration: Duration, + dur: Duration, +) -> Result { + (dur.as_nanos() / tick_duration.as_nanos()) + .try_into() + .map_err(|_| TimerError::DurationTooLong { + requested: dur, + max: max_duration(tick_duration), + }) +} + +#[track_caller] +#[inline] +#[must_use] +pub(in crate::time) fn max_duration(tick_duration: Duration) -> Duration { + ticks_to_dur(tick_duration, Ticks::MAX) +} + +impl Instant { + /// Returns an instant corresponding to "now". + /// + /// This function uses the [global default timer][global]. See [the + /// module-level documentation][global] for details on using the global + /// default timer. + /// + /// # Panics + /// + /// This function panics if the [global default timer][global] has not been + /// set. + /// + /// For a version of this function that returns a [`Result`] rather than + /// panicking, use [`Instant::try_now`] instead. + /// + /// [global]: crate::time#global-timers + #[must_use] + pub fn now() -> Instant { + todo!() + } + + /// Returns an instant corresponding to "now", without panicking. + /// + /// This function uses the [global default timer][global]. See [the + /// module-level documentation][global] for details on using the global + /// default timer. + /// + /// # Returns + /// + /// - [`Ok`]`(`[`Instant`]`)` if the [global default timer] is available. + /// - [`Err`]`(`[`TimerError`]`)` if no [global default timer][global] has + /// been set. + /// + /// [global]: crate::time#global-timers + pub fn try_now() -> Result { + Ok(timer::global::default()?.now()) + } + + /// Returns the amount of time elapsed from another instant to this one, + /// or zero duration if that instant is later than this one. + #[must_use] + pub fn duration_since(&self, earlier: Instant) -> Duration { + self.checked_duration_since(earlier).unwrap_or_default() + } + + /// Returns the amount of time elapsed from another instant to this one, + /// or [`None`]` if that instant is later than this one. + #[must_use] + pub fn checked_duration_since(&self, earlier: Instant) -> Option { + todo!() + // self.0.checked_sub(earlier.0) + } + + /// Returns the amount of time elapsed since this instant. + #[must_use] + pub fn elapsed(&self) -> Duration { + todo!() + // self.duration_since(Instant::now()) + } + + /// Returns `Some(t)` where `t` is the time `self + duration` if `t` can be represented as + /// `Instant` (which means it's inside the bounds of the underlying data structure), [`None`] + /// otherwise. + #[must_use] + pub fn checked_add(&self, duration: Duration) -> Option { + todo!() + // self.0.checked_add(duration).map(Instant) + } + + /// Returns `Some(t)` where `t` is the time `self - duration` if `t` can be represented as + /// `Instant` (which means it's inside the bounds of the underlying data structure), [`None`] + /// otherwise. + #[must_use] + pub fn checked_sub(&self, duration: Duration) -> Option { + todo!() + // self.0.checked_sub(duration).map(Instant) + } +} + +impl Add for Instant { + type Output = Instant; + + /// # Panics + /// + /// This function may panic if the resulting point in time cannot be represented by the + /// underlying data structure. See [`Instant::checked_add`] for a version without panic. + fn add(self, other: Duration) -> Instant { + self.checked_add(other) + .expect("overflow when adding duration to instant") + } +} + +impl AddAssign for Instant { + fn add_assign(&mut self, other: Duration) { + *self = *self + other; + } +} + +impl Sub for Instant { + type Output = Instant; + + fn sub(self, other: Duration) -> Instant { + self.checked_sub(other) + .expect("overflow when subtracting duration from instant") + } +} + +impl SubAssign for Instant { + fn sub_assign(&mut self, other: Duration) { + *self = *self - other; + } +} + +impl Sub for Instant { + type Output = Duration; + + /// Returns the amount of time elapsed from another instant to this one, + /// or zero duration if that instant is later than this one. + fn sub(self, other: Instant) -> Duration { + self.duration_since(other) + } +} + +// impl fmt::Display for Instant { +// #[inline] +// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { +// fmt::Debug::fmt(&self.0, f) +// } +// } diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 80f8bf72..44a865e6 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -6,6 +6,7 @@ //! [`Sleep`]: crate::time::Sleep //! [`Timeout`]: crate::time::Timeout //! [future]: core::future::Future +use super::clock::{self, Clock, Instant, Ticks}; use crate::{ loom::sync::{ atomic::{AtomicUsize, Ordering::*}, @@ -185,11 +186,7 @@ use self::sleep::Sleep; /// [`try_timeout`]: crate::time::try_timeout() /// [global]: crate::time#global-timers pub struct Timer { - /// The duration represented by one tick of this timer. - /// - /// This represents the timer's finest granularity; durations shorter than - /// this are rounded up to one tick. - tick_duration: Duration, + clock: Clock, /// A count of how many timer ticks have elapsed since the last time the /// timer's [`Core`] was updated. @@ -254,9 +251,6 @@ pub struct Turn { tick_duration: Duration, } -/// Timer ticks are always counted by a 64-bit unsigned integer. -pub type Ticks = u64; - /// Errors returned by [`Timer::try_sleep`], [`Timer::try_timeout`], and the /// global [`try_sleep`] and [`try_timeout`] functions. /// @@ -299,21 +293,24 @@ pub enum TimerError { impl Timer { loom_const_fn! { - /// Returns a new `Timer` with the specified `tick_duration` for a single timer - /// tick. + /// Returns a new `Timer` with the specified hardware [`Clock`]. #[must_use] - pub fn new(tick_duration: Duration) -> Self { + pub fn new(clock: Clock) -> Self { Self { - tick_duration, + clock, pending_ticks: AtomicUsize::new(0), core: Mutex::new(wheel::Core::new()), } } } + pub fn now(&self) -> Instant { + self.clock.now() + } + /// Returns the maximum duration of [`Sleep`] futures driven by this timer. pub fn max_duration(&self) -> Duration { - ticks_to_dur(self.tick_duration, u64::MAX) + self.clock.max_duration() } /// Returns a [`Future`] that will complete in `duration`. @@ -563,6 +560,14 @@ impl Timer { self.advance_locked(self.core.lock(), ticks) } + pub(in crate::time) fn ticks_to_dur(&self, ticks: Ticks) -> Duration { + clock::ticks_to_dur(self.clock.tick_duration(), ticks) + } + + pub(in crate::time) fn dur_to_ticks(&self, duration: Duration) -> Result { + clock::dur_to_ticks(self.clock.tick_duration(), duration) + } + fn advance_locked(&self, mut core: MutexGuard<'_, wheel::Core>, ticks: Ticks) -> Turn { // take any pending ticks. let pending_ticks = self.pending_ticks.swap(0, AcqRel) as Ticks; @@ -579,7 +584,7 @@ impl Timer { expired: expired.saturating_add(pend_exp), next_deadline_ticks: next_deadline.map(|d| d.ticks), now: core.now(), - tick_duration: self.tick_duration, + tick_duration: self.clock.tick_duration(), } } @@ -587,16 +592,6 @@ impl Timer { self.core.lock() } - #[track_caller] - fn dur_to_ticks(&self, dur: Duration) -> Result { - (dur.as_nanos() / self.tick_duration.as_nanos()) - .try_into() - .map_err(|_| TimerError::DurationTooLong { - requested: dur, - max: self.max_duration(), - }) - } - #[cfg(all(test, not(loom)))] fn reset(&self) { let mut core = self.core(); @@ -607,10 +602,16 @@ impl Timer { impl fmt::Debug for Timer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self { + clock, + pending_ticks, + core, + } = self; f.debug_struct("Timer") - .field("tick_duration", &self.tick_duration) - .field("pending_ticks", &self.pending_ticks.load(Acquire)) - .field("core", &fmt::opt(&self.core.try_lock()).or_else("")) + .field("clock", &clock) + .field("tick_duration", &clock.tick_duration()) + .field("pending_ticks", &pending_ticks.load(Acquire)) + .field("core", &fmt::opt(&core.try_lock()).or_else("")) .finish() } } @@ -633,14 +634,15 @@ impl Turn { #[inline] #[must_use] pub fn time_to_next_deadline(&self) -> Option { - self.ticks_to_next_deadline().map(|deadline| ticks_to_dur(self.tick_duration, deadline)) + self.ticks_to_next_deadline() + .map(|deadline| clock::ticks_to_dur(self.tick_duration, deadline)) } /// Returns the total elapsed time since this timer wheel started running. #[inline] #[must_use] pub fn elapsed(&self) -> Duration { - ticks_to_dur(self.tick_duration, self.now) + clock::ticks_to_dur(self.tick_duration, self.now) } /// Returns `true` if there are currently pending [`Sleep`] futures @@ -652,15 +654,6 @@ impl Turn { } } -#[track_caller] -#[inline] -#[must_use] -fn ticks_to_dur(tick_duration: Duration, ticks: Ticks) -> Duration { - let nanos = tick_duration.subsec_nanos() as u64 * ticks; - let secs = tick_duration.as_secs() * ticks; - Duration::new(secs, nanos as u32) -} - // === impl TimerError ==== impl fmt::Display for TimerError { diff --git a/maitake/src/time/timer/sleep.rs b/maitake/src/time/timer/sleep.rs index e81dac0f..8af6261e 100644 --- a/maitake/src/time/timer/sleep.rs +++ b/maitake/src/time/timer/sleep.rs @@ -84,7 +84,7 @@ impl<'timer> Sleep<'timer> { /// Returns the [`Duration`] that this `Sleep` future will sleep for. pub fn duration(&self) -> Duration { - super::ticks_to_dur(self.timer.tick_duration, self.entry.ticks) + self.timer.ticks_to_dur(self.entry.ticks) } } diff --git a/maitake/src/time/timer/tests.rs b/maitake/src/time/timer/tests.rs index ea14e926..d8e87db5 100644 --- a/maitake/src/time/timer/tests.rs +++ b/maitake/src/time/timer/tests.rs @@ -1,6 +1,91 @@ #[cfg(any(loom, feature = "alloc"))] use super::*; +use core::cell::RefCell; +use core::sync::atomic::{AtomicU64, Ordering}; +use std::sync::Arc; + +thread_local! { + static CLOCK: RefCell>> = const { RefCell::new(None) }; +} + +#[derive(Debug, Clone)] +pub struct TestClock(Arc); + +#[derive(Debug)] +struct TestClockState { + now: AtomicU64, +} + +#[derive(Debug)] +#[must_use = "dropping a test clock immediately resets the clock"] +pub struct ClockHandle(Arc); + +impl TestClock { + pub fn start() -> ClockHandle { + let this = Self(Arc::new(TestClockState { + now: AtomicU64::new(0), + })); + this.enter() + } + + pub fn enter(&self) -> ClockHandle { + CLOCK.with(|current| { + let prev = current.replace(Some(self.0.clone())); + assert!(prev.is_none(), "test clock already started!"); + }); + ClockHandle(self.0.clone()) + } + + pub const fn clock() -> Clock { + Clock::new(Self::now, Duration::from_millis(1)) + } + + fn now() -> u64 { + CLOCK.with(|current| { + let current = current.borrow(); + let clock = current + .as_ref() + .expect("the test clock has not started on this thread!"); + clock.now.load(Ordering::SeqCst) + }) + } +} + +impl ClockHandle { + pub fn advance(&self, duration: Duration) { + info!(?duration, "advancing test clock by"); + self.advance_ticks_inner(duration.as_millis() as u64) + } + + pub fn advance_ticks(&self, ticks: Ticks) { + info!(ticks, "advancing test clock by"); + self.advance_ticks_inner(ticks) + } + + fn advance_ticks_inner(&self, ticks: Ticks) { + let prev = self.0.now.fetch_add(ticks, Ordering::SeqCst); + assert!( + Ticks::MAX - prev >= ticks, + "clock overflowed (now: {prev}; advanced by {ticks})" + ); + } + + pub fn ticks(&self) -> Ticks { + self.0.now.load(Ordering::SeqCst) + } + + pub fn test_clock(&self) -> TestClock { + TestClock(self.0.clone()) + } +} + +impl Drop for ClockHandle { + fn drop(&mut self) { + let _ = CLOCK.try_with(|current| current.borrow_mut().take()); + } +} + #[cfg(all(feature = "alloc", not(loom)))] mod wheel_tests; diff --git a/maitake/src/time/timer/tests/concurrent.rs b/maitake/src/time/timer/tests/concurrent.rs index 436feab6..45a0beeb 100644 --- a/maitake/src/time/timer/tests/concurrent.rs +++ b/maitake/src/time/timer/tests/concurrent.rs @@ -25,16 +25,20 @@ fn model(f: impl Fn() + Send + Sync + 'static) { #[cfg_attr(not(loom), ignore)] fn one_sleep() { model(|| { - let timer = Arc::new(Timer::new(Duration::from_millis(1))); + let clock = TestClock::start(); + let timer = Arc::new(Timer::new(TestClock::clock())); let thread = thread::spawn({ let timer = timer.clone(); + let clock = clock.test_clock(); move || { + let _clock = clock.enter(); let sleep = timer.sleep(Duration::from_secs(1)); block_on(sleep) } }); for _ in 0..10 { + clock.advance(Duration::from_secs(1)); timer.advance(Duration::from_secs(1)); thread::yield_now(); } @@ -47,23 +51,29 @@ fn one_sleep() { #[cfg_attr(not(loom), ignore)] fn two_sleeps_parallel() { model(|| { - let timer = Arc::new(Timer::new(Duration::from_millis(1))); + let clock = TestClock::start(); + let timer = Arc::new(Timer::new(TestClock::clock())); let thread1 = thread::spawn({ let timer = timer.clone(); + let clock = clock.test_clock(); move || { + let _clock = clock.enter(); let sleep = timer.sleep(Duration::from_secs(1)); block_on(sleep) } }); let thread2 = thread::spawn({ let timer = timer.clone(); + let clock = clock.test_clock(); move || { + let _clock = clock.enter(); let sleep = timer.sleep(Duration::from_secs(1)); block_on(sleep) } }); for _ in 0..10 { + clock.advance(Duration::from_secs(1)); timer.advance(Duration::from_secs(1)); thread::yield_now(); } @@ -77,10 +87,13 @@ fn two_sleeps_parallel() { #[cfg_attr(not(loom), ignore)] fn two_sleeps_sequential() { model(|| { - let timer = Arc::new(Timer::new(Duration::from_millis(1))); + let clock = TestClock::start(); + let timer = Arc::new(Timer::new(TestClock::clock())); let thread = thread::spawn({ let timer = timer.clone(); + let clock = clock.test_clock(); move || { + let _clock = clock.enter(); block_on(async move { timer.sleep(Duration::from_secs(1)).await; timer.sleep(Duration::from_secs(1)).await; @@ -89,6 +102,7 @@ fn two_sleeps_sequential() { }); for _ in 0..10 { + clock.advance(Duration::from_secs(1)); timer.advance(Duration::from_secs(1)); thread::yield_now(); } @@ -99,36 +113,41 @@ fn two_sleeps_sequential() { #[test] fn cancel_polled_sleeps() { - fn poll_and_cancel(timer: Arc) -> impl FnOnce() { - move || { - let timer = timer; - block_on(async move { - let sleep = timer.sleep_ticks(15); - pin_mut!(sleep); - future::poll_fn(move |cx| { - // poll once to register the sleep with the timer wheel, and - // then return `Ready` so it gets canceled. - let poll = sleep.as_mut().poll(cx); - tokio_test::assert_pending!( - poll, - "sleep should not complete, as its timer has not been advanced", - ); - let poll = sleep.as_mut().poll(cx); - tokio_test::assert_pending!( - poll, - "sleep should not complete, as its timer has not been advanced", - ); - Poll::Ready(()) - }) - .await - }); - } + fn poll_and_cancel(timer: Arc) { + block_on(async move { + let sleep = timer.sleep_ticks(15); + pin_mut!(sleep); + future::poll_fn(move |cx| { + // poll once to register the sleep with the timer wheel, and + // then return `Ready` so it gets canceled. + let poll = sleep.as_mut().poll(cx); + tokio_test::assert_pending!( + poll, + "sleep should not complete, as its timer has not been advanced", + ); + let poll = sleep.as_mut().poll(cx); + tokio_test::assert_pending!( + poll, + "sleep should not complete, as its timer has not been advanced", + ); + Poll::Ready(()) + }) + .await + }) } model(|| { - let timer = Arc::new(Timer::new(Duration::from_secs(1))); - let thread = thread::spawn(poll_and_cancel(timer.clone())); - poll_and_cancel(timer)(); + let clock = TestClock::start(); + let timer = Arc::new(Timer::new(TestClock::clock())); + let thread = thread::spawn({ + let timer = timer.clone(); + let clock = clock.test_clock(); + move || { + let _clock = clock.enter(); + poll_and_cancel(timer.clone()) + } + }); + poll_and_cancel(timer); thread.join().unwrap() }) } @@ -137,10 +156,13 @@ fn cancel_polled_sleeps() { #[cfg_attr(not(loom), ignore)] fn reregister_waker() { model(|| { - let timer = Arc::new(Timer::new(Duration::from_millis(1))); + let clock = TestClock::start(); + let timer = Arc::new(Timer::new(TestClock::clock())); let thread = thread::spawn({ let timer = timer.clone(); + let clock = clock.test_clock(); move || { + let _clock = clock.enter(); let sleep = timer.sleep(Duration::from_secs(1)); pin_mut!(sleep); // poll the sleep future initially with a no-op waker. @@ -152,6 +174,7 @@ fn reregister_waker() { }); for _ in 0..10 { + clock.advance(Duration::from_secs(1)); timer.advance(Duration::from_secs(1)); thread::yield_now(); } diff --git a/maitake/src/time/timer/tests/wheel_tests.rs b/maitake/src/time/timer/tests/wheel_tests.rs index 06c02429..94731b7e 100644 --- a/maitake/src/time/timer/tests/wheel_tests.rs +++ b/maitake/src/time/timer/tests/wheel_tests.rs @@ -10,9 +10,9 @@ use std::sync::{ use proptest::{collection::vec, proptest}; struct SleepGroupTest { + clock: ClockHandle, scheduler: Scheduler, timer: &'static Timer, - now: Ticks, groups: BTreeMap, next_id: usize, @@ -31,16 +31,21 @@ struct SleepGroup { impl SleepGroupTest { fn new(timer: &'static Timer) -> Self { let _guard = crate::util::test::trace_init_with_default("info,maitake::time=trace"); + trace!(timer = ?fmt::alt(timer), "starting test"); Self { + clock: TestClock::start(), scheduler: Scheduler::new(), timer, - now: 0, groups: BTreeMap::new(), next_id: 0, _guard, } } + fn now(&self) -> Ticks { + self.clock.ticks() + } + fn spawn_group(&mut self, duration: Ticks, tasks: usize) { self.next_id += 1; let count = Arc::new(AtomicUsize::new(tasks)); @@ -61,10 +66,10 @@ impl SleepGroupTest { "spawned sleep group to sleep for {duration} ticks" ); self.groups.insert( - self.now + duration, + self.now() + duration, SleepGroup { duration, - t_start: self.now, + t_start: self.now(), tasks, count, id, @@ -85,7 +90,7 @@ impl SleepGroupTest { #[track_caller] fn assert_all_complete(&self) { - let t_1 = self.now; + let t_1 = self.now(); for ( &t_done, SleepGroup { @@ -116,7 +121,7 @@ impl SleepGroupTest { #[track_caller] fn assert(&self) { - let t_1 = self.now; + let t_1 = self.now(); for ( &t_done, SleepGroup { @@ -150,16 +155,16 @@ impl SleepGroupTest { #[track_caller] fn advance(&mut self, ticks: Ticks) { - let t_0 = self.now; - self.now += ticks; + let t_0 = self.now(); + self.clock.advance_ticks(ticks); info!(""); - let _span = span!(Level::INFO, "advance", ticks, from = t_0, to = self.now).entered(); - info!("advancing test timer to {}", self.now); + let _span = span!(Level::INFO, "advance", ticks, from = t_0, to = self.now()).entered(); + let t_1 = self.now(); // how many tasks are expected to complete? let expected_complete: usize = self .groups .iter_mut() - .take_while(|(&t, _)| t <= self.now) + .take_while(|(&t, _)| t <= t_1) .map(|(_, g)| std::mem::replace(&mut g.tasks, 0)) .sum(); @@ -174,18 +179,16 @@ impl SleepGroupTest { self.assert(); assert_eq!( - completed, - expected_complete, + completed, expected_complete, "expected {expected_complete} tasks to complete when advancing \ the timer from {t_0} to {t_1}", - t_1 = self.now, ); } } #[test] fn pend_advance_wakes() { - static TIMER: Timer = Timer::new(Duration::from_millis(1)); + static TIMER: Timer = Timer::new(TestClock::clock()); let mut test = SleepGroupTest::new(&TIMER); test.spawn_group(100, 2); @@ -200,7 +203,7 @@ fn pend_advance_wakes() { // advance the timer by 50 more ticks // but ONLY by pending - test.now += 50; + test.clock.advance_ticks(50); test.timer.pend_ticks(50); // Tick the scheduler, nothing should have happened @@ -208,12 +211,14 @@ fn pend_advance_wakes() { assert_eq!(tick.completed, 0); // How many do we expect to complete "now"? (it's two) - let expected_complete: usize = test - .groups - .iter_mut() - .take_while(|(&t, _)| t <= test.now) - .map(|(_, g)| std::mem::replace(&mut g.tasks, 0)) - .sum(); + let expected_complete: usize = { + let now = test.now(); + test.groups + .iter_mut() + .take_while(|(&t, _)| t <= now) + .map(|(_, g)| std::mem::replace(&mut g.tasks, 0)) + .sum() + }; // Call force, which will "notice" the pending ticks let turn = test.timer.force_advance_ticks(0); @@ -228,7 +233,7 @@ fn pend_advance_wakes() { #[test] fn timer_basically_works() { - static TIMER: Timer = Timer::new(Duration::from_millis(1)); + static TIMER: Timer = Timer::new(TestClock::clock()); let mut test = SleepGroupTest::new(&TIMER); test.spawn_group(100, 2); @@ -264,7 +269,7 @@ fn timer_basically_works() { #[test] fn schedule_after_start() { - static TIMER: Timer = Timer::new(Duration::from_millis(1)); + static TIMER: Timer = Timer::new(TestClock::clock()); let mut test = SleepGroupTest::new(&TIMER); test.spawn_group(100, 2); @@ -305,7 +310,7 @@ fn schedule_after_start() { #[test] fn expired_shows_up() { - static TIMER: Timer = Timer::new(Duration::from_millis(1)); + static TIMER: Timer = Timer::new(TestClock::clock()); let mut test = SleepGroupTest::new(&TIMER); test.spawn_group(150, 2); @@ -333,7 +338,7 @@ fn expired_shows_up() { // advance the timer by 50 more ticks, NOT past our new sleeps, // but forward - test.now += 50; + test.clock.advance_ticks(50); let turn = test.timer.force_advance_ticks(50); assert_eq!(turn.ticks_to_next_deadline(), Some(10)); @@ -354,7 +359,7 @@ fn expired_shows_up() { #[test] fn max_sleep() { - static TIMER: Timer = Timer::new(Duration::from_millis(1)); + static TIMER: Timer = Timer::new(TestClock::clock()); let mut test = SleepGroupTest::new(&TIMER); test.spawn_group(wheel::Core::MAX_SLEEP_TICKS, 2); @@ -421,7 +426,7 @@ proptest! { #[test] fn fuzz_timer(actions in vec(fuzz_action_strategy(), 0..MAX_FUZZ_ACTIONS)) { static FUZZ_RUNS: AtomicUsize = AtomicUsize::new(1); - static TIMER: Timer = Timer::new(Duration::from_millis(1)); + static TIMER: Timer = Timer::new(TestClock::clock()); TIMER.reset(); let mut test = SleepGroupTest::new(&TIMER); From b321d754aa9be2b8d0693e9a8a2b60ad80f04a2b Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 28 Jan 2024 12:17:42 -0800 Subject: [PATCH 02/10] feat(maitake): remove `now` from `Timer::advance` (#475) Now that we have a trait for hardware clocks, we expect the hardware timestamp to always come from the clock, rather than being passed in when the timer wheel is turned. This commit changes the timer APIs so that, rather than advancing the timer wheel to a particular `now` time, we instead call `Timer::turn`, which advances the wheel to "whatever the `Clock` thinks the current time is". This also means we can get rid of `pending_ticks`, which I always kinda hated. --- maitake/src/time/timer.rs | 128 +++++--------------- maitake/src/time/timer/tests/concurrent.rs | 8 +- maitake/src/time/timer/tests/wheel_tests.rs | 7 +- maitake/src/time/timer/wheel.rs | 3 +- src/rt.rs | 2 +- 5 files changed, 43 insertions(+), 105 deletions(-) diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 44a865e6..99737d09 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -408,47 +408,8 @@ impl Timer { self.pending_ticks.fetch_add(ticks as usize, Release); } - /// Advance the timer by `duration`, potentially waking any [`Sleep`] futures - /// that have completed. - /// - /// # Returns - /// - /// - [`Some`]`(`[`Turn`]`)` if the lock was acquired and the wheel was - /// advanced. A [`Turn`] structure describes what occurred during this - /// turn of the wheel, including the [current time][elapsed] and the - /// [deadline of the next expiring timer][next], if one exists. - /// - [`None`] if the wheel was not advanced because the lock was already - /// held. - /// - /// [elapsed]: Turn::elapsed - /// [next]: Turn::time_to_next_deadline - /// - /// # Interrupt Safety - /// - /// This method will *never* spin if the timer wheel lock is held; instead, - /// it will add any new ticks to a counter of "pending" ticks and return - /// immediately. Therefore, it is safe to call this method in an interrupt - /// handler, as it will never acquire a lock that may already be locked. - /// - /// The [`force_advance`] method will spin to lock the timer wheel lock if - /// it is currently held, *ensuring* that any pending wakeups are processed. - /// That method should never be called in an interrupt handler. - /// - /// If a timer is driven primarily by calling `advance` in an interrupt - /// handler, it may be desirable to occasionally call [`force_advance`] - /// *outside* of an interrupt handler (i.e., as as part of an occasional - /// runtime bookkeeping process). This ensures that any pending ticks are - /// observed by the timer in a relatively timely manner. - /// - /// [`force_advance`]: Timer::force_advance - #[inline] - pub fn advance(&self, duration: Duration) { - let ticks = expect_display(self.dur_to_ticks(duration), "cannot advance timer"); - self.advance_ticks(ticks) - } - - /// Advance the timer by `ticks` timer ticks, potentially waking any [`Sleep`] - /// futures that have completed. + /// Attempt to turn the timer to the current `now` if the timer is not + /// locked, potentially waking any [`Sleep`] futures that have completed. /// /// # Returns /// @@ -481,26 +442,21 @@ impl Timer { /// /// [`force_advance_ticks`]: Timer::force_advance_ticks #[inline] - pub fn advance_ticks(&self, ticks: Ticks) { + pub fn try_turn(&self) -> Option { // `advance` may be called in an ISR, so it can never actually spin. // instead, if the timer wheel is busy (e.g. the timer ISR was called on // another core, or if a `Sleep` future is currently canceling itself), // we just add to a counter of pending ticks, and bail. if let Some(core) = self.core.try_lock() { - trace!(ticks, "locked timer wheel; advancing"); - self.advance_locked(core, ticks); + Some(self.advance_locked(core)) } else { - trace!(ticks, "could not lock timer wheel; pending"); - // if the core of the timer wheel is already locked, add to the pending - // tick count, which we will then advance the wheel by when it becomes - // available. - // TODO(eliza): if pending ticks overflows that's probably Bad News - self.pend_ticks(ticks) + trace!("could not lock timer wheel"); + None } } - /// Advance the timer by `duration`, ensuring any `Sleep` futures that have - /// completed are woken, even if a lock must be acquired. + /// Advance the timer to the current time, ensuring any [`Sleep`] futures that + /// have completed are woken, even if a lock must be acquired. /// /// # Returns /// @@ -525,39 +481,8 @@ impl Timer { /// /// [`advance`]: Timer::advance #[inline] - pub fn force_advance(&self, duration: Duration) -> Turn { - let ticks = expect_display(self.dur_to_ticks(duration), "cannot advance timer"); - self.force_advance_ticks(ticks) - } - - /// Advance the timer by `ticks` timer ticks, ensuring any `Sleep` futures - /// that have completed are woken, even if a lock must be acquired. - /// - /// # Returns - /// - /// A [`Turn`] structure describing what occurred during this turn of the - /// wheel, including including the [current time][elapsed] and the [deadline - /// of the next expiring timer][next], if one exists. - /// - /// [elapsed]: Turn::elapsed - /// [next]: Turn::time_to_next_deadline - /// - /// # Interrupt Safety - /// - /// This method will spin to acquire the timer wheel lock if it is currently - /// held elsewhere. Therefore, this method must *NEVER* be called in an - /// interrupt handler! - /// - /// If a timer is advanced inside an interrupt handler, use the [`advance_ticks`] - /// method instead. If a timer is advanced primarily by calls to - /// [`advance_ticks`], it may be desirable to occasionally call `force_advance` - /// outside an interrupt handler, to ensure that pending ticks are drained - /// frequently. - /// - /// [`advance_ticks`]: Timer::advance_ticks - #[inline] - pub fn force_advance_ticks(&self, ticks: Ticks) -> Turn { - self.advance_locked(self.core.lock(), ticks) + pub fn turn(&self) -> Turn { + self.advance_locked(self.core.lock()) } pub(in crate::time) fn ticks_to_dur(&self, ticks: Ticks) -> Duration { @@ -568,23 +493,36 @@ impl Timer { clock::dur_to_ticks(self.clock.tick_duration(), duration) } - fn advance_locked(&self, mut core: MutexGuard<'_, wheel::Core>, ticks: Ticks) -> Turn { + fn advance_locked(&self, mut core: MutexGuard<'_, wheel::Core>) -> Turn { // take any pending ticks. let pending_ticks = self.pending_ticks.swap(0, AcqRel) as Ticks; // we do two separate `advance` calls here instead of advancing once // with the sum, because `ticks` + `pending_ticks` could overflow. - let mut pend_exp = 0; + let mut expired: usize = 0; if pending_ticks > 0 { - let (expired, _next_deadline) = core.advance(pending_ticks); - pend_exp = expired; + let (expiring, _next_deadline) = core.turn_to(pending_ticks); + expired = expired.saturating_add(expiring); } - let (expired, next_deadline) = core.advance(ticks); - Turn { - expired: expired.saturating_add(pend_exp), - next_deadline_ticks: next_deadline.map(|d| d.ticks), - now: core.now(), - tick_duration: self.clock.tick_duration(), + let mut now = self.clock.now_ticks(); + loop { + let (expiring, next_deadline) = core.turn_to(now); + expired = expired.saturating_add(expiring); + if let Some(next) = next_deadline { + now = self.clock.now_ticks(); + if now >= next.ticks { + // we've advanced past the next deadline, so we need to + // advance again. + continue; + } + } + + return Turn { + expired, + next_deadline_ticks: next_deadline.map(|d| d.ticks), + now, + tick_duration: self.clock.tick_duration(), + }; } } diff --git a/maitake/src/time/timer/tests/concurrent.rs b/maitake/src/time/timer/tests/concurrent.rs index 45a0beeb..ea12211c 100644 --- a/maitake/src/time/timer/tests/concurrent.rs +++ b/maitake/src/time/timer/tests/concurrent.rs @@ -39,7 +39,7 @@ fn one_sleep() { for _ in 0..10 { clock.advance(Duration::from_secs(1)); - timer.advance(Duration::from_secs(1)); + timer.try_turn(); thread::yield_now(); } @@ -74,7 +74,7 @@ fn two_sleeps_parallel() { for _ in 0..10 { clock.advance(Duration::from_secs(1)); - timer.advance(Duration::from_secs(1)); + timer.try_turn(); thread::yield_now(); } @@ -103,7 +103,7 @@ fn two_sleeps_sequential() { for _ in 0..10 { clock.advance(Duration::from_secs(1)); - timer.advance(Duration::from_secs(1)); + timer.try_turn(); thread::yield_now(); } @@ -175,7 +175,7 @@ fn reregister_waker() { for _ in 0..10 { clock.advance(Duration::from_secs(1)); - timer.advance(Duration::from_secs(1)); + timer.try_turn(); thread::yield_now(); } diff --git a/maitake/src/time/timer/tests/wheel_tests.rs b/maitake/src/time/timer/tests/wheel_tests.rs index 94731b7e..ec119163 100644 --- a/maitake/src/time/timer/tests/wheel_tests.rs +++ b/maitake/src/time/timer/tests/wheel_tests.rs @@ -169,7 +169,8 @@ impl SleepGroupTest { .sum(); // advance the timer. - self.timer.advance_ticks(ticks); + let turn = self.timer.try_turn(); + debug!(?turn); let completed = self.scheduler.tick().completed; @@ -221,7 +222,7 @@ fn pend_advance_wakes() { }; // Call force, which will "notice" the pending ticks - let turn = test.timer.force_advance_ticks(0); + let turn = test.timer.turn(); assert_eq!(turn.expired, expected_complete); // NOW the tasks will show up as scheduled, and complete @@ -339,7 +340,7 @@ fn expired_shows_up() { // advance the timer by 50 more ticks, NOT past our new sleeps, // but forward test.clock.advance_ticks(50); - let turn = test.timer.force_advance_ticks(50); + let turn = test.timer.turn(); assert_eq!(turn.ticks_to_next_deadline(), Some(10)); let tick = test.scheduler.tick(); diff --git a/maitake/src/time/timer/wheel.rs b/maitake/src/time/timer/wheel.rs index fb6813ad..2bebae24 100644 --- a/maitake/src/time/timer/wheel.rs +++ b/maitake/src/time/timer/wheel.rs @@ -100,8 +100,7 @@ impl Core { } #[inline(never)] - pub(super) fn advance(&mut self, ticks: Ticks) -> (usize, Option) { - let now = self.now + ticks; + pub(super) fn turn_to(&mut self, now: Ticks) -> (usize, Option) { let mut fired = 0; // sleeps that need to be rescheduled on lower-level wheels need to be diff --git a/src/rt.rs b/src/rt.rs index 733f7f88..9b424842 100644 --- a/src/rt.rs +++ b/src/rt.rs @@ -127,7 +127,7 @@ impl Core { // turn the timer wheel if it wasn't turned recently and no one else is // holding a lock, ensuring any pending timer ticks are consumed. - TIMER.advance_ticks(0); + TIMER.turn(); // if there are remaining tasks to poll, continue without stealing. if tick.has_remaining { From ee096735ee313bc8513ab86ecdc72dbd391f5d3f Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 30 Jan 2024 10:44:19 -0800 Subject: [PATCH 03/10] feat(hal_x86_64): working RDTSC clock calibration (#475) It would be really nice to be able to use RDTSC as the timestamp source on x86_64, as it's the nicest hardware timer available on newer x86 CPUs. This commit starts on implementing a calibration routine for RDTSC, although it's not quite done. --- Cargo.lock | 1 + hal-x86_64/Cargo.toml | 1 + hal-x86_64/src/interrupt/apic/local.rs | 13 ++++-- hal-x86_64/src/time/tsc.rs | 59 ++++++++++++++++++++++++++ src/arch/x86_64.rs | 27 +++++++++--- src/arch/x86_64/interrupt.rs | 2 +- src/lib.rs | 1 + src/rt.rs | 6 +-- src/shell.rs | 12 +++--- 9 files changed, 103 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0d87b69a..a209c29d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1060,6 +1060,7 @@ version = "0.1.0" dependencies = [ "acpi", "hal-core", + "maitake", "mycelium-trace", "mycelium-util", "mycotest", diff --git a/hal-x86_64/Cargo.toml b/hal-x86_64/Cargo.toml index 328ba67c..bfc1362a 100644 --- a/hal-x86_64/Cargo.toml +++ b/hal-x86_64/Cargo.toml @@ -13,6 +13,7 @@ alloc = [] [dependencies] acpi = "4.1.1" hal-core = { path = "../hal-core" } +maitake = { path = "../maitake" } mycelium-util = { path = "../util" } mycelium-trace = { path = "../trace" } mycotest = { path = "../mycotest"} diff --git a/hal-x86_64/src/interrupt/apic/local.rs b/hal-x86_64/src/interrupt/apic/local.rs index 952ae1e5..c5c3d18d 100644 --- a/hal-x86_64/src/interrupt/apic/local.rs +++ b/hal-x86_64/src/interrupt/apic/local.rs @@ -152,6 +152,15 @@ impl LocalApic { // CPUID didn't help, so fall back to calibrating the APIC frequency // using the PIT. tracing::debug!("calibrating APIC timer frequency using PIT..."); + + // lock the PIT now, before actually starting the timer IRQ, so that we + // don't include any time spent waiting for the PIT lock. + // + // since we only run this code on startup, before any other cores have + // been started, this probably never actually waits for a lock. but...we + // should do it the right way, anyway. + let mut pit = crate::time::PIT.lock(); + unsafe { // set timer divisor to 16 self.write_register(TIMER_DIVISOR, 0b11); @@ -166,9 +175,7 @@ impl LocalApic { } // use the PIT to sleep for 10ms - crate::time::PIT - .lock() - .sleep_blocking(Duration::from_millis(10)) + pit.sleep_blocking(Duration::from_millis(10)) .expect("the PIT should be able to send a 10ms interrupt..."); unsafe { diff --git a/hal-x86_64/src/time/tsc.rs b/hal-x86_64/src/time/tsc.rs index a181a90d..0d3d72e7 100644 --- a/hal-x86_64/src/time/tsc.rs +++ b/hal-x86_64/src/time/tsc.rs @@ -1,4 +1,9 @@ use crate::cpu::{intrinsics, FeatureNotSupported}; +use core::{ + sync::atomic::{AtomicU32, Ordering::*}, + time::Duration, +}; +use maitake::time::Clock; use raw_cpuid::CpuId; #[derive(Copy, Clone, Debug)] @@ -22,6 +27,7 @@ impl Rdtsc { /// Reads the current value of the timestamp counter. #[inline(always)] + #[must_use] pub fn read_timestamp(&self) -> u64 { unsafe { // safety: if an instance of this type was constructed, then we @@ -29,4 +35,57 @@ impl Rdtsc { intrinsics::rdtsc() } } + + /// Returns a [`maitake::time::Clock`] defining a clock that uses `rdtsc` + /// timestamps to produce `maitake` ticks. + pub fn into_maitake_clock(self) -> Result { + const NOT_YET_CALIBRATED: u32 = u32::MAX; + static MAITAKE_TICK_SHIFT: AtomicU32 = AtomicU32::new(NOT_YET_CALIBRATED); + + fn now() -> u64 { + let rdtsc = unsafe { intrinsics::rdtsc() }; + let shift = MAITAKE_TICK_SHIFT.load(Relaxed); + rdtsc >> shift + } + + tracing::info!("calibrating RDTSC Maitake clock from PIT..."); + let mut pit = super::PIT.lock(); + let sleep_duration = Duration::from_millis(10); + + let t0 = self.read_timestamp(); + pit.sleep_blocking(sleep_duration) + .expect("PIT sleep failed for some reason"); + let t1 = self.read_timestamp(); + + let elapsed_cycles = t1 - t0; + tracing::debug!(t0, t1, elapsed_cycles, "slept for {sleep_duration:?}"); + + let mut shift = 0; + loop { + assert!( + shift < 64, + "shifted away all the precision in the timestamp counter! \ + this definitely should never happen..." + ); + let elapsed_ticks = elapsed_cycles >> shift; + tracing::debug!(shift, elapsed_ticks, "trying a new tick shift"); + + let elapsed_ticks: u32 = elapsed_ticks.try_into().expect( + "there is no way that the sleep duration is more than \ + u32::MAX rdtsc cycles...right?", + ); + + let tick_duration = sleep_duration / elapsed_ticks; + if tick_duration.as_nanos() > 0 { + // Ladies and gentlemen...we got him! + tracing::info!(?tick_duration, shift, "calibrated RDTSC Maitake clock"); + MAITAKE_TICK_SHIFT + .compare_exchange(NOT_YET_CALIBRATED, shift, AcqRel, Acquire) + .map_err(|_| "RDTSC Maitake clock has already been calibrated")?; + return Ok(Clock::new(now, tick_duration)); + } else { + shift += 1; + } + } + } } diff --git a/src/arch/x86_64.rs b/src/arch/x86_64.rs index 96a85d15..ef0ae8d1 100644 --- a/src/arch/x86_64.rs +++ b/src/arch/x86_64.rs @@ -2,7 +2,7 @@ use bootloader_api::config::{BootloaderConfig, Mapping}; use hal_core::boot::BootInfo; use hal_x86_64::{ cpu::{self, local::GsLocalData}, - vga, + time, vga, }; pub use hal_x86_64::{ cpu::{entropy::seed_rng, local::LocalKey, wait_for_interrupt}, @@ -67,7 +67,7 @@ pub fn init(_info: &impl BootInfo, archinfo: &ArchInfo) { } tracing::info!("set up the boot processor's local data"); - if let Some(rsdp) = archinfo.rsdp_addr { + let did_acpi_irq_init = if let Some(rsdp) = archinfo.rsdp_addr { let acpi = acpi::acpi_tables(rsdp); let platform_info = acpi.and_then(|acpi| acpi.platform_info()); match platform_info { @@ -76,17 +76,32 @@ pub fn init(_info: &impl BootInfo, archinfo: &ArchInfo) { interrupt::enable_hardware_interrupts(Some(&platform.interrupt_model)); acpi::bringup_smp(&platform) .expect("failed to bring up application processors! this is bad news!"); - return; + true + } + Err(error) => { + tracing::warn!(?error, "missing ACPI platform info"); + false } - Err(error) => tracing::warn!(?error, "missing ACPI platform info"), } } else { // TODO(eliza): try using MP Table to bringup application processors? tracing::warn!("no RSDP from bootloader, skipping SMP bringup"); + false + }; + + if !did_acpi_irq_init { + // no ACPI + interrupt::enable_hardware_interrupts(None); } - // no ACPI - interrupt::enable_hardware_interrupts(None); + match time::Rdtsc::new() { + Ok(rdtsc) => { + let rdtsc_clock = rdtsc.into_maitake_clock(); + + tracing::info!(?rdtsc_clock, "calibrated TSC"); + } + Err(error) => tracing::warn!(%error, "no RDTSC support"), + } } // TODO(eliza): this is now in arch because it uses the serial port, would be diff --git a/src/arch/x86_64/interrupt.rs b/src/arch/x86_64/interrupt.rs index 2465848f..8bd67d25 100644 --- a/src/arch/x86_64/interrupt.rs +++ b/src/arch/x86_64/interrupt.rs @@ -100,7 +100,7 @@ impl hal_core::interrupt::Handlers for InterruptHandlers { } fn timer_tick() { - crate::rt::TIMER.pend_ticks(1); + // crate::rt::TIMER.pend_ticks(1); } fn ps2_keyboard(scancode: u8) { diff --git a/src/lib.rs b/src/lib.rs index d3498fb7..4a0ba87c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -165,6 +165,7 @@ pub fn kernel_start(bootinfo: impl BootInfo, archinfo: crate::arch::ArchInfo) -> } fn kernel_main(bootinfo: impl BootInfo) -> ! { + loop {} rt::spawn(keyboard_demo()); let mut core = rt::Core::new(); diff --git a/src/rt.rs b/src/rt.rs index 9b424842..afcb9e92 100644 --- a/src/rt.rs +++ b/src/rt.rs @@ -52,7 +52,7 @@ struct Runtime { /// 512 CPU cores ought to be enough for anybody... pub const MAX_CORES: usize = 512; -pub static TIMER: time::Timer = time::Timer::new(arch::interrupt::TIMER_INTERVAL); +// pub static TIMER: time::Timer = time::Timer::new(arch::interrupt::TIMER_INTERVAL); static RUNTIME: Runtime = { // This constant is used as an array initializer; the clippy warning that it @@ -90,7 +90,7 @@ where /// Initialize the kernel runtime. pub fn init() { - time::set_global_timer(&TIMER).expect("`rt::init` should only be called once!"); + // time::set_global_timer(&TIMER).expect("`rt::init` should only be called once!"); tracing::info!("kernel runtime initialized"); } @@ -127,7 +127,7 @@ impl Core { // turn the timer wheel if it wasn't turned recently and no one else is // holding a lock, ensuring any pending timer ticks are consumed. - TIMER.turn(); + // TIMER.turn(); // if there are remaining tasks to poll, continue without stealing. if tick.has_remaining { diff --git a/src/shell.rs b/src/shell.rs index e49c825b..5c34bd00 100644 --- a/src/shell.rs +++ b/src/shell.rs @@ -98,12 +98,12 @@ const DUMP: Command = Command::new("dump") Command::new("archinfo") .with_help("print the architecture information structure") .with_fn(|ctx| Err(ctx.other_error("not yet implemented"))), - Command::new("timer") - .with_help("print the timer wheel") - .with_fn(|_| { - tracing::info!(target: "shell", timer = ?rt::TIMER); - Ok(()) - }), + // Command::new("timer") + // .with_help("print the timer wheel") + // .with_fn(|_| { + // tracing::info!(target: "shell", timer = ?rt::TIMER); + // Ok(()) + // }), rt::DUMP_RT, crate::arch::shell::DUMP_ARCH, Command::new("heap") From aedeb56da00347de3be3155e76fa0a1941bfc4b4 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 31 Jan 2024 10:17:09 -0800 Subject: [PATCH 04/10] fix(x86_64): put back stupid timer in mycelium (#475) Unfortunately, my RDTSC calibration code from 9d549a3 is not that good, and using RDTSC currently results in a bunch of weird time travel bugs. This commit puts back the less accurate interrupt-driven clock for now. I'll finish RDTSC later once the timer wheel changes land. --- Cargo.lock | 1 + hal-x86_64/src/interrupt.rs | 35 ++++++++++---------- hal-x86_64/src/time.rs | 2 +- hal-x86_64/src/time/pit.rs | 57 ++++++++++++++++++++++++++------- hal-x86_64/src/time/tsc.rs | 19 +++++++---- maitake/src/time/clock.rs | 54 ++++++++++++++++++------------- maitake/src/time/timer/tests.rs | 2 +- src/arch/x86_64.rs | 37 +++++++++++---------- src/arch/x86_64/interrupt.rs | 27 ++++++++++------ src/lib.rs | 5 ++- src/rt.rs | 10 +++--- 11 files changed, 154 insertions(+), 95 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a209c29d..1c15b90f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1597,6 +1597,7 @@ version = "0.1.0" dependencies = [ "embedded-graphics", "hal-core", + "maitake", "mycelium-util", "tracing 0.2.0", "tracing-core 0.2.0", diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs index 876d2bec..47ec5d43 100644 --- a/hal-x86_64/src/interrupt.rs +++ b/hal-x86_64/src/interrupt.rs @@ -1,4 +1,4 @@ -use crate::{cpu, mm, segment, PAddr, VAddr}; +use crate::{cpu, mm, segment, time, PAddr, VAddr}; use core::{arch::asm, marker::PhantomData, time::Duration}; use hal_core::interrupt::Control; use hal_core::interrupt::{ctx, Handlers}; @@ -37,6 +37,12 @@ pub struct CodeFault<'a> { /// An interrupt service routine. pub type Isr = extern "x86-interrupt" fn(&mut Context); +#[derive(Debug)] +pub enum PeriodicTimerError { + Pit(time::PitError), + Apic(time::InvalidDuration), +} + #[derive(Debug)] #[repr(C)] pub struct Interrupt { @@ -229,6 +235,7 @@ impl Controller { // `sti` may not be called until the interrupt controller static is // fully initialized, as an interrupt that occurs before it is // initialized may attempt to access the static to finish the interrupt! + core::sync::atomic::fence(core::sync::atomic::Ordering::SeqCst); unsafe { crate::cpu::intrinsics::sti(); } @@ -238,15 +245,15 @@ impl Controller { /// Starts a periodic timer which fires the `timer_tick` interrupt of the /// provided [`Handlers`] every time `interval` elapses. - pub fn start_periodic_timer( - &self, - interval: Duration, - ) -> Result<(), crate::time::InvalidDuration> { + pub fn start_periodic_timer(&self, interval: Duration) -> Result<(), PeriodicTimerError> { match self.model { - InterruptModel::Pic(_) => crate::time::PIT.lock().start_periodic_timer(interval), - InterruptModel::Apic { ref local, .. } => { - local.start_periodic_timer(interval, Idt::LOCAL_APIC_TIMER as u8) - } + InterruptModel::Pic(_) => crate::time::PIT + .lock() + .start_periodic_timer(interval) + .map_err(PeriodicTimerError::Pit), + InterruptModel::Apic { ref local, .. } => local + .start_periodic_timer(interval, Idt::LOCAL_APIC_TIMER as u8) + .map_err(PeriodicTimerError::Apic), } } } @@ -388,14 +395,8 @@ impl hal_core::interrupt::Control for Idt { } extern "x86-interrupt" fn pit_timer_isr>(_regs: Registers) { - use core::sync::atomic::Ordering; - // if we weren't trying to do a PIT sleep, handle the timer tick - // instead. - let was_sleeping = crate::time::pit::SLEEPING - .compare_exchange(true, false, Ordering::AcqRel, Ordering::Acquire) - .is_ok(); - if !was_sleeping { - H::timer_tick(); + if crate::time::Pit::handle_interrupt() { + H::timer_tick() } else { tracing::trace!("PIT sleep completed"); } diff --git a/hal-x86_64/src/time.rs b/hal-x86_64/src/time.rs index 5a88db60..284023e6 100644 --- a/hal-x86_64/src/time.rs +++ b/hal-x86_64/src/time.rs @@ -2,7 +2,7 @@ pub(crate) mod pit; mod tsc; pub use self::{ - pit::{Pit, PIT}, + pit::{Pit, PitError, PIT}, tsc::Rdtsc, }; use core::fmt; diff --git a/hal-x86_64/src/time/pit.rs b/hal-x86_64/src/time/pit.rs index 6a1c094b..692d13b3 100644 --- a/hal-x86_64/src/time/pit.rs +++ b/hal-x86_64/src/time/pit.rs @@ -3,7 +3,7 @@ use super::InvalidDuration; use crate::cpu::{self, Port}; use core::{ convert::TryFrom, - sync::atomic::{AtomicBool, Ordering}, + sync::atomic::{AtomicBool, AtomicU64, Ordering}, time::Duration, }; use mycelium_util::{ @@ -86,6 +86,17 @@ pub struct Pit { channel0_interval: Option, } +/// Errors returned by [`Pit::start_periodic_timer`] and [`Pit::sleep_blocking`]. +#[derive(Debug)] +pub enum PitError { + /// The periodic timer is already running. + AlreadyRunning, + /// A [`Pit::sleep_blocking`] call is in progress. + SleepInProgress, + /// The provided duration was invalid. + InvalidDuration(InvalidDuration), +} + bitfield! { struct Command { /// BCD/binary mode. @@ -202,7 +213,7 @@ enum_from_bits! { pub static PIT: Mutex = Mutex::new(Pit::new()); /// Are we currently sleeping on an interrupt? -pub(crate) static SLEEPING: AtomicBool = AtomicBool::new(false); +static SLEEPING: AtomicBool = AtomicBool::new(false); impl Pit { /// The PIT's base frequency runs at roughly 1.193182 MHz, for [extremely @@ -267,11 +278,12 @@ impl Pit { fields(?duration), err, )] - pub fn sleep_blocking(&mut self, duration: Duration) -> Result<(), InvalidDuration> { + pub fn sleep_blocking(&mut self, duration: Duration) -> Result<(), PitError> { SLEEPING .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) - .expect("if another CPU core is sleeping, it should be holding the lock on the PIT, preventing us from starting a sleep!"); - self.interrupt_in(duration)?; + .map_err(|_| PitError::SleepInProgress)?; + self.interrupt_in(duration) + .map_err(PitError::InvalidDuration)?; tracing::debug!("started PIT sleep"); // spin until the sleep interrupt fires. @@ -306,21 +318,20 @@ impl Pit { fields(?interval), err, )] - pub fn start_periodic_timer(&mut self, interval: Duration) -> Result<(), InvalidDuration> { - debug_assert!( - !SLEEPING.load(Ordering::Acquire), - "tried to start a periodic timer while a sleep was in progress!" - ); + pub fn start_periodic_timer(&mut self, interval: Duration) -> Result<(), PitError> { + if SLEEPING.load(Ordering::Acquire) { + return Err(PitError::SleepInProgress); + } let interval_ms = usize::try_from(interval.as_millis()).map_err(|_| { - InvalidDuration::new( + PitError::invalid_duration( interval, "PIT periodic timer interval as milliseconds would exceed a `usize`", ) })?; let interval_ticks = Self::TICKS_PER_MS * interval_ms; let divisor = u16::try_from(interval_ticks).map_err(|_| { - InvalidDuration::new(interval, "PIT channel 0 divisor would exceed a `u16`") + PitError::invalid_duration(interval, "PIT channel 0 divisor would exceed a `u16`") })?; // store the periodic timer interval so we can reset later. @@ -394,6 +405,10 @@ impl Pit { Ok(()) } + pub(crate) fn handle_interrupt() -> bool { + SLEEPING.swap(false, Ordering::AcqRel) + } + fn set_divisor(&mut self, divisor: u16) { tracing::trace!(divisor = &fmt::hex(divisor), "Pit::set_divisor"); let low = divisor as u8; @@ -413,3 +428,21 @@ impl Pit { } } } + +// === impl PitError === + +impl PitError { + fn invalid_duration(duration: Duration, msg: &'static str) -> Self { + Self::InvalidDuration(InvalidDuration::new(duration, msg)) + } +} + +impl fmt::Display for PitError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::AlreadyRunning => write!(f, "PIT periodic timer is already running"), + Self::SleepInProgress => write!(f, "a PIT sleep is currently in progress"), + Self::InvalidDuration(e) => write!(f, "invalid PIT duration: {e}"), + } + } +} diff --git a/hal-x86_64/src/time/tsc.rs b/hal-x86_64/src/time/tsc.rs index 0d3d72e7..efa2d24f 100644 --- a/hal-x86_64/src/time/tsc.rs +++ b/hal-x86_64/src/time/tsc.rs @@ -40,8 +40,16 @@ impl Rdtsc { /// timestamps to produce `maitake` ticks. pub fn into_maitake_clock(self) -> Result { const NOT_YET_CALIBRATED: u32 = u32::MAX; + + // 50ms is stolen from Linux, so i'm assuming its a reasonable duration + // for PIT-based calibration: + // https://github.com/torvalds/linux/blob/4ae004a9bca8bef118c2b4e76ee31c7df4514f18/arch/x86/kernel/tsc.c#L688-L839 + const PIT_SLEEP_DURATION: Duration = Duration::from_millis(50); + static MAITAKE_TICK_SHIFT: AtomicU32 = AtomicU32::new(NOT_YET_CALIBRATED); + return Err("calibration routine doesn't really work yet, sorry!"); + fn now() -> u64 { let rdtsc = unsafe { intrinsics::rdtsc() }; let shift = MAITAKE_TICK_SHIFT.load(Relaxed); @@ -50,15 +58,14 @@ impl Rdtsc { tracing::info!("calibrating RDTSC Maitake clock from PIT..."); let mut pit = super::PIT.lock(); - let sleep_duration = Duration::from_millis(10); let t0 = self.read_timestamp(); - pit.sleep_blocking(sleep_duration) + pit.sleep_blocking(PIT_SLEEP_DURATION) .expect("PIT sleep failed for some reason"); let t1 = self.read_timestamp(); let elapsed_cycles = t1 - t0; - tracing::debug!(t0, t1, elapsed_cycles, "slept for {sleep_duration:?}"); + tracing::debug!(t0, t1, elapsed_cycles, "slept for {PIT_SLEEP_DURATION:?}"); let mut shift = 0; loop { @@ -71,18 +78,18 @@ impl Rdtsc { tracing::debug!(shift, elapsed_ticks, "trying a new tick shift"); let elapsed_ticks: u32 = elapsed_ticks.try_into().expect( - "there is no way that the sleep duration is more than \ + "there is no way that a 50ms sleep duration is more than \ u32::MAX rdtsc cycles...right?", ); - let tick_duration = sleep_duration / elapsed_ticks; + let tick_duration = PIT_SLEEP_DURATION / elapsed_ticks; if tick_duration.as_nanos() > 0 { // Ladies and gentlemen...we got him! tracing::info!(?tick_duration, shift, "calibrated RDTSC Maitake clock"); MAITAKE_TICK_SHIFT .compare_exchange(NOT_YET_CALIBRATED, shift, AcqRel, Acquire) .map_err(|_| "RDTSC Maitake clock has already been calibrated")?; - return Ok(Clock::new(now, tick_duration)); + return Ok(Clock::new(tick_duration, now).named("CLOCK_RDTSC")); } else { shift += 1; } diff --git a/maitake/src/time/clock.rs b/maitake/src/time/clock.rs index 15a604d4..e6043c01 100644 --- a/maitake/src/time/clock.rs +++ b/maitake/src/time/clock.rs @@ -2,7 +2,10 @@ use super::{ timer::{self, TimerError}, Duration, }; -use core::ops::{Add, AddAssign, Sub, SubAssign}; +use core::{ + fmt, + ops::{Add, AddAssign, Sub, SubAssign}, +}; // /// A hardware clock. // pub trait Clock: Send + Sync + 'static { @@ -59,6 +62,7 @@ use core::ops::{Add, AddAssign, Sub, SubAssign}; pub struct Clock { now: fn() -> Ticks, tick_duration: Duration, + name: Option<&'static str>, } /// A measurement of a monotonically nondecreasing clock. @@ -82,17 +86,25 @@ pub struct Clock { /// allows measuring the duration between two instants (or comparing two /// instants). #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)] -pub struct Instant { - now: Ticks, - tick_duration: Duration, -} +pub struct Instant(Duration); /// Timer ticks are always counted by a 64-bit unsigned integer. pub type Ticks = u64; impl Clock { - pub const fn new(now: fn() -> Ticks, tick_duration: Duration) -> Self { - Self { now, tick_duration } + pub const fn new(tick_duration: Duration, now: fn() -> Ticks) -> Self { + Self { + now, + tick_duration, + name: None, + } + } + + pub const fn named(self, name: &'static str) -> Self { + Self { + name: Some(name), + ..self + } } #[must_use] @@ -109,7 +121,7 @@ impl Clock { pub fn now(&self) -> Instant { let now = self.now_ticks(); let tick_duration = self.tick_duration(); - Instant { now, tick_duration } + Instant(ticks_to_dur(tick_duration, now)) } pub fn max_duration(&self) -> Duration { @@ -166,7 +178,7 @@ impl Instant { /// [global]: crate::time#global-timers #[must_use] pub fn now() -> Instant { - todo!() + Self::try_now().expect("no global timer set") } /// Returns an instant corresponding to "now", without panicking. @@ -197,15 +209,13 @@ impl Instant { /// or [`None`]` if that instant is later than this one. #[must_use] pub fn checked_duration_since(&self, earlier: Instant) -> Option { - todo!() - // self.0.checked_sub(earlier.0) + self.0.checked_sub(earlier.0) } /// Returns the amount of time elapsed since this instant. #[must_use] pub fn elapsed(&self) -> Duration { - todo!() - // self.duration_since(Instant::now()) + self.0 } /// Returns `Some(t)` where `t` is the time `self + duration` if `t` can be represented as @@ -213,8 +223,7 @@ impl Instant { /// otherwise. #[must_use] pub fn checked_add(&self, duration: Duration) -> Option { - todo!() - // self.0.checked_add(duration).map(Instant) + self.0.checked_add(duration).map(Instant) } /// Returns `Some(t)` where `t` is the time `self - duration` if `t` can be represented as @@ -222,8 +231,7 @@ impl Instant { /// otherwise. #[must_use] pub fn checked_sub(&self, duration: Duration) -> Option { - todo!() - // self.0.checked_sub(duration).map(Instant) + self.0.checked_sub(duration).map(Instant) } } @@ -271,9 +279,9 @@ impl Sub for Instant { } } -// impl fmt::Display for Instant { -// #[inline] -// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { -// fmt::Debug::fmt(&self.0, f) -// } -// } +impl fmt::Display for Instant { + #[inline] + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&self.0, f) + } +} diff --git a/maitake/src/time/timer/tests.rs b/maitake/src/time/timer/tests.rs index d8e87db5..f200409f 100644 --- a/maitake/src/time/timer/tests.rs +++ b/maitake/src/time/timer/tests.rs @@ -38,7 +38,7 @@ impl TestClock { } pub const fn clock() -> Clock { - Clock::new(Self::now, Duration::from_millis(1)) + Clock::new(Duration::from_millis(1), Self::now) } fn now() -> u64 { diff --git a/src/arch/x86_64.rs b/src/arch/x86_64.rs index ef0ae8d1..e0e0d46d 100644 --- a/src/arch/x86_64.rs +++ b/src/arch/x86_64.rs @@ -58,7 +58,7 @@ pub fn arch_entry(info: &'static mut bootloader_api::BootInfo) -> ! { crate::kernel_start(boot_info, archinfo); } -pub fn init(_info: &impl BootInfo, archinfo: &ArchInfo) { +pub fn init(_info: &impl BootInfo, archinfo: &ArchInfo) -> maitake::time::Clock { pci::init_pci(); // init boot processor's core-local data @@ -67,41 +67,40 @@ pub fn init(_info: &impl BootInfo, archinfo: &ArchInfo) { } tracing::info!("set up the boot processor's local data"); - let did_acpi_irq_init = if let Some(rsdp) = archinfo.rsdp_addr { + if let Some(rsdp) = archinfo.rsdp_addr { let acpi = acpi::acpi_tables(rsdp); let platform_info = acpi.and_then(|acpi| acpi.platform_info()); match platform_info { Ok(platform) => { tracing::debug!("found ACPI platform info"); - interrupt::enable_hardware_interrupts(Some(&platform.interrupt_model)); + let irq_ctrl = + interrupt::enable_hardware_interrupts(Some(&platform.interrupt_model)); acpi::bringup_smp(&platform) .expect("failed to bring up application processors! this is bad news!"); - true + irq_ctrl } Err(error) => { tracing::warn!(?error, "missing ACPI platform info"); - false + interrupt::enable_hardware_interrupts(None) } } } else { // TODO(eliza): try using MP Table to bringup application processors? tracing::warn!("no RSDP from bootloader, skipping SMP bringup"); - false + interrupt::enable_hardware_interrupts(None) }; - if !did_acpi_irq_init { - // no ACPI - interrupt::enable_hardware_interrupts(None); - } - - match time::Rdtsc::new() { - Ok(rdtsc) => { - let rdtsc_clock = rdtsc.into_maitake_clock(); - - tracing::info!(?rdtsc_clock, "calibrated TSC"); - } - Err(error) => tracing::warn!(%error, "no RDTSC support"), - } + time::Rdtsc::new() + .map_err(|error| { + tracing::warn!(%error, "RDTSC not supported"); + }) + .and_then(|rdtsc| { + rdtsc + .into_maitake_clock() + .inspect(|clock| tracing::info!(?clock, "calibrated RDTSC clock")) + .map_err(|error| tracing::warn!(%error, "could not calibrate RDTSC clock")) + }) + .unwrap_or(interrupt::IDIOTIC_CLOCK) } // TODO(eliza): this is now in arch because it uses the serial port, would be diff --git a/src/arch/x86_64/interrupt.rs b/src/arch/x86_64/interrupt.rs index 8bd67d25..6fedeb2f 100644 --- a/src/arch/x86_64/interrupt.rs +++ b/src/arch/x86_64/interrupt.rs @@ -1,5 +1,5 @@ use super::{oops, Oops}; -use core::sync::atomic::{AtomicUsize, Ordering}; +use core::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use hal_core::{interrupt, VAddr}; pub use hal_x86_64::interrupt::*; use hal_x86_64::{ @@ -20,12 +20,13 @@ pub fn enable_exceptions() { } #[tracing::instrument(skip(acpi))] -pub fn enable_hardware_interrupts(acpi: Option<&acpi::InterruptModel>) { - let controller = Controller::enable_hardware_interrupts(acpi, &crate::ALLOC); - controller - .start_periodic_timer(TIMER_INTERVAL) - .expect("10ms should be a reasonable interval for the PIT or local APIC timer..."); - tracing::info!(granularity = ?TIMER_INTERVAL, "global timer initialized") +pub fn enable_hardware_interrupts(acpi: Option<&acpi::InterruptModel>) -> &'static Controller { + let irq_ctrl = Controller::enable_hardware_interrupts(acpi, &crate::ALLOC); + + irq_ctrl + .start_periodic_timer(IDIOTIC_CLOCK_INTERVAL) + .expect("failed to start periodic timer"); + irq_ctrl } // TODO(eliza): put this somewhere good. @@ -55,10 +56,18 @@ static TSS: sync::Lazy = sync::Lazy::new(|| { pub(in crate::arch) static GDT: sync::InitOnce = sync::InitOnce::uninitialized(); -pub const TIMER_INTERVAL: time::Duration = time::Duration::from_millis(10); +const IDIOTIC_CLOCK_INTERVAL: time::Duration = time::Duration::from_millis(10); + +static IDIOTIC_CLOCK_TICKS: AtomicU64 = AtomicU64::new(0); static TEST_INTERRUPT_WAS_FIRED: AtomicUsize = AtomicUsize::new(0); +pub const IDIOTIC_CLOCK: maitake::time::Clock = + maitake::time::Clock::new(IDIOTIC_CLOCK_INTERVAL, || { + IDIOTIC_CLOCK_TICKS.load(Ordering::Relaxed) + }) + .named("CLOCK_IDIOTIC"); + pub(crate) struct InterruptHandlers; /// Forcibly unlock the IOs we write to in an oops (VGA buffer and COM1 serial @@ -100,7 +109,7 @@ impl hal_core::interrupt::Handlers for InterruptHandlers { } fn timer_tick() { - // crate::rt::TIMER.pend_ticks(1); + IDIOTIC_CLOCK_TICKS.fetch_add(1, Ordering::Release); } fn ps2_keyboard(scancode: u8) { diff --git a/src/lib.rs b/src/lib.rs index 4a0ba87c..5c2b1d35 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -153,10 +153,10 @@ pub fn kernel_start(bootinfo: impl BootInfo, archinfo: crate::arch::ArchInfo) -> // perform arch-specific initialization once we have an allocator and // tracing. - arch::init(&bootinfo, &archinfo); + let clock = arch::init(&bootinfo, &archinfo); // initialize the kernel runtime. - rt::init(); + rt::init(clock); #[cfg(test)] arch::run_tests(); @@ -165,7 +165,6 @@ pub fn kernel_start(bootinfo: impl BootInfo, archinfo: crate::arch::ArchInfo) -> } fn kernel_main(bootinfo: impl BootInfo) -> ! { - loop {} rt::spawn(keyboard_demo()); let mut core = rt::Core::new(); diff --git a/src/rt.rs b/src/rt.rs index afcb9e92..aa6f723e 100644 --- a/src/rt.rs +++ b/src/rt.rs @@ -7,6 +7,7 @@ use core::{ }; use maitake::{ scheduler::{self, StaticScheduler, Stealer}, + sync::spin, time, }; use mycelium_util::{fmt, sync::InitOnce}; @@ -52,7 +53,7 @@ struct Runtime { /// 512 CPU cores ought to be enough for anybody... pub const MAX_CORES: usize = 512; -// pub static TIMER: time::Timer = time::Timer::new(arch::interrupt::TIMER_INTERVAL); +static TIMER: spin::InitOnce = spin::InitOnce::uninitialized(); static RUNTIME: Runtime = { // This constant is used as an array initializer; the clippy warning that it @@ -89,8 +90,9 @@ where } /// Initialize the kernel runtime. -pub fn init() { - // time::set_global_timer(&TIMER).expect("`rt::init` should only be called once!"); +pub fn init(clock: maitake::time::Clock) { + let timer = TIMER.init(time::Timer::new(clock)); + time::set_global_timer(timer).expect("`rt::init` should only be called once!"); tracing::info!("kernel runtime initialized"); } @@ -127,7 +129,7 @@ impl Core { // turn the timer wheel if it wasn't turned recently and no one else is // holding a lock, ensuring any pending timer ticks are consumed. - // TIMER.turn(); + TIMER.get().turn(); // if there are remaining tasks to poll, continue without stealing. if tick.has_remaining { From 3349ab98f5279619b69b5bb5b49e8622fce8cc2e Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 31 Jan 2024 16:01:09 -0800 Subject: [PATCH 05/10] feat(trace) add timestamps to tracing (#475) --- trace/Cargo.toml | 1 + trace/src/lib.rs | 32 ++++++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/trace/Cargo.toml b/trace/Cargo.toml index aefb37d4..2abd4465 100644 --- a/trace/Cargo.toml +++ b/trace/Cargo.toml @@ -12,6 +12,7 @@ embedded-graphics = ["dep:embedded-graphics", "hal-core/embedded-graphics-core"] default = ["embedded-graphics",] [dependencies] +maitake = { path = "../maitake" } tracing-core = {git = "https://github.com/tokio-rs/tracing", default_features = false } tracing = { git = "https://github.com/tokio-rs/tracing", default_features = false } hal-core = { path = "../hal-core" } diff --git a/trace/src/lib.rs b/trace/src/lib.rs index 0ffc0879..ff9968bc 100644 --- a/trace/src/lib.rs +++ b/trace/src/lib.rs @@ -124,7 +124,7 @@ impl Subscriber { event: "├", }; const DISPLAY_INDENT_CFG: IndentCfg = IndentCfg { - indent: " ", + indent: "", new_span: "> ", event: " ", }; @@ -177,13 +177,14 @@ where }; let mut writer = self.writer(meta); + let _ = write_timestamp(&mut writer); let _ = write_level(&mut writer, meta.level()); let _ = writer.indent_initial(IndentKind::NewSpan); let _ = writer.with_bold().write_str(meta.name()); let _ = writer.with_fg_color(Color::BrightBlack).write_str(": "); // ensure the span's fields are nicely indented if they wrap by - // "entering" and then "exiting" + // "entering" and then "exiting"`````findent` // the span. self.enter(&id); span.record(&mut Visitor::new(&mut writer, false)); @@ -203,6 +204,7 @@ where fn event(&self, event: &Event) { let meta = event.metadata(); let mut writer = self.writer(meta); + let _ = write_timestamp(&mut writer); let _ = write_level(&mut writer, meta.level()); let _ = writer.indent_initial(IndentKind::Event); let _ = write!( @@ -239,7 +241,7 @@ impl Output { W: MakeWriter<'a>, { let cfg = OutputCfg { - line_len: make_writer.line_len(), + line_len: make_writer.line_len() - 16, indent: AtomicU64::new(0), indent_cfg, }; @@ -396,7 +398,8 @@ impl<'a, W: Write> Writer<'a, W> { } fn write_newline(&mut self) -> fmt::Result { - self.writer.write_str(" ")?; + // including width of the 16-character timestamp bit + self.writer.write_str(" ")?; self.current_line = 3; self.indent(IndentKind::Indent) } @@ -492,6 +495,27 @@ where w.write_char(']') } +#[inline] +fn write_timestamp(w: &mut W) -> fmt::Result +where + W: fmt::Write + SetColor, +{ + w.write_char('[')?; + if let Ok(now) = maitake::time::Instant::try_now() { + let now = now.elapsed(); + write!( + w.with_fg_color(Color::BrightBlack), + "{:>6}.{:06}", + now.as_secs(), + now.subsec_micros() + )?; + } else { + write!(w.with_fg_color(Color::BrightBlack), " ?.??????")?; + } + w.write_char(']')?; + Ok(()) +} + impl<'writer, W> Visitor<'writer, W> where W: fmt::Write, From 591450b7bf561d24edcbf7bac4a3de569f1045e7 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 31 Jan 2024 16:05:11 -0800 Subject: [PATCH 06/10] fix(maitake): fix time travel (lol) (#475) I'm a dumbass lmao --- maitake/src/time/clock.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/maitake/src/time/clock.rs b/maitake/src/time/clock.rs index e6043c01..f4b31dee 100644 --- a/maitake/src/time/clock.rs +++ b/maitake/src/time/clock.rs @@ -133,9 +133,7 @@ impl Clock { #[inline] #[must_use] pub(in crate::time) fn ticks_to_dur(tick_duration: Duration, ticks: Ticks) -> Duration { - let nanos = tick_duration.subsec_nanos() as u64 * ticks; - let secs = tick_duration.as_secs() * ticks; - Duration::new(secs, nanos as u32) + tick_duration.saturating_mul(ticks as u32) } #[track_caller] From 698acc33f59af62a1005d898dbfd5741a2954415 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 2 Feb 2024 09:00:31 -0800 Subject: [PATCH 07/10] feat(maitake): use `Clock::now` in `Sleep::new` (#475) This commit changes the construction of new `Sleep`s to use the nice `Clock` API. --- hal-x86_64/src/time/tsc.rs | 6 ++++-- maitake/src/time/clock.rs | 36 +++++++++++++++++++++++++-------- maitake/src/time/timer.rs | 6 ++++++ maitake/src/time/timer/sleep.rs | 17 ++++++++++++++-- maitake/src/time/timer/wheel.rs | 19 +++++++++-------- src/arch/x86_64/boot.rs | 2 +- src/rt.rs | 5 +++++ 7 files changed, 70 insertions(+), 21 deletions(-) diff --git a/hal-x86_64/src/time/tsc.rs b/hal-x86_64/src/time/tsc.rs index efa2d24f..694597ab 100644 --- a/hal-x86_64/src/time/tsc.rs +++ b/hal-x86_64/src/time/tsc.rs @@ -39,6 +39,10 @@ impl Rdtsc { /// Returns a [`maitake::time::Clock`] defining a clock that uses `rdtsc` /// timestamps to produce `maitake` ticks. pub fn into_maitake_clock(self) -> Result { + // TODO(eliza): fix this + #![allow(unreachable_code)] + return Err("calibration routine doesn't really work yet, sorry!"); + const NOT_YET_CALIBRATED: u32 = u32::MAX; // 50ms is stolen from Linux, so i'm assuming its a reasonable duration @@ -48,8 +52,6 @@ impl Rdtsc { static MAITAKE_TICK_SHIFT: AtomicU32 = AtomicU32::new(NOT_YET_CALIBRATED); - return Err("calibration routine doesn't really work yet, sorry!"); - fn now() -> u64 { let rdtsc = unsafe { intrinsics::rdtsc() }; let shift = MAITAKE_TICK_SHIFT.load(Relaxed); diff --git a/maitake/src/time/clock.rs b/maitake/src/time/clock.rs index f4b31dee..e8b23f37 100644 --- a/maitake/src/time/clock.rs +++ b/maitake/src/time/clock.rs @@ -62,7 +62,7 @@ use core::{ pub struct Clock { now: fn() -> Ticks, tick_duration: Duration, - name: Option<&'static str>, + name: &'static str, } /// A measurement of a monotonically nondecreasing clock. @@ -92,19 +92,18 @@ pub struct Instant(Duration); pub type Ticks = u64; impl Clock { + #[must_use] pub const fn new(tick_duration: Duration, now: fn() -> Ticks) -> Self { Self { now, tick_duration, - name: None, + name: "", } } + #[must_use] pub const fn named(self, name: &'static str) -> Self { - Self { - name: Some(name), - ..self - } + Self { name, ..self } } #[must_use] @@ -124,16 +123,37 @@ impl Clock { Instant(ticks_to_dur(tick_duration, now)) } + #[must_use] pub fn max_duration(&self) -> Duration { max_duration(self.tick_duration()) } + + #[must_use] + pub fn name(&self) -> &'static str { + self.name + } } #[track_caller] #[inline] #[must_use] pub(in crate::time) fn ticks_to_dur(tick_duration: Duration, ticks: Ticks) -> Duration { - tick_duration.saturating_mul(ticks as u32) + const NANOS_PER_SEC: u32 = 1_000_000_000; + // Multiply nanoseconds as u64, because it cannot overflow that way. + let total_nanos = tick_duration.subsec_nanos() as u64 * ticks; + let extra_secs = total_nanos / (NANOS_PER_SEC as u64); + let nanos = (total_nanos % (NANOS_PER_SEC as u64)) as u32; + let Some(secs) = tick_duration.as_secs().checked_mul(ticks) else { + panic!( + "ticks_to_dur({tick_duration:?}, {ticks}): multiplying tick \ + duration seconds by ticks would overflow" + ); + }; + let Some(secs) = secs.checked_add(extra_secs) else { + panic!("ticks_to_dur({tick_duration:?}, {ticks}): extra seconds from nanos ({extra_secs}s) would overflow total seconds") + }; + debug_assert!(nanos < NANOS_PER_SEC); + Duration::new(secs, nanos) } #[track_caller] @@ -155,7 +175,7 @@ pub(in crate::time) fn dur_to_ticks( #[inline] #[must_use] pub(in crate::time) fn max_duration(tick_duration: Duration) -> Duration { - ticks_to_dur(tick_duration, Ticks::MAX) + tick_duration.saturating_mul(u32::MAX) } impl Instant { diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 99737d09..1ad99143 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -308,6 +308,12 @@ impl Timer { self.clock.now() } + /// Returns the hardware [`Clock`] definition used by this timer. + #[must_use] + pub fn clock(&self) -> &Clock { + &self.clock + } + /// Returns the maximum duration of [`Sleep`] futures driven by this timer. pub fn max_duration(&self) -> Duration { self.clock.max_duration() diff --git a/maitake/src/time/timer/sleep.rs b/maitake/src/time/timer/sleep.rs index 8af6261e..d44155ce 100644 --- a/maitake/src/time/timer/sleep.rs +++ b/maitake/src/time/timer/sleep.rs @@ -67,7 +67,15 @@ enum State { impl<'timer> Sleep<'timer> { pub(super) fn new(timer: &'timer Timer, ticks: Ticks) -> Self { - let deadline = timer.core().now() + ticks; + let now = timer.clock().now_ticks(); + let deadline = now + ticks; + debug!( + target: "maitake::time::sleep", + now, + sleep.ticks = ticks, + sleep.deadline = deadline, + "Sleep::new({ticks})" + ); Self { state: State::Unregistered, timer, @@ -93,7 +101,12 @@ impl Future for Sleep<'_> { fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let mut this = self.as_mut().project(); - trace!(sleep.addr = ?format_args!("{:p}", this.entry), "Sleep::poll"); + trace!( + target: "maitake::time::sleep", + addr = ?fmt::ptr(&*this.entry), + state = ?*this.state, + "Sleep::poll" + ); // If necessary, register the sleep match test_dbg!(*this.state) { State::Unregistered => { diff --git a/maitake/src/time/timer/wheel.rs b/maitake/src/time/timer/wheel.rs index 2bebae24..d596067d 100644 --- a/maitake/src/time/timer/wheel.rs +++ b/maitake/src/time/timer/wheel.rs @@ -157,17 +157,20 @@ impl Core { self.now = now; // reschedule pending sleeps. - debug!( - now = self.now, - fired, - rescheduled = pending_reschedule.len(), - ?next_deadline, - "wheel turned to" - ); // If we need to reschedule something, we may need to recalculate the next deadline let any = !pending_reschedule.is_empty(); + if any || fired > 0 { + debug!( + now = self.now, + fired, + rescheduled = pending_reschedule.len(), + ?next_deadline, + "the Wheel of Time has turned" + ); + } + for entry in pending_reschedule { let deadline = unsafe { entry.as_ref().deadline }; debug_assert!(deadline > self.now); @@ -238,7 +241,7 @@ impl Core { fn next_deadline(&self) -> Option { self.wheels.iter().find_map(|wheel| { let next_deadline = wheel.next_deadline(self.now)?; - trace!( + test_trace!( now = self.now, next_deadline.ticks, next_deadline.wheel, diff --git a/src/arch/x86_64/boot.rs b/src/arch/x86_64/boot.rs index b9eac58e..fd8d3ad0 100644 --- a/src/arch/x86_64/boot.rs +++ b/src/arch/x86_64/boot.rs @@ -95,7 +95,7 @@ impl BootInfo for BootloaderApiBootInfo { // TODO(eliza): it would be nice if this was configured by // non-arch-specific OS code... const DISABLED_TARGETS: &[&str] = &[ - "maitake::time", + // "maitake::time::timer", "maitake::task", "runtime::waker", "mycelium_alloc", diff --git a/src/rt.rs b/src/rt.rs index aa6f723e..54ef55ab 100644 --- a/src/rt.rs +++ b/src/rt.rs @@ -91,6 +91,11 @@ where /// Initialize the kernel runtime. pub fn init(clock: maitake::time::Clock) { + tracing::info!( + clock = %clock.name(), + clock.max_duration = ?clock.max_duration(), + "initializing kernel runtime...", + ); let timer = TIMER.init(time::Timer::new(clock)); time::set_global_timer(timer).expect("`rt::init` should only be called once!"); From 32655528de26b99a0b445078eb544c5bed1b3424 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 22 Feb 2024 08:35:09 -0800 Subject: [PATCH 08/10] feat(maitake): turn timer when inserting `Sleep`s (#475) This commit changes the `Sleep` future in `maitake::time` to turn the timer to the `Clock`'s current `now()` time when the timer wheel lock is acquired to register a new `Sleep`. This way, the timer is (at least occasionally) turned while polling `Sleep`s, in addition to when scheduled by an interrupt or by a scheduler tick completing. This way, we fire completing sleeps much more frequently, which should help improve the timer's accuracy substantially. Because we only do this when the timer wheel lock already has to be acquired to register the `Sleep` in the first place, this shouldn't increase contention for the wheel lock all that much. Fixes #474 --- hal-x86_64/src/time/pit.rs | 2 +- maitake/src/time/clock.rs | 93 ++++++++++++++++----------------- maitake/src/time/timer.rs | 12 +++-- maitake/src/time/timer/sleep.rs | 14 ++++- maitake/src/time/timer/wheel.rs | 5 -- 5 files changed, 65 insertions(+), 61 deletions(-) diff --git a/hal-x86_64/src/time/pit.rs b/hal-x86_64/src/time/pit.rs index 692d13b3..843d74d7 100644 --- a/hal-x86_64/src/time/pit.rs +++ b/hal-x86_64/src/time/pit.rs @@ -3,7 +3,7 @@ use super::InvalidDuration; use crate::cpu::{self, Port}; use core::{ convert::TryFrom, - sync::atomic::{AtomicBool, AtomicU64, Ordering}, + sync::atomic::{AtomicBool, Ordering}, time::Duration, }; use mycelium_util::{ diff --git a/maitake/src/time/clock.rs b/maitake/src/time/clock.rs index e8b23f37..56ff56f3 100644 --- a/maitake/src/time/clock.rs +++ b/maitake/src/time/clock.rs @@ -7,52 +7,6 @@ use core::{ ops::{Add, AddAssign, Sub, SubAssign}, }; -// /// A hardware clock. -// pub trait Clock: Send + Sync + 'static { -// /// Returns the current hardware timestamp, in ticks of this `Clock`. -// /// -// /// # Monotonicity -// /// -// /// Implementations of this method MUST ensure that timestampss returned by -// /// `now()` MUST be [monontonically non-decreasing]. This means that a call -// /// to `now()` MUST NOT ever return a value less than the value returned by -// /// a previous call to`now()`. -// /// -// /// Note that this means that timestamps returned by `now()` are expected -// /// not to overflow. Of course, all integers *will* overflow eventually, so -// /// this requirement can reasonably be weakened to expecting that timestamps -// /// returned by `now()` will not overflow unless the system has been running -// /// for a duration substantially longer than the system is expected to run -// /// for. For example, if a system is expected to run for as long as a year -// /// without being restarted, it's not unreasonable for timestamps returned -// /// by `now()` to overflow after, say, 100 years. Ideally, a general-purpose -// /// `Clock` implementation would not overflow for, say, 1,000 years. -// /// -// /// The implication of this is that if the timestamp counters provided by -// /// the hardware platform are less than 64 bits wide (e.g., 16- or 32-bit -// /// timestamps), the `Clock` implementation is responsible for ensuring that -// /// they are extended to 64 bits, such as by counting overflows in the -// /// `Clock` implementation. -// /// -// /// [monotonic]: https://en.wikipedia.org/wiki/Monotonic_function -// #[must_use] -// fn now_ticks(&self) -> Ticks; - -// #[must_use] -// fn tick_duration(&self) -> Duration; - -// #[must_use] -// fn now(&self) -> Instant { -// let now = self.now_ticks(); -// let tick_duration = self.tick_duration(); -// Instant { now, tick_duration } -// } - -// fn max_duration(&self) -> Duration { -// max_duration(self.tick_duration()) -// } -// } - /// A hardware clock definition. /// /// A `Clock` consists of a function that returns the hardware clock's current @@ -68,7 +22,7 @@ pub struct Clock { /// A measurement of a monotonically nondecreasing clock. /// Opaque and useful only with [`Duration`]. /// -/// Provided that the [`Clock` implementation is correct, `Instant`s are always +/// Provided that the [`Clock`] implementation is correct, `Instant`s are always /// guaranteed to be no less than any previously measured instant when created, /// and are often useful for tasks such as measuring benchmarks or timing how /// long an operation takes. @@ -92,6 +46,38 @@ pub struct Instant(Duration); pub type Ticks = u64; impl Clock { + /// Returns a new [`Clock`] with the provided tick [`Duration`] and `now()` + /// function. + /// + /// # Implementing `now()` + /// + /// The `now` function provided when constructing a [`Clock`] is a function + /// that returns the current hardware timestamp in a 64-bit number of + /// _ticks_. The period of time represented by a tick is indicated by the + /// `tick_duration` argument. + /// + /// Implementations of `now()` MUST ensure that timestamps returned by + /// `now()` MUST be [monontonically non-decreasing]. This means that a call + /// to `now()` MUST NOT ever return a value less than the value returned by + /// a previous call to`now()`. + /// + /// Note that this means that timestamps returned by `now()` are expected + /// not to overflow. Of course, all integers *will* overflow eventually, so + /// this requirement can reasonably be weakened to expecting that timestamps + /// returned by `now()` will not overflow unless the system has been running + /// for a duration substantially longer than the system is expected to run + /// for. For example, if a system is expected to run for as long as a year + /// without being restarted, it's not unreasonable for timestamps returned + /// by `now()` to overflow after, say, 100 years. Ideally, a general-purpose + /// `Clock` implementation would not overflow for, say, 1,000 years. + /// + /// The implication of this is that if the timestamp counters provided by + /// the hardware platform are less than 64 bits wide (e.g., 16- or 32-bit + /// timestamps), the `Clock` implementation is responsible for ensuring that + /// they are extended to 64 bits, such as by counting overflows in the + /// `Clock` implementation. + /// + /// [monotonic]: https://en.wikipedia.org/wiki/Monotonic_function #[must_use] pub const fn new(tick_duration: Duration, now: fn() -> Ticks) -> Self { Self { @@ -101,21 +87,30 @@ impl Clock { } } + /// Add an arbitrary user-defined name to this `Clock`. + /// + /// This is generally used to describe the hardware time source used by the + /// `now()` function for this `Clock`. #[must_use] pub const fn named(self, name: &'static str) -> Self { Self { name, ..self } } + /// Returns the current `now` timestamp, in [`Ticks`] of this clock's base + /// tick duration. #[must_use] pub(crate) fn now_ticks(&self) -> Ticks { (self.now)() } + /// Returns the [`Duration`] of one tick of this clock. #[must_use] pub fn tick_duration(&self) -> Duration { self.tick_duration } + /// Returns an [`Instant`] representing the current timestamp according to + /// this [`Clock`]. #[must_use] pub fn now(&self) -> Instant { let now = self.now_ticks(); @@ -123,11 +118,14 @@ impl Clock { Instant(ticks_to_dur(tick_duration, now)) } + /// Returns the maximum duration of this clock. #[must_use] pub fn max_duration(&self) -> Duration { max_duration(self.tick_duration()) } + /// Returns this `Clock`'s name, if it was given one using the [`named`] + /// method. #[must_use] pub fn name(&self) -> &'static str { self.name @@ -158,7 +156,6 @@ pub(in crate::time) fn ticks_to_dur(tick_duration: Duration, ticks: Ticks) -> Du #[track_caller] #[inline] -#[must_use] pub(in crate::time) fn dur_to_ticks( tick_duration: Duration, dur: Duration, diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 1ad99143..27fb91d5 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -304,11 +304,13 @@ impl Timer { } } + /// Returns the current timestamp according to this timer's hardware + /// [`Clock`], as an [`Instant`]. pub fn now(&self) -> Instant { self.clock.now() } - /// Returns the hardware [`Clock`] definition used by this timer. + /// Borrows the hardware [`Clock`] definition used by this timer. #[must_use] pub fn clock(&self) -> &Clock { &self.clock @@ -453,8 +455,8 @@ impl Timer { // instead, if the timer wheel is busy (e.g. the timer ISR was called on // another core, or if a `Sleep` future is currently canceling itself), // we just add to a counter of pending ticks, and bail. - if let Some(core) = self.core.try_lock() { - Some(self.advance_locked(core)) + if let Some(mut core) = self.core.try_lock() { + Some(self.advance_locked(&mut core)) } else { trace!("could not lock timer wheel"); None @@ -488,7 +490,7 @@ impl Timer { /// [`advance`]: Timer::advance #[inline] pub fn turn(&self) -> Turn { - self.advance_locked(self.core.lock()) + self.advance_locked(&mut self.core.lock()) } pub(in crate::time) fn ticks_to_dur(&self, ticks: Ticks) -> Duration { @@ -499,7 +501,7 @@ impl Timer { clock::dur_to_ticks(self.clock.tick_duration(), duration) } - fn advance_locked(&self, mut core: MutexGuard<'_, wheel::Core>) -> Turn { + fn advance_locked(&self, core: &mut MutexGuard<'_, wheel::Core>) -> Turn { // take any pending ticks. let pending_ticks = self.pending_ticks.swap(0, AcqRel) as Ticks; // we do two separate `advance` calls here instead of advancing once diff --git a/maitake/src/time/timer/sleep.rs b/maitake/src/time/timer/sleep.rs index d44155ce..66f0ca42 100644 --- a/maitake/src/time/timer/sleep.rs +++ b/maitake/src/time/timer/sleep.rs @@ -47,7 +47,7 @@ pub(super) struct Entry { /// The wheel's elapsed timestamp when this `sleep` future was first polled. pub(super) deadline: Ticks, - pub(super) ticks: Ticks, + pub(in crate::time::timer) ticks: Ticks, pub(super) linked: AtomicBool, @@ -112,7 +112,17 @@ impl Future for Sleep<'_> { State::Unregistered => { let ptr = unsafe { ptr::NonNull::from(Pin::into_inner_unchecked(this.entry.as_mut())) }; - match test_dbg!(this.timer.core().register_sleep(ptr)) { + // Acquire the wheel lock to insert the sleep. + let mut core: maitake_sync::spin::MutexGuard<'_, crate::time::timer::wheel::Core> = + this.timer.core(); + + // While we are holding the wheel lock, go ahead and advance the + // timer, too. This way, the timer wheel gets advanced more + // frequently than just when a scheduler tick completes or a + // timer IRQ fires, helping to increase timer accuracy. + this.timer.advance_locked(&mut core); + + match test_dbg!(core.register_sleep(ptr)) { Poll::Ready(()) => { *this.state = State::Completed; return Poll::Ready(()); diff --git a/maitake/src/time/timer/wheel.rs b/maitake/src/time/timer/wheel.rs index d596067d..8ad91003 100644 --- a/maitake/src/time/timer/wheel.rs +++ b/maitake/src/time/timer/wheel.rs @@ -95,10 +95,6 @@ impl Core { } } - pub(super) fn now(&self) -> Ticks { - self.now - } - #[inline(never)] pub(super) fn turn_to(&mut self, now: Ticks) -> (usize, Option) { let mut fired = 0; @@ -191,7 +187,6 @@ impl Core { let deadline = sleep.deadline; trace!( sleep.addr = ?format_args!("{sleep:p}"), - sleep.ticks = *sleep.as_ref().project_ref().ticks, sleep.deadline = deadline, now = self.now, "canceling sleep" From 02dd774878b94e798a3c88ade855f4ad761a1783 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 25 Feb 2024 08:03:02 -0800 Subject: [PATCH 09/10] test(maitake): unbreak loom tests (#475) --- maitake/src/loom.rs | 5 +++++ maitake/src/time/timer.rs | 2 +- maitake/src/time/timer/sleep.rs | 3 +-- maitake/src/time/timer/tests.rs | 8 ++++++-- maitake/src/time/timer/tests/concurrent.rs | 1 + maitake/src/time/timer/tests/wheel_tests.rs | 3 +-- 6 files changed, 15 insertions(+), 7 deletions(-) diff --git a/maitake/src/loom.rs b/maitake/src/loom.rs index 73ca374c..11a0708d 100644 --- a/maitake/src/loom.rs +++ b/maitake/src/loom.rs @@ -4,6 +4,7 @@ pub(crate) use self::inner::*; #[cfg(loom)] mod inner { #![allow(dead_code)] + pub(crate) use loom::thread_local; #[cfg(feature = "alloc")] pub(crate) mod alloc { @@ -101,6 +102,10 @@ mod inner { #[cfg(not(loom))] mod inner { #![allow(dead_code, unused_imports)] + + #[cfg(test)] + pub(crate) use std::thread_local; + pub(crate) mod sync { #[cfg(feature = "alloc")] pub use alloc::sync::*; diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 27fb91d5..1ba650d8 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -501,7 +501,7 @@ impl Timer { clock::dur_to_ticks(self.clock.tick_duration(), duration) } - fn advance_locked(&self, core: &mut MutexGuard<'_, wheel::Core>) -> Turn { + fn advance_locked(&self, core: &mut wheel::Core) -> Turn { // take any pending ticks. let pending_ticks = self.pending_ticks.swap(0, AcqRel) as Ticks; // we do two separate `advance` calls here instead of advancing once diff --git a/maitake/src/time/timer/sleep.rs b/maitake/src/time/timer/sleep.rs index 66f0ca42..92b291f1 100644 --- a/maitake/src/time/timer/sleep.rs +++ b/maitake/src/time/timer/sleep.rs @@ -113,8 +113,7 @@ impl Future for Sleep<'_> { let ptr = unsafe { ptr::NonNull::from(Pin::into_inner_unchecked(this.entry.as_mut())) }; // Acquire the wheel lock to insert the sleep. - let mut core: maitake_sync::spin::MutexGuard<'_, crate::time::timer::wheel::Core> = - this.timer.core(); + let mut core = this.timer.core(); // While we are holding the wheel lock, go ahead and advance the // timer, too. This way, the timer wheel gets advanced more diff --git a/maitake/src/time/timer/tests.rs b/maitake/src/time/timer/tests.rs index f200409f..c121161a 100644 --- a/maitake/src/time/timer/tests.rs +++ b/maitake/src/time/timer/tests.rs @@ -5,8 +5,8 @@ use core::cell::RefCell; use core::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; -thread_local! { - static CLOCK: RefCell>> = const { RefCell::new(None) }; +crate::loom::thread_local! { + static CLOCK: RefCell>> = RefCell::new(None); } #[derive(Debug, Clone)] @@ -58,6 +58,8 @@ impl ClockHandle { self.advance_ticks_inner(duration.as_millis() as u64) } + // This function isn't used in loom tests. + #[cfg_attr(loom, allow(dead_code))] pub fn advance_ticks(&self, ticks: Ticks) { info!(ticks, "advancing test clock by"); self.advance_ticks_inner(ticks) @@ -71,6 +73,8 @@ impl ClockHandle { ); } + // This function isn't used in loom tests. + #[cfg_attr(loom, allow(dead_code))] pub fn ticks(&self) -> Ticks { self.0.now.load(Ordering::SeqCst) } diff --git a/maitake/src/time/timer/tests/concurrent.rs b/maitake/src/time/timer/tests/concurrent.rs index ea12211c..dac6abc4 100644 --- a/maitake/src/time/timer/tests/concurrent.rs +++ b/maitake/src/time/timer/tests/concurrent.rs @@ -108,6 +108,7 @@ fn two_sleeps_sequential() { } thread.join().unwrap(); + drop(clock); }) } diff --git a/maitake/src/time/timer/tests/wheel_tests.rs b/maitake/src/time/timer/tests/wheel_tests.rs index ec119163..2ea66f53 100644 --- a/maitake/src/time/timer/tests/wheel_tests.rs +++ b/maitake/src/time/timer/tests/wheel_tests.rs @@ -188,7 +188,7 @@ impl SleepGroupTest { } #[test] -fn pend_advance_wakes() { +fn turn_wakes() { static TIMER: Timer = Timer::new(TestClock::clock()); let mut test = SleepGroupTest::new(&TIMER); @@ -205,7 +205,6 @@ fn pend_advance_wakes() { // advance the timer by 50 more ticks // but ONLY by pending test.clock.advance_ticks(50); - test.timer.pend_ticks(50); // Tick the scheduler, nothing should have happened let tick = test.scheduler.tick(); From 85db2c792ca4678598a715124f863cfdbe53c48d Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 5 Mar 2024 10:19:28 -0800 Subject: [PATCH 10/10] docs(maitake): update docs for new timer APIs (#475) This commit adds examples for the new `time::Clock` API, and removes references to `Timer::advance`/`Timer::force_advance`. --- maitake/src/time.rs | 2 +- maitake/src/time/clock.rs | 222 +++++++++++++++++++++++++++++++------- maitake/src/time/timer.rs | 148 ++++++++----------------- 3 files changed, 225 insertions(+), 147 deletions(-) diff --git a/maitake/src/time.rs b/maitake/src/time.rs index d087d663..80c1f5f7 100644 --- a/maitake/src/time.rs +++ b/maitake/src/time.rs @@ -43,7 +43,7 @@ //! [wheel]: http://www.cs.columbia.edu/~nahum/w6998/papers/sosp87-timing-wheels.pdf //! [driving-timers]: Timer#driving-timers #![warn(missing_docs, missing_debug_implementations)] -mod clock; +pub mod clock; pub mod timeout; mod timer; diff --git a/maitake/src/time/clock.rs b/maitake/src/time/clock.rs index 56ff56f3..eebf06cb 100644 --- a/maitake/src/time/clock.rs +++ b/maitake/src/time/clock.rs @@ -1,3 +1,6 @@ +//! [`Clock`]s provide a mechanism for tracking the current time. +//! +//! See the documentation for the [`Clock`] type for more details. use super::{ timer::{self, TimerError}, Duration, @@ -10,8 +13,179 @@ use core::{ /// A hardware clock definition. /// /// A `Clock` consists of a function that returns the hardware clock's current -/// timestamp in [`Ticks`], and a [`Duration`] that defines the amount of time -/// represented by a single tick of the clock. +/// timestamp in [`Ticks`] (`now()`), and a [`Duration`] that defines the amount +/// of time represented by a single tick of the clock. +/// +/// # Using `Clock`s +/// +/// A `Clock` must be provided when [constructing a `Timer`](super::Timer::new). +/// The `Clock` provided to [`Timer::new`](super::Timer::new) is used to +/// determine the time to advance to when turning the timer wheel, and to +/// determine the start time when adding a [`Sleep`](super::Sleep) future to the +/// timer wheel. +/// +/// In addition, once a global timer is set using the +/// [`set_global_timer`](super::set_global_timer) function, the +/// [`Instant::now()`] function may be used to produce [`Instant`]s representing +/// the current timestamp according to that timer's [`Clock`]. The [`Instant`] +/// type is analogous to the [`std::time::Instant`] type in the Rust standard +/// library, and may be used to compare the time elapsed between two events, as +/// a timestamp for logs and other diagnostics, and similar purposes. +/// +/// # Implementing `now()` +/// +/// Constructing a [new `Clock` definition](Self::new) takes a function, called +/// `now()`, that returns the current hardware timestamp in a 64-bit number of, +/// _ticks_. The period of time represented by a tick is indicated by the +/// `tick_duration` argument to [`Clock::new`]. In order to define a `Clock` +/// representing a particular hardware time source, a `now()` function must be +/// implemented using that time source. +/// +/// ## Monotonicity +/// +/// Implementations of `now()` MUST ensure that timestamps returned by +/// `now()` MUST be [monontonically non-decreasing][monotonic]. This means that +/// a call to `now()` MUST NOT ever return a value less than the value returned +/// by a previous call to`now()`. +/// +/// Note that this means that timestamps returned by `now()` are expected +/// not to overflow. Of course, all integers *will* overflow eventually, so +/// this requirement can reasonably be weakened to expecting that timestamps +/// returned by `now()` will not overflow unless the system has been running +/// for a duration substantially longer than the system is expected to run +/// for. For example, if a system is expected to run for as long as a year +/// without being restarted, it's not unreasonable for timestamps returned +/// by `now()` to overflow after, say, 100 years. Ideally, a general-purpose +/// `Clock` implementation would not overflow for, say, 1,000 years. +/// +/// The implication of this is that if the timestamp counters provided by +/// the hardware platform are less than 64 bits wide (e.g., 16- or 32-bit +/// timestamps), the `Clock` implementation is responsible for ensuring that +/// they are extended to 64 bits, such as by counting overflows in the +/// `Clock` implementation. +/// +/// ## Examples +/// +/// The simplest possible `Clock` implementation is one for a +/// timestamp-counter-style hardware clock, where the timestamp counter is +/// incremented on a fixed interval by the hardware. For such a counter, the +/// `Clock` definition might look something like this: +/// +///```rust +/// use maitake::time::{Clock, Duration}; +/// # mod arch { +/// # pub fn read_timestamp_counter() -> u64 { 0 } +/// # pub const TIMESTAMP_COUNTER_FREQUENCY_HZ: u64 = 50_000_000; +/// # } +/// +/// // The fixed interval at which the timestamp counter is incremented. +/// // +/// // This is a made-up value; for real hardware, this value would be +/// // determined from information provided by the hardware manufacturer, +/// // and may need to be calculated based on the system's clock frequency. +/// const TICK_DURATION: Duration = { +/// let dur_ns = 1_000_000_000 / arch::TIMESTAMP_COUNTER_FREQUENCY_HZ; +/// Duration::from_nanos(dur_ns) +/// }; +/// +/// // Define a `Clock` implementation for the timestamp counter. +/// let clock = Clock::new(TICK_DURATION, || { +/// // A (pretend) function that reads the value of the timestamp +/// // counter. In real life, this might be a specific instruction, +/// // or a read from a timestamp counter register. +/// arch::read_timestamp_counter() +/// }) +/// // Adding a name to the clock definition allows it to be i +/// // identified in fmt::Debug output. +/// .named("timestamp-counter"); +///``` +/// +/// On some platforms, the frequency with which a timestamp counter is +/// incremented may be configured by setting a divisor that divides the base +/// frequency of the clock. On such a platform, it is possible to select the +/// tick duration when constructing a new `Clock`. We could then provide a +/// function that returns a clock with a requested tick duration: +/// +///```rust +/// use maitake::time::{Clock, Duration}; +/// # mod arch { +/// # pub fn read_timestamp_counter() -> u64 { 0 } +/// # pub fn set_clock_divisor(divisor: u32) {} +/// # pub const CLOCK_BASE_DURATION_NS: u32 = 100; +/// # } +/// +/// fn new_clock(tick_duration: Duration) -> Clock { +/// // Determine the divisor necessary to achieve the requested tick +/// // duration, based on the hardware clock's base frequency. +/// let divisor = { +/// let duration_ns = tick_duration.as_nanos(); +/// assert!( +/// duration_ns as u32 >= arch::CLOCK_BASE_DURATION_NS, +/// "tick duration too short" +/// ); +/// let div_u128 = duration_ns / arch::CLOCK_BASE_DURATION_NS as u128; +/// u32::try_from(div_u128).expect("tick duration too long") +/// }; +/// +/// // Set the divisor to the hardware clock. On real hardware, this +/// // might be a write to a register or memory-mapped IO location +/// // that controls the clock's divisor. +/// arch::set_clock_divisor(divisor as u32); +/// +/// // Define a `Clock` implementation for the timestamp counter. +/// Clock::new(tick_duration, arch::read_timestamp_counter) +/// .named("timestamp-counter") +/// } +///``` +/// +/// In addition to timestamp-counter-based hardware clocks, a `Clock` definition +/// can be provided for an interrupt-based hardware clock. In this case, we +/// would provide an interrupt handler for the hardware timer interrupt that +/// increments an [`AtomicU64`](core::sync::atomic::AtomicU64) counter, and a +/// `now()` function that reads the counter. Essentially, we are reimplementing +/// a hardware timestamp counter in software, using the hardware timer interrupt +/// to increment the counter. For example: +/// +/// ```rust +/// use maitake::time::{Clock, Duration}; +/// use core::sync::atomic::{AtomicU64, Ordering}; +/// # mod arch { +/// # pub fn start_periodic_timer() -> u64 { 0 } +/// # pub fn set_timer_period_ns(_: u64) {} +/// # pub fn set_interrupt_handler(_: Interrupt, _: fn()) {} +/// # pub enum Interrupt { Timer } +/// # } +/// +/// // A counter that is incremented by the hardware timer interrupt. +/// static CLOCK_TICKS: AtomicU64 = AtomicU64::new(0); +/// +/// // The hardware timer interrupt handler. +/// fn timer_interrupt_handler() { +/// // Increment the counter. +/// CLOCK_TICKS.fetch_add(1, Ordering::Relaxed); +/// } +/// +/// // Returns the current timestamp by reading the counter. +/// fn now() -> u64 { +/// CLOCK_TICKS.load(Ordering::Relaxed) +/// } +/// +/// fn new_clock(tick_duration: Duration) -> Clock { +/// // Set the hardware timer to generate periodic interrupts. +/// arch::set_timer_period_ns(tick_duration.as_nanos() as u64); +/// +/// // Set the interrupt handler for the hardware timer interrupt, +/// // and start the timer. +/// arch::set_interrupt_handler(arch::Interrupt::Timer, timer_interrupt_handler); +/// arch::start_periodic_timer(); +/// +/// // Define a `Clock` implementation for the interrupt-based timer. +/// Clock::new(tick_duration, now) +/// .named("periodic-timer") +/// } +/// ``` +/// [`std::time::Instant`]: https://doc.rust-lang.org/std/time/struct.Instant.html +/// [monotonic]: https://en.wikipedia.org/wiki/Monotonic_function #[derive(Clone, Debug)] pub struct Clock { now: fn() -> Ticks, @@ -19,7 +193,7 @@ pub struct Clock { name: &'static str, } -/// A measurement of a monotonically nondecreasing clock. +/// A measurement of a monotonically nondecreasing [`Clock`]. /// Opaque and useful only with [`Duration`]. /// /// Provided that the [`Clock`] implementation is correct, `Instant`s are always @@ -34,50 +208,18 @@ pub struct Clock { /// backwards. /// As part of this non-guarantee it is also not specified whether system suspends count as /// elapsed time or not. The behavior varies across platforms and rust versions. -/// -/// Instants are opaque types that can only be compared to one another. There is -/// no method to get "the number of seconds" from an instant. Instead, it only -/// allows measuring the duration between two instants (or comparing two -/// instants). #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)] pub struct Instant(Duration); -/// Timer ticks are always counted by a 64-bit unsigned integer. +/// [`Clock`] ticks are always counted by a 64-bit unsigned integer. pub type Ticks = u64; impl Clock { /// Returns a new [`Clock`] with the provided tick [`Duration`] and `now()` /// function. /// - /// # Implementing `now()` - /// - /// The `now` function provided when constructing a [`Clock`] is a function - /// that returns the current hardware timestamp in a 64-bit number of - /// _ticks_. The period of time represented by a tick is indicated by the - /// `tick_duration` argument. - /// - /// Implementations of `now()` MUST ensure that timestamps returned by - /// `now()` MUST be [monontonically non-decreasing]. This means that a call - /// to `now()` MUST NOT ever return a value less than the value returned by - /// a previous call to`now()`. - /// - /// Note that this means that timestamps returned by `now()` are expected - /// not to overflow. Of course, all integers *will* overflow eventually, so - /// this requirement can reasonably be weakened to expecting that timestamps - /// returned by `now()` will not overflow unless the system has been running - /// for a duration substantially longer than the system is expected to run - /// for. For example, if a system is expected to run for as long as a year - /// without being restarted, it's not unreasonable for timestamps returned - /// by `now()` to overflow after, say, 100 years. Ideally, a general-purpose - /// `Clock` implementation would not overflow for, say, 1,000 years. - /// - /// The implication of this is that if the timestamp counters provided by - /// the hardware platform are less than 64 bits wide (e.g., 16- or 32-bit - /// timestamps), the `Clock` implementation is responsible for ensuring that - /// they are extended to 64 bits, such as by counting overflows in the - /// `Clock` implementation. - /// - /// [monotonic]: https://en.wikipedia.org/wiki/Monotonic_function + /// See the [type-level documentation for `Clock`](Self#implementing-now) + /// for details on implementing the `now()` function. #[must_use] pub const fn new(tick_duration: Duration, now: fn() -> Ticks) -> Self { Self { @@ -112,7 +254,7 @@ impl Clock { /// Returns an [`Instant`] representing the current timestamp according to /// this [`Clock`]. #[must_use] - pub fn now(&self) -> Instant { + pub(crate) fn now(&self) -> Instant { let now = self.now_ticks(); let tick_duration = self.tick_duration(); Instant(ticks_to_dur(tick_duration, now)) @@ -124,7 +266,7 @@ impl Clock { max_duration(self.tick_duration()) } - /// Returns this `Clock`'s name, if it was given one using the [`named`] + /// Returns this `Clock`'s name, if it was given one using the [`Clock::named`] /// method. #[must_use] pub fn name(&self) -> &'static str { diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 1ba650d8..224e2444 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -61,9 +61,24 @@ use self::sleep::Sleep; /// upon by an outside force!* /// /// Since `maitake` is intended for bare-metal platforms without an operating -/// system, a `Timer` instance cannot automatically advance time. Instead, it -/// must be driven by a *time source*, which calls the [`Timer::advance`] method -/// and/or the [`Timer::pend_duration`] and [`Timer::force_advance`] methods. +/// system, a `Timer` instance cannot automatically advance time. Instead, the +/// operating system's run loop must periodically call the [`Timer::turn`] +/// method, which advances the timer wheel to the current time and wakes any +/// futures whose timers have completed. +/// +/// The [`Timer::turn`] method may be called on a periodic interrupt, or on +/// every iteration of a system run loop. If the system is also using the +/// [`maitake::scheduler`](crate::scheduler) module, calling [`Timer::turn`] +/// after a call to [`Scheduler::tick`] is generally appropriate. +/// +/// # Clocks +/// +/// In addition to driving the timer, user code must also provide a [`Clock`] +/// when constructing a [`Timer`]. The [`Clock`] is a representation of a +/// hardware time source used to determine the current timestamp. See the +/// [`Clock`] type's documentation for details on implementing clocks. +/// +/// ## Hardware Time Sources /// /// Depending on the hardware platform, a time source may be a timer interrupt /// that fires on a known interval[^1], or a timestamp that's read by reading @@ -71,36 +86,12 @@ use self::sleep::Sleep; /// special instruction[^3]. A combination of multiple time sources can also be /// used. /// -/// In any case, the timer must be advanced periodically by the time source. -/// /// [^1]: Such as the [8253 PIT interrupt] on most x86 systems. /// /// [^2]: Such as the [`CCNT` register] on ARMv7. /// /// [^3]: Such as the [`rdtsc` instruction] on x86_64. /// -/// ### Interrupt-Driven Timers -/// -/// When the timer is interrupt-driven, the interrupt handler for the timer -/// interrupt should call either the [`Timer::pend_duration`] or -/// [`Timer::advance`] methods. -/// -/// [`Timer::advance`] will attempt to optimistically acquire a spinlock, and -/// advance the timer if it is acquired, or add to the pending tick counter if -/// the timer wheel is currently locked. Therefore, it is safe to call in an -/// interrupt handler, as it and cannot cause a deadlock. -/// -/// However, if interrupt handlers must be extremely short, the -/// [`Timer::pend_duration`] method can be used, instead. This method will -/// *never* acquire a lock, and does not actually turn the timer wheel. Instead, -/// it always performs only a single atomic add. If the time source is an -/// interrupt handler which calls [`Timer::pend_duration`], though, the timer -/// wheel must be turned externally. This can be done by calling the -/// [`Timer::force_advance_ticks`] method periodically outside of the interrupt -/// handler, with a duration of 0 ticks. In general, this should be done as some -/// form of runtime bookkeeping action. For example, the timer can be advanced -/// in a system's run loop every time the [`Scheduler::tick`] method completes. -/// /// ### Periodic and One-Shot Timer Interrupts /// /// Generally, hardware timer interrupts operate in one of two modes: _periodic_ @@ -111,9 +102,8 @@ use self::sleep::Sleep; /// /// Using a periodic timer with the `maitake` timer wheel is quite simple: /// construct the timer wheel with the minimum [granularity](#timer-granularity) -/// set to the period of the timer interrupt, and call -/// [`Timer::advance_ticks`]`(1)` or [`Timer::pend_ticks`]`(1)` in the interrupt -/// handler, as discused [above](#interrupt-driven-timers). +/// set to the period of the timer interrupt, and call [`Timer::try_turn`] every +/// time the periodic interrupt occurs. /// /// However, if the hardware platform provides a way to put the processor in a /// low-power state while waiting for an interrupt, it may be desirable to @@ -131,47 +121,32 @@ use self::sleep::Sleep; /// ### Timestamp-Driven Timers /// /// When the timer is advanced by reading from a time source, the -/// [`Timer::advance`] method should generally be used to drive the timer. Prior -/// to calling [`Timer::advance`], the time source is read to determine the -/// duration that has elapsed since the last time [`Timer::advance`] was called, -/// and that duration is provided when calling `advance`. -/// -/// This should occur periodically as part of a runtime loop (as discussed in -/// the previous section), such as every time [the scheduler is -/// ticked][`Scheduler::tick`]. Advancing the timer more frequently will result -/// in [`Sleep`] futures firing with a higher resolution, while less frequent -/// calls to [`Timer::advance`] will result in more noise in when [`Sleep`] +/// [`Timer::turn`] method should be called periodically as part of a runtime +/// loop (as discussed in he previous section), such as every time [the +/// scheduler is ticked][`Scheduler::tick`]. Advancing the timer more frequently +/// will result in [`Sleep`] futures firing with a higher resolution, while less +/// frequent calls to [`Timer::turn`] will result in more noise in when [`Sleep`] /// futures actually complete. /// -/// # Timer Granularity +/// ## Timer Granularity /// /// Within the timer wheel, the duration of a [`Sleep`] future is represented as /// a number of abstract "timer ticks". The actual duration in real life time /// that's represented by a number of ticks depends on the timer's _granularity. /// -/// When constructing a `Timer` (e.g. by calling [`Timer::new`]), the minimum -/// granularity of the timer is selected by providing the [`Duration`] -/// represented by a single timer tick. The selected tick duration influences -/// both the resolution of the timer (i.e. the minimum difference in duration -/// between two `Sleep` futures that can be distinguished by the timer), and -/// the maximum duration that can be represented by a `Sleep` future (which is -/// limited by the size of a 64-bit integer). +/// When constructing `Timer` (e.g. by calling [`Timer::new`]), the minimum +/// granularity of the timer is determined by the [`Clock`]'s `tick_duration` +/// value. The selected tick duration influences both the resolution of the +/// timer (i.e. the minimum difference in duration between two `Sleep` futures +/// that can be distinguished by the timer), and the maximum duration that can +/// be represented by a `Sleep` future (which is limited by the size of a 64-bit +/// integer). /// /// A longer tick duration will allow represented longer sleeps, as the maximum /// allowable sleep is the timer's granularity multiplied by [`u64::MAX`]. A /// shorter tick duration will allow for more precise sleeps at the expense of /// reducing the maximum allowed sleep. /// -/// When using an [interrupt-driven time source](#interrupt-driven-timers), the -/// tick duration should generally be the interval that the timer interrupt -/// fires at. A finer resolution won't have any benefit, as the timer only fires -/// at that frequency, and all sleeps that complete between two timer interrupts -/// will be woken at the same time anyway. -/// -/// When using a [timestamp-driven time source](#timestamp-driven-timers), -/// selecting the resolution of the timestamp counter as the timer's tick -/// duration is probably a good choice. -/// /// [`Sleep`]: crate::time::Sleep /// [`Timeout`]: crate::time::Timeout /// [future]: core::future::Future @@ -381,41 +356,6 @@ impl Timer { Sleep::new(self, ticks) } - /// Add pending time to the timer *without* turning the wheel. - /// - /// This function will *never* acquire a lock, and will *never* notify any - /// waiting [`Sleep`] futures. It can be called in an interrupt handler that - /// cannot perform significant amounts of work. - /// - /// However, if this method is used, then [`Timer::force_advance`] must be - /// called frequently from outside of the interrupt handler. - #[inline(always)] - pub fn pend_duration(&self, duration: Duration) { - let ticks = expect_display( - self.dur_to_ticks(duration), - "cannot add to pending duration", - ); - self.pend_ticks(ticks) - } - - /// Add pending ticks to the timer *without* turning the wheel. - /// - /// This function will *never* acquire a lock, and will *never* notify any - /// waiting [`Sleep`] futures. It can be called in an interrupt handler that - /// cannot perform significant amounts of work. - /// - /// However, if this method is used, then [`Timer::force_advance`] must be - /// called frequently from outside of the interrupt handler. - #[inline(always)] - #[track_caller] - pub fn pend_ticks(&self, ticks: Ticks) { - debug_assert!( - ticks < usize::MAX as u64, - "cannot pend more than `usize::MAX` ticks at once!" - ); - self.pending_ticks.fetch_add(ticks as usize, Release); - } - /// Attempt to turn the timer to the current `now` if the timer is not /// locked, potentially waking any [`Sleep`] futures that have completed. /// @@ -438,20 +378,18 @@ impl Timer { /// immediately. Therefore, it is safe to call this method in an interrupt /// handler, as it will never acquire a lock that may already be locked. /// - /// The [`force_advance_ticks`] method will spin to lock the timer wheel lock if + /// The [`Timer::turn`] method will spin to lock the timer wheel lock if /// it is currently held, *ensuring* that any pending wakeups are processed. /// That method should never be called in an interrupt handler. /// - /// If a timer is driven primarily by calling `advance` in an interrupt - /// handler, it may be desirable to occasionally call [`force_advance_ticks`] + /// If a timer is driven primarily by calling `try_turn` in an interrupt + /// handler, it may be desirable to occasionally call [`Timer::turn`] /// *outside* of an interrupt handler (i.e., as as part of an occasional /// runtime bookkeeping process). This ensures that any pending ticks are /// observed by the timer in a relatively timely manner. - /// - /// [`force_advance_ticks`]: Timer::force_advance_ticks #[inline] pub fn try_turn(&self) -> Option { - // `advance` may be called in an ISR, so it can never actually spin. + // `try_turn` may be called in an ISR, so it can never actually spin. // instead, if the timer wheel is busy (e.g. the timer ISR was called on // another core, or if a `Sleep` future is currently canceling itself), // we just add to a counter of pending ticks, and bail. @@ -481,13 +419,11 @@ impl Timer { /// held elsewhere. Therefore, this method must *NEVER* be called in an /// interrupt handler! /// - /// If a timer is advanced inside an interrupt handler, use the [`advance`] - /// method instead. If a timer is advanced primarily by calls to - /// [`advance`], it may be desirable to occasionally call `force_advance` - /// outside an interrupt handler, to ensure that pending ticks are drained - /// frequently. - /// - /// [`advance`]: Timer::advance + /// If a timer is advanced inside an interrupt handler, use the + /// [`Timer::try_turn`] method instead. If a timer is advanced primarily by + /// calls to [`Timer::try_turn`] in an interrupt handler, it may be + /// desirable to occasionally call `turn` outside an interrupt handler, to + /// ensure that pending ticks are drained frequently. #[inline] pub fn turn(&self) -> Turn { self.advance_locked(&mut self.core.lock())