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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Thomas Gleixner" <tglx@linutronix.de> writes:

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

I was made aware and have change the mood to imperative for next spin.

>
>> 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!

Thanks.

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

Right, this was also my understanding. I can add a note about this race.

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

Sounds like that would be useful.



Best regards,
Andreas Hindborg