Message-ID: <87r0pax9a6.ffs@tglx>
Date:   Sat, 15 Jul 2023 03:17:53 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Asahi Lina <lina@...hilina.net>, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        John Stultz <jstultz@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Josh Stone <jistone@...hat.com>,
        Gaelan Steele <gbs@...ishe.com>,
        Heghedus Razvan <heghedus.razvan@...tonmail.com>
Cc:     linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
        asahi@...ts.linux.dev, Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH v2] rust: time: New module for timekeeping functions

Lina!

On Fri, Jul 14 2023 at 16:55, Asahi Lina wrote:
> +ktime_t rust_helper_ktime_get_real(void) {
> +	return ktime_get_real();
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_ktime_get_real);

Colour me confused. But why does this need another export?

This just creates yet another layer of duct tape. If it's unsafe from
the rust perspective then wrapping it into rust_"unsafe_function" does not
make it any better.

Why on earth can't you use the actual C interfaces diretly which are
already exported?

> +use crate::{bindings, pr_err};
> +use core::marker::PhantomData;
> +use core::time::Duration;
> +
> +/// Represents a clock, that is, a unique time source.
> +pub trait Clock: Sized {}
> +
> +/// A time source that can be queried for the current time.

I doubt you can read anything else than current time from a time
source. At least the C side does not provide time traveling interfaces
unless the underlying hardware goes south.

> +pub trait Now: Clock {
> +impl<T: Clock> Instant<T> {
> +    fn new(nanoseconds: i64) -> Self {
> +        Instant {
> +            nanoseconds,
> +            _type: PhantomData,
> +        }
> +    }
> +
> +    /// Returns the time elapsed since an earlier Instant<t>, or
> +    /// None if the argument is a later Instant.
> +    pub fn since(&self, earlier: Instant<T>) -> Option<Duration> {
> +        if earlier.nanoseconds > self.nanoseconds {
> +            None
> +        } else {
> +            // Casting to u64 and subtracting is guaranteed to give the right
> +            // result for all inputs, as long as the condition we checked above
> +            // holds.
> +            Some(Duration::from_nanos(
> +                self.nanoseconds as u64 - earlier.nanoseconds as u64,

Clever, but any timestamp greater than KTIME_MAX or less than 0 for such
a comparison is invalid. I'm too lazy to do the math for you..

> +            ))
> +        }
> +    }
> +}
> +
> +impl<T: Clock + Now + Monotonic> Instant<T> {
> +    /// Returns the time elapsed since this Instant<T>.
> +    ///
> +    /// This is guaranteed to return a positive result, since
> +    /// it is only implemented for monotonic clocks.
> +    pub fn elapsed(&self) -> Duration {
> +        T::now().since(*self).unwrap_or_else(|| {
> +            pr_err!(
> +                "Monotonic clock {} went backwards!",
> +                core::any::type_name::<T>()

Can you please write this in one line?

 +            pr_err!("Monotonic clock {} went backwards!", core::any::type_name::<T>()

The above is unreadable gunk for no reason.

> +/// Contains the various clock source types available to the kernel.
> +pub mod clock {
> +    use super::*;
> +
> +    /// A clock representing the default kernel time source.
> +    ///
> +    /// This is `CLOCK_MONOTONIC` (though it is not the only
> +    /// monotonic clock) and also the default clock used by
> +    /// `ktime_get()` in the C API.

This "(though it is not the only monotonic clock)" phrase is irritating
at best. CLOCK_MONOTONIC is well defined as the other CLOCK_* variants.

> +    ///
> +    /// This is like `BootTime`, but does not include time
> +    /// spent sleeping.
> +
> +    pub struct KernelTime;
> +
> +    impl Clock for KernelTime {}
> +    impl Monotonic for KernelTime {}
> +    impl Now for KernelTime {
> +        fn now() -> Instant<Self> {
> +            Instant::<Self>::new(unsafe { bindings::ktime_get() })
> +        }
> +    }
> +
> +    /// A clock representing the time elapsed since boot.
> +    ///
> +    /// This is `CLOCK_MONOTONIC` (though it is not the only
> +    /// monotonic clock) and also the default clock used by
> +    /// `ktime_get()` in the C API.

The wonders of copy and pasta...

> +    ///
> +    /// This is like `KernelTime`, but does include time
> +    /// spent sleeping.

Can you please expand your editors line wrap limit to the year 2023
standards? This looks like a IBM 2260 terminal.

> +    /// A clock representing TAI time.
> +    ///
> +    /// This clock is not monotonic and can be changed from userspace.
> +    /// However, it is not affected by leap seconds.

I'm not impressed by this at all.

Lots of copy and pasta with zero content. I don't see how this is an
improvement over the admittedly lousy or non-existant kernel interface
documentations.

I thought Rust is set out to be better, but obviously it's just another
variant of copy & pasta and sloppy wrappers with useless documentation
around some admittedly not well documented, but well understood C
interfaces.

So the right approach to this is:

 1) Extend the kernel C-API documentations first if required

 2) Build your wrapper so that it can refer to the documentation which
    was either there already or got refined/added in #1

 3) Add one clock per patch with a proper changelog and not some
    wholesale drop it all.

It's well documented in Documentation/process/*, no?

Thanks,

        tglx

Powered by blists - more mailing lists