Re: [PATCH 3/5] rust: time: Introduce clock reading framework

[Benno Lossin]

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@xxxxxxxxxxxxxx>
>>> Signed-off-by: Heghedus Razvan <heghedus.razvan@xxxxxxxxxxxxxx>
>>> Co-developed-by: Asahi Lina <lina@xxxxxxxxxxxxx>
>>> Signed-off-by: Asahi Lina <lina@xxxxxxxxxxxxx>
>>> Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
>>> ---
>>>   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`.

-- 
Cheers,
Benno