Re: [PATCH v9 01/13] rust: hrtimer: introduce hrtimer support

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Feb 24 2025 at 13:03, Andreas Hindborg wrote:
> This patch adds support for intrusive use of the hrtimer system. For
> now,

git grep 'This patch' Documentation/process/

> only one timer can be embedded in a Rust struct.
>
> +//! ## State Diagram
> +//!
> +//! ```text
> +//!                                                   Return NoRestart
> +//!                       +---------------------------------------------------------------------+
> +//!                       |                                                                     |
> +//!                       |                                                                     |
> +//!                       |                                                                     |
> +//!                       |                                         Return Restart              |
> +//!                       |                                      +------------------------+     |
> +//!                       |                                      |                        |     |
> +//!                       |                                      |                        |     |
> +//!                       v                                      v                        |     |
> +//!           +-----------------+      Start      +------------------+           +--------+-----+--+
> +//!           |                 +---------------->|                  |           |                 |
> +//! Init      |                 |                 |                  |  Expire   |                 |
> +//! --------->|    Stopped      |                 |      Started     +---------->|     Running     |
> +//!           |                 |     Cancel      |                  |           |                 |
> +//!           |                 |<----------------+                  |           |                 |
> +//!           +-----------------+                 +---------------+--+           +-----------------+
> +//!                                                     ^         |
> +//!                                                     |         |
> +//!                                                     +---------+
> +//!                                                      Restart
> +//! ```
> +//!
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** by the `start` operation, with an **expiry** time. After the
> +//! `start` operation, the timer is in the **started** state. When the timer
> +//! **expires**, the timer enters the **running** state and the handler is
> +//! executed. After the handler has finished executing, the timer may enter the
> +//! **started* or **stopped** state, depending on the return value of the
> +//! handler. A running timer can be **canceled** by the `cancel` operation. A
> +//! timer that is cancelled enters the **stopped** state.
> +//!
> +//! A `cancel` or `restart` operation on a timer in the **running** state takes
> +//! effect after the handler has finished executing and the timer has transitioned
> +//! out of the **running** state.
> +//!
> +//! A `restart` operation on a timer in the **stopped** state is equivalent to a
> +//! `start` operation.

Nice explanation!

> +    /// Cancel an initialized and potentially running timer.
> +    ///
> +    /// If the timer handler is running, this will block until the handler is
> +    /// finished.
> +    ///
> +    /// Users of the `HrTimer` API would not usually call this method directly.
> +    /// Instead they would use the safe [`HrTimerHandle::cancel`] on the handle
> +    /// returned when the timer was started.
> +    ///
> +    /// This function does not create any references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    #[allow(dead_code)]
> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
> +
> +        // If the handler is running, this will wait for the handler to finish
> +        // before returning.
> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on C side.

You might want to be more explicit about the provided synchronization.
The hrtimer core only guarantees that operations on the hrtimer object
are strictly serialized. But it does not provide any guarantee about
external concurrency. The following case cannot be handled by the core:

         T0                         T1
         cancel()                   start()
           lock()
           ....                     lock() <- contended
           dequeue()
           unlock()
                                    enqueue()
                                    unlock()

So there is no guarantee for T0 that the timer is actually canceled in
this case. The hrtimer core can do nothing about this, that's a problem
of the call sites.

We've implemented timer_shutdown() for the timer wheel timers, which
prevents that the timer can be started after shutdown() succeeds. It
might be a good thing to implement this for hrtimers as well.

Thanks,

        tglx