Message-ID: <87h64nfxxk.fsf@kernel.org>
Date: Fri, 21 Feb 2025 10:29:27 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
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`

"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.

>
>> +    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.

[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