Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 12:17:15 +0000
Tom Gundersen <tgunders@xxxxxxxxxx> wrote:

>> -/// A Rust wrapper around a `ktime_t`.
>> +/// A specific point in time.
>>  #[repr(transparent)]
>>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
>> -pub struct Ktime {
>> +pub struct Instant {
>> +    // Range from 0 to `KTIME_MAX`.
>>      inner: bindings::ktime_t,
>>  }
>>
>> -impl Ktime {
>> -    /// Create a `Ktime` from a raw `ktime_t`.
>> +impl Instant {
>> +    /// Create a `Instant` from a raw `ktime_t`.
>>      #[inline]
>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
>>
> 
> How do we know inner is between 0 and KTIME_MAX?

In my understanding, the kernel assumes that the range of the ktime_t
type is from 0 to KTIME_MAX. The ktime APIs guarantees to give a valid
ktime_t. The Rust abstraction creates ktime_t via ktime_get() so it's
fine now.

However, if we makes from_raw() public, a caller can create invalid
ktime_t by not using ktime APIs. Then from_raw() needs ceiling like
C's ktime_set(), I think.


>          Self { inner }
>>      }
>>
>>      /// Get the current time using `CLOCK_MONOTONIC`.
>>      #[inline]
>> -    pub fn ktime_get() -> Self {
>> +    pub fn now() -> Self {
>>          // SAFETY: It is always safe to call `ktime_get` outside of NMI
>> context.
>>
> 
> Similarly, should there be a comment here about the range being guaranteed
> to be correct?
> 
>          Self::from_raw(unsafe { bindings::ktime_get() })

We could add something like "The ktime API guarantees a valid
ktime_t". But adding similar comments to all the places where the
ktime API is called is redundant?

The comment on Instant struct must be improved instead, I think.