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

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.

>          let res = self.wait_internal(TASK_INTERRUPTIBLE, guard, jiffies);
>  
> -        match (res as Jiffies, crate::current!().signal_pending()) {
> -            (jiffies, true) => CondVarTimeoutResult::Signal { jiffies },
> +        match (res, crate::current!().signal_pending()) {
> +            (jiffies, true) => CondVarTimeoutResult::Signal {
> +                // CAST: `wait_internal()` never returns negative,
> +                // so it is safe to cast `jiffies` to `c_ulong`.
> +                jiffies: Jiffies::new(jiffies as c_ulong),
> +            },
>              (0, false) => CondVarTimeoutResult::Timeout,
> -            (jiffies, false) => CondVarTimeoutResult::Woken { jiffies },
> +            (jiffies, false) => CondVarTimeoutResult::Woken {
> +                // CAST: `wait_internal()` never returns negative,
> +                // so it is safe to cast `jiffies` to `c_ulong`.
> +                jiffies: Jiffies::new(jiffies as c_ulong),
> +            },
>          }
>      }
>  
> 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.

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?

Best,
Gary

>  
> -/// 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`.

> +        Self(jiffies)
> +    }
> +

This function can be `pub`.

> +    #[inline]
> +    pub(crate) const fn as_raw(self) -> crate::ffi::c_ulong {
> +        self.0
> +    }
> +
> +    /// Return `true` if the [`Jiffies`] has a value of zero.
> +    #[inline]
> +    pub fn is_zero(self) -> bool {
> +        self.0 == 0
> +    }
> +}
> +
> +impl From<Delta> for Jiffies {
> +    #[inline]
> +    fn from(delta: Delta) -> Self {
> +        let millis = u32::try_from(delta.as_millis_ceil().max(0)).unwrap_or(u32::MAX);
> +        // SAFETY: The `__msecs_to_jiffies` function is always safe to call no
> +        // matter what the argument is.
> +        let jiffies = unsafe { bindings::__msecs_to_jiffies(millis) };
> +        Self(jiffies)
> +    }
>  }
>  
>  /// Trait for clock sources.
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 7e253b6f299c..41fa1a9eb9d7 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -136,6 +136,7 @@
>  //! ```
>  //! use kernel::sync::Arc;
>  //! use kernel::workqueue::{self, impl_has_delayed_work, new_delayed_work, DelayedWork, WorkItem};
> +//! use kernel::time::Jiffies;
>  //!
>  //! #[pin_data]
>  //! struct MyStruct {
> @@ -171,7 +172,7 @@
>  //! /// This method will enqueue the struct for execution on the system workqueue, where its value
>  //! /// will be printed 12 jiffies later.
>  //! fn print_later(val: Arc<MyStruct>) {
> -//!     let _ = workqueue::system().enqueue_delayed(val, 12);
> +//!     let _ = workqueue::system().enqueue_delayed(val, Jiffies::new(12));
>  //! }
>  //!
>  //! /// It is also possible to use the ordinary `enqueue` method together with `DelayedWork`. This
> @@ -303,7 +304,11 @@ pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
>      /// This may fail if the work item is already enqueued in a workqueue.
>      ///
>      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> -    pub fn enqueue_delayed<W, const ID: u64>(&self, w: W, delay: Jiffies) -> W::EnqueueOutput
> +    pub fn enqueue_delayed<W, const ID: u64>(
> +        &self,
> +        w: W,
> +        delay: impl Into<Jiffies>,
> +    ) -> W::EnqueueOutput
>      where
>          W: RawDelayedWorkItem<ID> + Send + 'static,
>      {
> @@ -328,7 +333,7 @@ pub fn enqueue_delayed<W, const ID: u64>(&self, w: W, delay: Jiffies) -> W::Enqu
>                      bindings::wq_misc_consts_WORK_CPU_UNBOUND as ffi::c_int,
>                      queue_ptr,
>                      container_of!(work_ptr, bindings::delayed_work, work),
> -                    delay,
> +                    delay.into().as_raw(),
>                  )
>              })
>          }