Messages in this thread | | | From | Andreas Hindborg <> | Subject | Re: [PATCH] rust: hrtimer: introduce hrtimer support | Date | Tue, 30 Apr 2024 14:33:50 +0200 |
| |
Boqun Feng <boqun.feng@gmail.com> writes:
> On Fri, Apr 26, 2024 at 11:27:49AM +0200, Andreas Hindborg wrote: >> Benno Lossin <benno.lossin@proton.me> writes: >> >> > On 25.04.24 11:46, Andreas Hindborg wrote: >> >> From: Andreas Hindborg <a.hindborg@samsung.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@samsung.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.
Yes, there is a bit of race condition with the ktime series. I guess I will update this when the ktime patch is in. I am not sure if that was picked yet or what tree it is going to go through.
> >> > >> >> 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.
Since this is only using the atomic from Rust code, it should be fine right? There is no mixing of memory models on this memory location.
> >> >> +//! } >> >> +//! >> >> +//! 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).
I do not currently have any plans to add support for stack allocated timers. I can give it another try if someone needs it. I ran into problems with drop order when I tried it.
I will update the docs to mention this only supports heap allocated timers.
> >> > 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.
Nice catch. We can use `bindings::hrtimer_cancel` to drop the `Arc` used to enqueue if the timer was already enqueued. I think that should be OK as far as usability of the API goes?
> TBH, if you don't need the re-schedule and cancel functionality, maybe > start with `impl<T> RawTimer for Pin<Box<T>>` first.
I do not need to reschedule, but I need to support reference counted types, and cancel would be nice to have eventually.
Best regards, Andreas
| |