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

On Sat, 18 Jul 2026 16:20:10 +0100
"Gary Guo" <[email protected]> wrote:

> On Fri Jul 17, 2026 at 5:22 AM BST, FUJITA Tomonori wrote:
>> From: FUJITA Tomonori <[email protected]>
>>
>> Turn Jiffies into a distinct newtype and let these APIs take
>> impl Into<Jiffies>. A caller can now pass a Delta, the duration type
>> already used elsewhere, so the unit is part of the type rather than a
>> caller convention. A caller that already holds a jiffies count can
>> still pass Jiffies directly, avoiding a lossy jiffies -> Delta ->
>> jiffies round trip.
>>
>> Update CondVar::wait_interruptible_timeout() and
>> Queue::enqueue_delayed(), which took a raw jiffies count through a
>> bare c_ulong alias with no type safety. Callers had to know on their
>> own that the value meant jiffies and convert to and from it
>> themselves, which is easy to get wrong.
>>
>> Once callers express timeouts and delays as Delta, msecs_to_jiffies()
>> and the Msecs alias have no remaining users, so remove them.
>>
>> Signed-off-by: FUJITA Tomonori <[email protected]>
>> ---
>>  drivers/android/binder/process.rs |  7 +++---
>>  rust/kernel/sync/condvar.rs       | 21 +++++++++++-----
>>  rust/kernel/time.rs               | 41 ++++++++++++++++++++++++-------
>>  rust/kernel/workqueue.rs          | 11 ++++++---
>>  4 files changed, 59 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
>> index cdd1a9079726..5230ea492a75 100644
>> --- a/drivers/android/binder/process.rs
>> +++ b/drivers/android/binder/process.rs
>> @@ -1482,8 +1482,9 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
>>          inner.is_frozen = IsFrozen::InProgress;
>>  
>>          if info.timeout_ms > 0 {
>> -            let mut jiffies = kernel::time::msecs_to_jiffies(info.timeout_ms);
>> -            while jiffies > 0 {
>> +            let mut jiffies: kernel::time::Jiffies =
>> +                kernel::time::Delta::from_millis(info.timeout_ms.into()).into();
>> +            while !jiffies.is_zero() {
>>                  if inner.outstanding_txns == 0 {
>>                      break;
>>                  }
>> @@ -1500,7 +1501,7 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
>>                          jiffies = remaining;
>>                      }
>>                      CondVarTimeoutResult::Timeout => {
>> -                        jiffies = 0;
>> +                        jiffies = kernel::time::Jiffies::ZERO;
>>                      }
>>                  }
>>              }
>> diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
>> index 69d58dfbad7b..e215f83825e4 100644
>> --- a/rust/kernel/sync/condvar.rs
>> +++ b/rust/kernel/sync/condvar.rs
>> @@ -7,7 +7,7 @@
>>  
>>  use super::{lock::Backend, lock::Guard, LockClassKey};
>>  use crate::{
>> -    ffi::{c_int, c_long},
>> +    ffi::{c_int, c_long, c_ulong},
>>      str::{CStr, CStrExt as _},
>>      task::{
>>          MAX_SCHEDULE_TIMEOUT, TASK_FREEZABLE, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE,
>> @@ -186,15 +186,24 @@ pub fn wait_interruptible_freezable<T: ?Sized, B: Backend>(
>>      pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
>>          &self,
>>          guard: &mut Guard<'_, T, B>,
>> -        jiffies: Jiffies,
>> +        duration: impl Into<Jiffies>,
>>      ) -> CondVarTimeoutResult {
>> -        let jiffies = jiffies.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT);
>> +        let raw_jiffies: c_ulong = duration.into().as_raw();
>> +        let jiffies = c_long::try_from(raw_jiffies).unwrap_or(MAX_SCHEDULE_TIMEOUT);
> 
> This is existing problem, but this code pretty much assumes that
> `MAX_SCHEDULE_TIMEOUT == c_long::MAX`, which is true but would be bad code. So I
> would expect this to be just a compare, or would eventually become
> `saturating_cast_signed` (once it's stable) which explicitly relies on that
> assumption.

Agreed - the fallback should be c_long::MAX, not MAX_SCHEDULE_TIMEOUT;
relying on the two being equal is exactly the kind of assumption to
avoid.

>> 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).

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.


>> -/// 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?