Date: Mon, 29 Apr 2024 10:31:27 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Andreas Hindborg <nmi@...aspace.dk>
Cc: Benno Lossin <benno.lossin@...ton.me>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Anna-Maria Behnsen <anna-maria@...utronix.de>,
	Frederic Weisbecker <frederic@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Alice Ryhl <aliceryhl@...gle.com>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: hrtimer: introduce hrtimer support

On Fri, Apr 26, 2024 at 11:27:49AM +0200, Andreas Hindborg wrote:
> Benno Lossin <benno.lossin@...ton.me> writes:
> 
> > On 25.04.24 11:46, Andreas Hindborg wrote:
> >> From: Andreas Hindborg <a.hindborg@...sung.com>
> >> 
> >> This patch adds support for intrusive use of the hrtimer system. For now, only
> >> one timer can be embedded in a Rust struct.
> >> 
> >> The hrtimer Rust API is based on the intrusive style pattern introduced by the
> >> Rust workqueue API.
> >> 
> >> Signed-off-by: Andreas Hindborg <a.hindborg@...sung.com>
> >> 
> >> ---
> >> 
> >> This patch is a dependency for the Rust null block driver [1].
> >> 
> >> Link: https://lore.kernel.org/rust-for-linux/20240313110515.70088-1-nmi@metaspace.dk/T/#me0990150b9ba9f5b3d00293ec9a473c7bc3cc506 [1]
> >> 
> >>  rust/kernel/hrtimer.rs | 283 +++++++++++++++++++++++++++++++++++++++++
> >>  rust/kernel/lib.rs     |   1 +
> >>  2 files changed, 284 insertions(+)
> >>  create mode 100644 rust/kernel/hrtimer.rs
> >> 
> >> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
> >
> > Hmm is this the right place? I imagine there are other timers, does this
> > fit better into the `time` module (ie make `hrtimer` a submodule of
> > `time`) or should we later introduce a `timer` parent module?
> 
> We can always move it. We will move stuff anyway when the kernel crate
> is split.
> 
> We can also take it to `kernel::time::hrtimer` now, either way is fine.
> 

I think `kernel::time::hrtimer` makes more sense, since ideally
schedule() function should take a time delta type as the input instead
of `u64`. So hrtimer has some logical connection to timekeeping module.

> >
> >> new file mode 100644
> >> index 000000000000..1e282608e70c
> >> --- /dev/null
> >> +++ b/rust/kernel/hrtimer.rs
> >> @@ -0,0 +1,283 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Intrusive high resolution timers.
> >> +//!
> >> +//! Allows scheduling timer callbacks without doing allocations at the time of
> >> +//! scheduling. For now, only one timer per type is allowed.
> >> +//!
> >> +//! # Example
> >> +//!
> >> +//! ```rust
> >> +//! use kernel::{
> >> +//!     sync::Arc, hrtimer::{RawTimer, Timer, TimerCallback},
> >> +//!     impl_has_timer, prelude::*, stack_pin_init
> >> +//! };
> >> +//! use core::sync::atomic::AtomicBool;
> >> +//! use core::sync::atomic::Ordering;
> >> +//!
> >> +//! #[pin_data]
> >> +//! struct IntrusiveTimer {
> >> +//!     #[pin]
> >> +//!     timer: Timer<Self>,
> >> +//!     flag: AtomicBool,

Could you see if you can replace this with a `SpinLock<bool>` +
`CondVar`? We shouldn't use Rust atomic in kernel now. I know it's
unfortunate that LKMM atomics are still work in process, but in real
world, you won't do busy waiting for a timer to fire, so a
`CondVar::wait` is better for example purpose.

