Re: [PATCH v3 2/2] rust: use Delta and a Jiffies newtype for timeouts and delays

On Mon Jul 20, 2026 at 10:47 AM BST, FUJITA Tomonori wrote:
>>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>>> index 23ef5c77383f..206296b6b6ea 100644
>>> --- a/rust/kernel/time.rs
>>> +++ b/rust/kernel/time.rs
>>> @@ -40,17 +40,40 @@
>>>  pub const NSEC_PER_SEC: i64 = bindings::NSEC_PER_SEC as i64;
>>>  
>>>  /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
>>> -pub type Jiffies = crate::ffi::c_ulong;
>>> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
>>> +pub struct Jiffies(crate::ffi::c_ulong);
>> 
>> Hmm, I feel that we are again making the same mistake that we had for `ktime_t`
>> abstraction, namely that we use the same type for instant and delta, albeit this
>> time the measure unit is jiffies and not nanoseconds. The reason that signed and
>> unsigned casts is needed in the above code basically is this.
>> 
>> Arguably, your earlier version don't have this issue, but then we are
>> introducing costly divisions implicitly in many places which is bad for
>> different reason.
>
> Agreed.
>
>> I wonder if we should have time types being generic over units. So you can have
>> `Delta<Nsec>` and `Delta<Jiffy>` and `Instant<Nsec>`, `Instant<Jiffy>`, with the
>> generic default being set to `Nsec`.
>> 
>>     Delta::new(42) // Delta<Nsec>
>>     Delta::new_jiffies(42) // Delta<Jiffy>
>> 
>> Thoughts?
>
> I think making Delta generic over the time unit makes sense; Delta
> <Nsec> and Delta<Jiffy>.
>
> However, I don't think making Instant generic over the time unit is a
> good idea, even though it clearly is for Delta.
>
> Instant is already generic over ClockSource, and jiffies is not a
> clock source: it has no clockid_t, it is read via get_jiffies_64()
> rather than ktime_get(), and it cannot be armed through hrtimer. That
> leaves two ways to force a jiffies Instant, both looks wrong:
>
> a) Add a second unit parameter, Instant<C, Unit>. But then the type
> admits meaningless combinations - there is no Instant<Monotonic,
> Jiffy> - and jiffies still has no ClockSource to put in the C slot, so
> (a) really collapses into (b).

We could split the `ClockSource` trait to be a generic `ClockSource` that
supports everything and a `HrClockSource` that provides ID.

>
> b) Make jiffies a fake ClockSource. But ClockSource::ID is passed
> straight to hrtimer_setup(), so a fabricated clockid_t would make
> HrTimer over jiffies type-check even though there is no corresponding
> C operation.
>
> This matches the C world: for deltas, jiffy and nsec interconvert
> (nsecs_to_jiffies() and friends), which is exactly what Delta<Unit>
> with conversions models. But the jiffies counter and ktime_get()
> values are never mixed - there isn't even an API to compare or convert
> between them - so there is no unit-generic notion of an instant to
> represent. A jiffies point in time, should be its own concrete type
> rather than a specialization of Instant.

Well, `Instant` never inter-converts with anything, so a `Instant<Jiffies>`
would be fine too. That said, we can delay making the change until we have a
user that needs to get jiffies count. So far it seems that people just use it as
a delta only.
>
>
>>> -/// The millisecond time unit.
>>> -pub type Msecs = crate::ffi::c_uint;
>>> +impl Jiffies {
>>> +    /// A jiffies value of zero.
>>> +    pub const ZERO: Self = Self(0);
>>>  
>>> -/// Converts milliseconds to jiffies.
>>> -#[inline]
>>> -pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
>>> -    // SAFETY: The `__msecs_to_jiffies` function is always safe to call no
>>> -    // matter what the argument is.
>>> -    unsafe { bindings::__msecs_to_jiffies(msecs) }
>>> +    /// Create a new [`Jiffies`] from the C side's `jiffies` value.
>>> +    #[inline]
>>> +    pub const fn new(jiffies: crate::ffi::c_ulong) -> Self {
>> 
>> Given that this is a public API not just for binding code, I think it's better
>> to use `usize`.
>
> Good point, I'll use usize.
>
>
>> This function can be `pub`.
>> 
>>> +    #[inline]
>>> +    pub(crate) const fn as_raw(self) -> crate::ffi::c_ulong {
>>> +        self.0
>>> +    }
>>> +
>
> I'd prefer to keep this pub(crate) until there's an in-tree user,
> following the usual practice of not exposing public API without a
> user. Also, if we do make it pub, it should return usize rather than
> ffi::c_ulong, to stay consistent with the constructor change above. Do
> you have a specific use case in mind that needs it public?

I think knowing how many jiffies are there in a `Delta<Jiffy>` is a reasonable
need while getting a raw pointer type from other abstractions are more
suspicious for drivers.

Best,
Gary