Message-ID: <ec4977d7-7df3-44f4-80c7-3ecdfc6266d1@proton.me>
Date: Fri, 21 Feb 2025 10:22:26 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Danilo Krummrich <dakr@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Lyude Paul <lyude@...hat.com>, Guangbo Cui <2407018371@...com>, Dirk Behme <dirk.behme@...il.com>, Daniel Almeida <daniel.almeida@...labora.com>, Tamir Duberstein <tamird@...il.com>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 13/14] rust: hrtimer: add clocksource selection through `ClockSource`

On 21.02.25 10:29, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@...ton.me> writes:
> 
>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>> Allow selecting a clock source for timers by passing a `ClockSource`
>>> variant to `HrTimer::new`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>>> ---
>>>  rust/kernel/time/hrtimer.rs | 52 +++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>> index db49061f830c3..2b46d66eaa313 100644
>>> --- a/rust/kernel/time/hrtimer.rs
>>> +++ b/rust/kernel/time/hrtimer.rs
>>> @@ -73,7 +73,7 @@ unsafe impl<T> Sync for HrTimer<T> {}
>>>
>>>  impl<T> HrTimer<T> {
>>>      /// Return an initializer for a new timer instance.
>>> -    pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
>>> +    pub fn new(mode: HrTimerMode, clock: ClockSource) -> impl PinInit<Self>
>>>      where
>>>          T: HrTimerCallback,
>>>      {
>>> @@ -87,7 +87,7 @@ pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
>>>                      bindings::hrtimer_setup(
>>>                          place,
>>>                          Some(T::CallbackTarget::run),
>>> -                        bindings::CLOCK_MONOTONIC as i32,
>>> +                        clock.into(),
>>>                          mode.into(),
>>>                      );
>>>                  }
>>> @@ -448,6 +448,54 @@ fn from(value: HrTimerMode) -> Self {
>>>      }
>>>  }
>>>
>>> +/// The clock source to use for a [`HrTimer`].
>>> +pub enum ClockSource {
>>> +    /// A settable system-wide clock that measures real (i.e., wall-clock) time.
>>
>> Missing newline here to separate the short one-line description and the
>> rest of the docs. (also below)
> 
> Right. It is copy pasta from the C code, but I guess we can add some newlines.
> 
>>
>>> +    /// Setting this clock requires appropriate privileges. This clock is
>>> +    /// affected by discontinuous jumps in the system time (e.g., if the system
>>> +    /// administrator manually changes the clock), and by frequency adjustments
>>> +    /// performed by NTP and similar applications via adjtime(3), adjtimex(2),
>>> +    /// clock_adjtime(2), and ntp_adjtime(3). This clock normally counts the
>>> +    /// number of seconds since 1970-01-01 00:00:00 Coordinated Universal Time
>>> +    /// (UTC) except that it ignores leap seconds; near a leap second it is
>>> +    /// typically adjusted by NTP to stay roughly in sync with UTC.
>>
>> Thanks for the extensive descriptions of the various clock sources!
> 
> Just FYI, I pulled these from other documentation sources, I didn't
> author all of this.

I still find it useful for you to copy these, I've seen a patch in the
past where the C documentation gave more information than the Rust
abstraction.

>>> +    RealTime,
>>> +    /// A nonsettable system-wide clock that represents monotonic time since—as
>>> +    /// described by POSIX—"some unspecified point in the past" On Linux, that
>>> +    /// point corresponds to the number of seconds that the system has been
>>> +    /// running since it was booted.
>>> +    ///
>>> +    /// The CLOCK_MONOTONIC clock is not affected by discontinuous jumps in the
>>> +    /// system time (e.g., if the system administrator manually changes the
>>> +    /// clock), but is affected by frequency adjustments. This clock does not
>>> +    /// count time that the system is suspended.
>>> +    Monotonic,
>>> +    /// A nonsettable system-wide clock that is identical to CLOCK_MONOTONIC,
>>> +    /// except that it also includes any time that the system is suspended. This
>>> +    /// allows applications to get a suspend-aware monotonic clock without
>>> +    /// having to deal with the complications of CLOCK_REALTIME, which may have
>>> +    /// discontinuities if the time is changed using settimeofday(2) or similar.
>>> +    BootTime,
>>> +    /// A nonsettable system-wide clock derived from wall-clock time but
>>> +    /// counting leap seconds. This clock does not experience discontinuities or
>>> +    /// frequency adjustments caused by inserting leap seconds as CLOCK_REALTIME
>>> +    /// does.
>>> +    ///
>>> +    /// The acronym TAI refers to International Atomic Time.
>>
>> In that case, I would expect the acronym to be `IAT`.
> 
> Yea, this one is funny. The abbreviation apparently is French [1] and
> TAI is the correct and agreed upon (beyond Linux) abbreviation to use.

What a world we have... :)

---
Cheers,
Benno

> [1] https://en.wikipedia.org/wiki/International_Atomic_Time
> 
>>
>>> +    TAI,
>>> +}
>>> +
>>> +impl From<ClockSource> for bindings::clockid_t {
>>> +    fn from(value: ClockSource) -> Self {
>>> +        match value {
>>> +            ClockSource::RealTime => bindings::CLOCK_REALTIME as i32,
>>> +            ClockSource::Monotonic => bindings::CLOCK_MONOTONIC as i32,
>>> +            ClockSource::BootTime => bindings::CLOCK_BOOTTIME as i32,
>>> +            ClockSource::TAI => bindings::CLOCK_TAI as i32,
>>> +        }
>>> +    }
>>> +}
>>
>> Same question here as for the `HrTimerMode`, do drivers need this impl?
>> If not, then I think a private conversion function is a better fit.
> 
> I will hide it.
> 
> 
> Best regards,
> Andreas Hindborg
> 
> 

Powered by blists - more mailing lists