> >> +//! }
> >> +//!
> >> +//! impl IntrusiveTimer {
> >> +//!     fn new() -> impl PinInit<Self> {
> >> +//!         pin_init!(Self {
> >> +//!             timer <- Timer::new(),
> >> +//!             flag: AtomicBool::new(false),
> >> +//!         })
> >> +//!     }
> >> +//! }
> >> +//!
> >> +//! impl TimerCallback for IntrusiveTimer {
> >> +//!     type Receiver = Arc<IntrusiveTimer>;
> >> +//!
> >> +//!     fn run(this: Self::Receiver) {
> >> +//!         pr_info!("Timer called\n");
> >> +//!         this.flag.store(true, Ordering::Relaxed);
> >> +//!     }
> >> +//! }
> >> +//!
> >> +//! impl_has_timer! {
> >> +//!     impl HasTimer<Self> for IntrusiveTimer { self.timer }
> >> +//! }
> >> +//!
> >> +//! let has_timer = Arc::pin_init(IntrusiveTimer::new())?;
> >
> > I would not name this variable `has_timer`. Maybe `my_timer` is better?
> 
> Right, thanks.
> 
> >
> >> +//! has_timer.clone().schedule(200_000_000);
> >> +//! while !has_timer.flag.load(Ordering::Relaxed) { core::hint::spin_loop() }
> >
> > Weird formatting, we should also use `rustfmt` in examples.
> 
> `format_code_in_doc_comments` is a nightly `rustfmt` feature. I tried
> enabling it in `.rustfmt.toml` and running `rustfmt +nightly
> hrtimer.rs`. It did not have any effect. There is some discussion here:
> https://github.com/rust-lang/rustfmt/issues/3348
> 
> >
[...]
> >> +#[pinned_drop]
> >> +impl<T> PinnedDrop for Timer<T> {
> >> +    fn drop(self: Pin<&mut Self>) {
> >> +        // SAFETY: By struct invariant `self.timer` was initialized by
> >> +        // `hrtimer_init` so by C API contract it is safe to call
> >> +        // `hrtimer_cancel`.
> >> +        unsafe {
> >> +            bindings::hrtimer_cancel(self.timer.get());
> >> +        }
> >> +    }
> >> +}
> >
> > Why is this needed? The only way to schedule a timer using this API is
> > by having an `Arc` with a timer-containing struct inside. But to
> > schedule the `Arc`, you consume one refcount which is then sent to the
> > timer subsystem. So it is impossible for the refcount to drop below zero
> > while the timer is scheduled, but not yet running.
> > Do you need to call `hrtimer_cancel` after/while a timer is running?
> 
> This is not required any longer. It is a leftover from an earlier
> revision where timers could be stack allocated. I will remove it.
> 

So the plan is to add Arc<HasTimer> support first and stack allocated
timer later? If so, please do add a paragraph in the module level doc
describing the limition (e.g. stack allocated timers are not supported).

> > Also is it ok to call `hrtimer_cancel` inside the timer callback? Since
> > that can happen when the timer callback owns the last refcount.
> 
> That should be fine, `self` is still valid when the drop method is run?
> 
> >
> >> +
> >> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
> >> +/// facilitates queueing the timer through the pointer that implements the
> >> +/// trait.
> >> +///
> >> +/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
> >> +/// has a field of type `Timer`.
> >> +///
> >> +/// Target must be [`Sync`] because timer callbacks happen in another thread of
> >> +/// execution.
> >> +///
> >> +/// [`Box<T>`]: Box
> >> +/// [`Arc<T>`]: Arc
> >> +/// [`ARef<T>`]: crate::types::ARef
> >> +pub trait RawTimer: Sync {
> >> +    /// Schedule the timer after `expires` time units
> >> +    fn schedule(self, expires: u64);

This function should have a return value, see below:

> >> +}
[...]
> >> +impl<T> RawTimer for Arc<T>
> >> +where
> >> +    T: Send + Sync,
> >> +    T: HasTimer<T>,
> >> +{
> >> +    fn schedule(self, expires: u64) {
> >> +        let self_ptr = Arc::into_raw(self);
> >> +

so if the timer is already scheduled, re-scheduling will leak it, e.g.

	let timer: Arc<SomeTimer> = ...;

	let reschedule_handle = timer.clone(); // refcount == 2
	timer.schedule(...);

	...

	// later on, a reschedule is needed
	reschedule_handle.schedule(...); // refcount == 2

	// <timer callback invoked>
	Arc::drop();
	// refcount == 1, the Arc is leaked.

Looks to me `schedule()` should return the `Arc` back if it's already
in the queue.

TBH, if you don't need the re-schedule and cancel functionality, maybe
start with `impl<T> RawTimer for Pin<Box<T>>` first.

Regards,
Boqun

> >> +        // SAFETY: `self_ptr` is a valid pointer to a `T`
> >> +        let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
> >> +
> >> +        // `Timer` is `repr(transparent)`
> >> +        let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
> >> +
> >> +        // Schedule the timer - if it is already scheduled it is removed and
> >> +        // inserted
> >> +
> >> +        // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
> >> +        // initialized by `hrtimer_init`
> >> +        unsafe {
> >> +            bindings::hrtimer_start_range_ns(
> >> +                c_timer_ptr.cast_mut(),
> >> +                expires as i64,
> >> +                0,
> >> +                bindings::hrtimer_mode_HRTIMER_MODE_REL,
> >> +            );
> >> +        }
> >> +    }
> >> +}
> >> +
[...]

Powered by blists - more mailing lists