Message-ID: <87zfi7prf2.fsf@kernel.org>
Date: Thu, 27 Feb 2025 12:18:09 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Thomas Gleixner" <tglx@...utronix.de>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Anna-Maria Behnsen"
 <anna-maria@...utronix.de>,  "Frederic Weisbecker" <frederic@...nel.org>,
  "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>,  "Benno
 Lossin" <benno.lossin@...ton.me>,  "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 v9 12/13] rust: hrtimer: add clocksource selection
 through `ClockSource`

"Thomas Gleixner" <tglx@...utronix.de> writes:

> On Mon, Feb 24 2025 at 13:03, Andreas Hindborg wrote:
>>
>> +/// The clock source to use for a [`HrTimer`].
>> +pub enum ClockSource {
>
> ClockSource is a confusing name as 'clocksource' is used in the kernel
> already for devices providing counters, which can be used for
> timekeeping.
>
> Also these clocks are not really hrtimer specific. These CLOCK ids are
> system wide valid and are used for other purposes obviously internally
> in timekeeping. hrtimers are built on top of timekeeping, which provides
> the underlying time.

I see. How about renaming to `ClockId` and moving the type one level up
to `kernel::time`?

>
>> +    /// A settable system-wide clock that measures real (i.e., wall-clock) time.
>> +    ///
>> +    /// 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.
>
> That's not correct. It depends on the implementation/configuration of
> NTP. The default is that the leap second is actually applied at the
> requested time, by setting the clock one second forth or back.
>
> Though there are NTP configurations/implementations out there which use
> leap second "smearing" to avoid the jump. They adjust the conversion
> factors around the leap second event by slowing down or speeding up for
> a while. That avoids a few common issues, e.g. in data bases.
>
> But it brings all clocks out of sync with the actual progress of time, which
> is patently bad for systems which require strict synchronization.
>
> The problem is that the kernel uses the NTP/PTP frequency adjustment to
> steer the conversion of all clocks, except CLOCK_MONOTONIC_RAW. The
> kernel internal base clock is CLOCK_MONOTONIC. The other clocks are
> derived from that:
>
>         CLOCK_[X] = CLOCK_MONOTONIC + offset[X]

I see. I lifted the text from `clock_getres(2)` in linux-man [1]. We
might consider updating that source with the info we collect here.

How about changing the text like so:

.. by frequency adjustments performed by NTP ...

to

.. by frequency adjustments performed by some implementations of NTP ...

?

>
>> +    RealTime,
>> +    /// A monotonically increasing clock.
>> +    ///
>> +    /// 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
>
> s/system time/CLOCK_REALTIME/

OK.

>
>> +    /// clock), but is affected by frequency adjustments. This clock does not
>> +    /// count time that the system is suspended.
>> +    Monotonic,
>> +    /// A monotonic that ticks while system is suspended.
>> +    ///
>> +    /// 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,
>> +    /// International Atomic Time.
>> +    ///
>> +    /// 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.
>
> Only partially correct.
>
> CLOCK_TAI can be set as CLOCK_TAI is obviously coupled to CLOCK_REALTIME
> and vice versa.

So it cannot be set directly, but if CLOCK_REALTIME is set, CLOCK_TAI
will update?

In that case I would add the following paragraph:

  This clock is coupled to CLOCK_REALTIME and will be set when
  CLOCK_REALTIME is set.

> Also if the NTP implementation does leap seconds smearing then the
> adjustment affects CLOCK_TAI as well. See above. That's compensated for
> by adjusting the TAI offset to be in sync with reality, but during the
> smear phase the readout is not precise.

I would add the following paragraph then:

  However, if NTP adjusts CLOCK_REALTIME by leap second smearing, this
  clock will not be precise during leap second smearing.



Best regards,
Andreas Hindborg


[1] https://web.git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man/man2/clock_getres.2?h=man-pages-6.12

Powered by blists - more mailing lists