lkml.org 
[lkml]   [2024]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 3/5] rust: time: Introduce clock reading framework
    On Wed, Mar 27, 2024 at 12:50:50PM +0000, Benno Lossin wrote:
    > On 26.03.24 20:19, Boqun Feng wrote:
    > > On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
    > >> On 24.03.24 23:33, Boqun Feng wrote:
    > >>> To make sure Rust code doesn't mix timestamps from different clocks, a
    > >>> type safe clock reading framework is introduced. It includes:
    > >>>
    > >>> * A `Clock` trait that represents different clocks, to read a particular
    > >>> clock, one needs implement the `Clock::now()` function.
    > >>>
    > >>> * A `Instant<Clock>` type that represents timestamps of different
    > >>> clocks, whose implementation is just a `ktime_t`, so all the
    > >>> calculation on `ktime_t` should apply to it as well.
    > >>>
    > >>> Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
    > >>> Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
    > >>> Co-developed-by: Asahi Lina <lina@asahilina.net>
    > >>> Signed-off-by: Asahi Lina <lina@asahilina.net>
    > >>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
    > >>> ---
    > >>> rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
    > >>> 1 file changed, 49 insertions(+)
    > >>>
    > >>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
    > >>> index b238b3a4e899..0f9f5605ed48 100644
    > >>> --- a/rust/kernel/time.rs
    > >>> +++ b/rust/kernel/time.rs
    > >>> @@ -6,6 +6,9 @@
    > >>> //! have been ported or wrapped for usage by Rust code in the kernel.
    > >>> //!
    > >>> //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
    > >>> +//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
    > >>> +
    > >>> +use core::marker::PhantomData;
    > >>>
    > >>> /// The number of nanoseconds per millisecond.
    > >>> pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
    > >>> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
    > >>> self.inner
    > >>> }
    > >>> }
    > >>> +
    > >>> +/// Represents a clock, that is, a unique time source and it can be queried for the current time.
    > >>> +pub trait Clock: Sized {
    > >>> + /// Returns the current time for this clock.
    > >>> + fn now() -> Instant<Self>;
    > >>> +}
    > >>> +
    > >>> +/// Marker trait for clock sources that are guaranteed to be monotonic.
    > >>> +pub trait Monotonic {}
    > >>
    > >> Why is this trait not an extension of `Clock`?
    > >>
    > >
    > > This was carried over from the old version, for myself, it doesn't make
    > > much difference between:
    > >
    > > trait A { .. }
    > > trait B { .. }
    > >
    > > impl<T: A + B> ...
    > >
    > > vs
    > >
    > > trait A { .. }
    > > trait B: A { .. }
    > >
    > > impl<T: B> ...
    > >
    > > hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
    > > change it in the next version.
    >
    > I think it would also make sense to rename it to `MonotonicClock`.
    >
    > >
    > >>> +
    > >>> +/// A timestamp of a given [`Clock`]
    > >>
    > >> Missing '.'.
    > >>
    > >
    > > Fixed locally.
    > >
    > >>> +#[repr(transparent)]
    > >>> +#[derive(Debug)]
    > >>> +pub struct Instant<T: Clock> {
    > >>> + inner: bindings::ktime_t,
    > >>> + clock: PhantomData<T>,
    > >>> +}
    > >>> +
    > >>> +impl<T: Clock> Clone for Instant<T> {
    > >>> + fn clone(&self) -> Self {
    > >>> + *self
    > >>> + }
    > >>> +}
    > >>> +
    > >>> +impl<T: Clock> Copy for Instant<T> {}
    > >>> +
    > >>> +impl<T: Clock> Instant<T> {
    > >>> + fn new(ktime: bindings::ktime_t) -> Self {
    > >>
    > >> When compiling, this function is marked as dead-code in this patch.
    > >>
    > >
    > > Hmm... I cannot trigger any compile errors with this patch. If you see
    > > an error, could you share the build command? It's not a `pub` function
    > > though.
    >
    > I created a fresh config using `menuconfig` and enabled CONFIG_RUST.
    > Then I get this error:
    >
    > error: associated function `new` is never used
    > --> rust/kernel/time.rs:97:8
    > |
    > 96 | impl<T: Clock> Instant<T> {
    > | ------------------------- associated function in this implementation
    > 97 | fn new(ktime: bindings::ktime_t) -> Self {
    > | ^^^
    > |
    > = note: `-D dead-code` implied by `-D warnings`
    > = help: to override `-D warnings` add `#[allow(dead_code)]`
    >
    > error: aborting due to 1 previous error
    >
    > Note that the error vanishes in the next patch, since there you do have
    > a user for `new`.
    >

    Seems I didn't select CONFIG_WERROR=y, so I missed it as a warning.
    and I can hit it now with that config. Will fix it in the next version.
    Thanks!

    Regards

    > --
    > Cheers,
    > Benno
    \
     
     \ /
      Last update: 2024-05-27 16:12    [W:5.270 / U:0.388 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site