Messages in this thread | | | From | Andreas Hindborg <> | Subject | Re: [PATCH] rust: hrtimer: introduce hrtimer support | Date | Mon, 29 Apr 2024 15:47:47 +0200 |
| |
Alice Ryhl <aliceryhl@google.com> writes:
> Andreas Hindborg <nmi@metaspace.dk> writes: >> 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 very similar to the workqueue I implemented. It seems like > we have the following correspondence between them: > > * Your HasTimer is my HasWork. > * Your RawTimerCallback is my WorkItemPointer. > * Your TimerCallback is my WorkItem. > * Your RawTimer is my RawWorkItem. (but the match isn't great here) > > I think it would make sense to have the names be more consistent. I > propose renaming RawTimerCallback to TimerCallbackPointer. > > Or we can name them TimerEntry and RawTimerEntry?
I took some advice from Benno and merged `RawTimerCallback` with `RawTimer` and renamed the resulting trait `TimerPointer`. There is not really any reason they should be split for the `hrtimer` as far as I can tell.
> I also note that the method on your RawTimer trait seems to be the > public API of how you're supposed to schedule a timer, whereas the > workqueue RawWorkItem only provides a raw low-level method, and instead > has the "public API" be a function on the Workqueue struct. > > I'm not such a big fan of having the primary method everying is supposed > to use be a method on a trait whose name starts with "Raw".
I would remove the `Raw`.
> It's worth > considering whether it makes more sense to have a free-standing function > called `schedule_timer` and have that be how you're supposed to schedule > timers, instead of the RawTimer trait.
I think being able to call `my_timer_containing_struct.schedule()` is nice.
> >> +#[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()); >> + } >> + } >> +} > > Assuming that this is mirroring the workqueue, then this is not > necessary. The timer owns a refcount to the element, so the destructor > cannot run while the timer is scheduled.
Yes, it is very much a mirror. Yes, it is a leftover from trying to support stack allocated timers. I will remove it.
> Also, as a generaly note, putting semicolons outside of unsafe blocks > formats better.
👍
> >> +/// Implemented by pointer types that can be the target of a C timer callback. >> +pub trait RawTimerCallback: RawTimer { >> + /// Callback to be called from C. >> + /// >> + /// # Safety >> + /// >> + /// Only to be called by C code in `hrtimer`subsystem. >> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart; >> +} > > Safety comment is missing a space.
Thanks.
> >> +/// Implemented by pointers to structs that can the target of a timer callback >> +pub trait TimerCallback { >> + /// Type of `this` argument for `run()`. >> + type Receiver: RawTimerCallback; >> + >> + /// Called by the timer logic when the timer fires >> + fn run(this: Self::Receiver); >> +} > > The documentation says that this is implemented by pointers to structs, > but that is not the case.
I will update the doc comment, it should say "implemented by structs that can be the target...". Thanks.
> >> +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); >> + >> + // 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>(); > > I would add an `raw_get` method to `Timer` instead of this cast, > analogous to `Work::raw_get`. >
Why is that? It is a lot of extra code, extra safety comments, etc.
In any case, would you prefer to implement said method with a cast (which we can because `Timer` is transparent), or by `Opaque::raw_get`:
`Opaque::raw_get(core::ptr::addr_of!((*ptr).timer))`
Best regards, Andreas
| |