[PATCH v1 2/4] rust: hrtimer: Introduce HrTimerPin to make arming exclusive
FUJITA Tomonori <[email protected]>
| Newsgroups | org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
From: FUJITA Tomonori <[email protected]> Pin<&T> has the same hole that Arc<T> had: it is Copy and ScopedHrTimerPointer::start_scoped() is safe, so a copy captured by the closure can arm a timer while it is already armed and its callback may be running. Split the right to arm out of Pin<&T> into HrTimerPin<'a, T>, which is created from a Pin<&'a mut T> and consumed by start_scoped(). The borrow checker supplies the exclusivity here, and the closure keeps reading the object through the shared pinned reference returned by HrTimerPin::as_ref(). All four pointer types now separate sharing an object from arming its timer, so the restart operation no longer exists in the safe API. Drop it from the documentation. Fixes: 3f2a5ba784b8 ("rust: hrtimer: Add HrTimerCallbackContext and ::forward()") Signed-off-by: FUJITA Tomonori <[email protected]> --- rust/kernel/time/hrtimer.rs | 50 +++++++-------- rust/kernel/time/hrtimer/pin.rs | 105 +++++++++++++++++++++++--------- 2 files changed, 98 insertions(+), 57 deletions(-) diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs index a7587db1d552..d94275f2e93f 100644 --- a/rust/kernel/time/hrtimer.rs +++ b/rust/kernel/time/hrtimer.rs @@ -9,15 +9,15 @@ //! //! States: //! -//! - Stopped: initialized but not started, or cancelled, or not restarted. -//! - Started: initialized and started or restarted. +//! - Stopped: initialized but not started, cancelled, or the callback returned +//! `NoRestart`. +//! - Started: initialized and started, or the callback returned `Restart`. //! - Running: executing the callback. //! //! Operations: //! //! * Start //! * Cancel -//! * Restart //! //! Events: //! @@ -42,11 +42,7 @@ //! --------->| Stopped | | Started +---------->| Running | //! | | Cancel | | | | //! | |<----------------+ | | | -//! +-----------------+ +---------------+--+ +-----------------+ -//! ^ | -//! | | -//! +---------+ -//! Restart +//! +-----------------+ +------------------+ +-----------------+ //! ``` //! //! @@ -60,16 +56,13 @@ //! 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 returned and the timer has transitioned -//! out of the **running** state. +//! A `cancel` operation on a timer in the **running** state takes effect after +//! the handler has returned 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. -//! -//! When a type implements both `HrTimerPointer` and `Clone`, it is possible to -//! issue the `start` operation while the timer is in the **started** state. In -//! this case the `start` operation is equivalent to the `restart` operation. +//! The `start` operation consumes the pointer it is called on, so a timer in the +//! **started** or **running** state cannot be started again. It has to be +//! **cancelled** first. //! //! # Examples //! @@ -253,8 +246,8 @@ //! # }, //! # time::{ //! # hrtimer::{ -//! # ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerPointer, HrTimerRestart, -//! # HasHrTimer, RelativeMode, HrTimerCallbackContext +//! # ScopedHrTimerPointer, HrTimer, HrTimerCallback, HrTimerPin, HrTimerPointer, +//! # HrTimerRestart, HasHrTimer, RelativeMode, HrTimerCallbackContext //! # }, //! # Delta, Monotonic, //! # }, @@ -282,7 +275,7 @@ //! } //! //! impl HrTimerCallback for IntrusiveHrTimer { -//! type Pointer<'a> = Pin<&'a Self>; +//! type Pointer<'a> = HrTimerPin<'a, Self>; //! //! fn run(this: Pin<&Self>, _ctx: HrTimerCallbackContext<'_, Self>) -> HrTimerRestart { //! pr_info!("Timer called\n"); @@ -301,9 +294,12 @@ //! } //! //! stack_pin_init!( let has_timer = IntrusiveHrTimer::new() ); -//! has_timer.as_ref().start_scoped(Delta::from_micros(200), || { -//! while has_timer.flag.load(ordering::Relaxed) != 1 { -//! has_timer.cond.wait_for_completion(); +//! let timer_pin = HrTimerPin::new(has_timer); +//! let shared = timer_pin.as_ref(); +//! +//! timer_pin.start_scoped(Delta::from_micros(200), || { +//! while shared.flag.load(ordering::Relaxed) != 1 { +//! shared.cond.wait_for_completion(); //! } //! }); //! @@ -618,7 +614,8 @@ pub trait HrTimerPointer: Sync + Sized { /// [`HrTimerHandle`] returned by `start` would be unsound. This is the case for /// stack allocated timers. /// -/// Typical implementers are pinned references such as [`Pin<&T>`]. +/// Typical implementers are [`HrTimerPin`] and pinned references such as +/// [`Pin<&mut T>`]. /// /// # Safety /// @@ -640,8 +637,7 @@ pub unsafe trait UnsafeHrTimerPointer: Sync + Sized { /// until the timer is stopped and the callback has completed. type TimerHandle: HrTimerHandle; - /// Start the timer after `expires` time units. If the timer was already - /// running, it is restarted at the new expiry time. + /// Start the timer after `expires` time units. /// /// # Safety /// @@ -1111,7 +1107,7 @@ unsafe fn timer_container_of( mod arc; pub use arc::{ArcHrTimerHandle, HrTimerArc}; mod pin; -pub use pin::PinHrTimerHandle; +pub use pin::{HrTimerPin, PinHrTimerHandle}; mod pin_mut; pub use pin_mut::PinMutHrTimerHandle; // `box` is a reserved keyword, so prefix with `t` for timer diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs index 4d39ef781697..f44ac07cb722 100644 --- a/rust/kernel/time/hrtimer/pin.rs +++ b/rust/kernel/time/hrtimer/pin.rs @@ -10,50 +10,58 @@ use super::UnsafeHrTimerPointer; use core::pin::Pin; -/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be -/// running. -pub struct PinHrTimerHandle<'a, T> +/// A wrapper around a pinned shared reference that's guaranteed unique. +/// +/// The `HrTimerPin` type can be thought of as a special pinned reference to an object that +/// owns the permission to arm the [`HrTimer`] stored in the object. By ensuring that each +/// object has only one `HrTimerPin`, the owner of it is assured exclusive access to the arming +/// operation. Starting a timer consumes the `HrTimerPin`, and the returned +/// [`PinHrTimerHandle`] keeps the object borrowed, so the timer cannot be armed again until the +/// handle is dropped. +/// +/// While this `HrTimerPin` is unique, shared pinned references to the object can still be +/// obtained with [`HrTimerPin::as_ref`]. +/// +/// # Invariants +/// +/// * Each object has at most one `HrTimerPin`. +pub struct HrTimerPin<'a, T> where T: HasHrTimer<T>, { - pub(crate) inner: Pin<&'a T>, + pin: Pin<&'a T>, } -// SAFETY: We cancel the timer when the handle is dropped. The implementation of -// the `cancel` method will block if the timer handler is running. -unsafe impl<'a, T> HrTimerHandle for PinHrTimerHandle<'a, T> +impl<'a, T> HrTimerPin<'a, T> where T: HasHrTimer<T>, { - fn cancel(&mut self) -> bool { - let self_ptr: *const T = self.inner.get_ref(); - - // SAFETY: As we got `self_ptr` from a reference above, it must point to - // a valid `T`. - let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) }; - - // SAFETY: As `timer_ptr` is derived from a reference, it must point to - // a valid and initialized `HrTimer`. - unsafe { HrTimer::<T>::raw_cancel(timer_ptr) } + /// Create a `HrTimerPin` from an exclusive pinned reference to a `T`. + #[inline] + pub fn new(inner: Pin<&'a mut T>) -> Self { + // INVARIANT: We have an exclusive reference, so there is no `HrTimerPin` for this + // object. + Self { + pin: inner.into_ref(), + } } -} -impl<'a, T> Drop for PinHrTimerHandle<'a, T> -where - T: HasHrTimer<T>, -{ - fn drop(&mut self) { - self.cancel(); + /// Get a shared pinned reference to the object. + /// + /// The returned reference can be used to access the object, but not to arm its timer. + #[inline] + pub fn as_ref(&self) -> Pin<&'a T> { + self.pin } } // SAFETY: We capture the lifetime of `Self` when we create a `PinHrTimerHandle`, // so `Self` will outlive the handle. -unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a T> +unsafe impl<'a, T> UnsafeHrTimerPointer for HrTimerPin<'a, T> where T: Send + Sync, T: HasHrTimer<T>, - T: HrTimerCallback<Pointer<'a> = Self>, + T: HrTimerCallback<Pointer<'a> = HrTimerPin<'a, T>>, { type TimerMode = <T as HasHrTimer<T>>::TimerMode; type TimerHandle = PinHrTimerHandle<'a, T>; @@ -63,7 +71,7 @@ unsafe fn start( expires: <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::Expires, ) -> Self::TimerHandle { // Cast to pointer - let self_ptr: *const T = self.get_ref(); + let self_ptr: *const T = self.pin.get_ref(); // SAFETY: // - As we derive `self_ptr` from a reference above, it must point to a @@ -71,16 +79,53 @@ unsafe fn start( // - We keep `self` alive by wrapping it in a handle below. unsafe { T::start(self_ptr, expires) }; - PinHrTimerHandle { inner: self } + PinHrTimerHandle { inner: self.pin } + } +} + +/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be +/// running. +pub struct PinHrTimerHandle<'a, T> +where + T: HasHrTimer<T>, +{ + pub(crate) inner: Pin<&'a T>, +} + +// SAFETY: We cancel the timer when the handle is dropped. The implementation of +// the `cancel` method will block if the timer handler is running. +unsafe impl<'a, T> HrTimerHandle for PinHrTimerHandle<'a, T> +where + T: HasHrTimer<T>, +{ + fn cancel(&mut self) -> bool { + let self_ptr: *const T = self.inner.get_ref(); + + // SAFETY: As we got `self_ptr` from a reference above, it must point to + // a valid `T`. + let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) }; + + // SAFETY: As `timer_ptr` is derived from a reference, it must point to + // a valid and initialized `HrTimer`. + unsafe { HrTimer::<T>::raw_cancel(timer_ptr) } + } +} + +impl<'a, T> Drop for PinHrTimerHandle<'a, T> +where + T: HasHrTimer<T>, +{ + fn drop(&mut self) { + self.cancel(); } } -impl<'a, T> RawHrTimerCallback for Pin<&'a T> +impl<'a, T> RawHrTimerCallback for HrTimerPin<'a, T> where T: HasHrTimer<T>, T: HrTimerCallback<Pointer<'a> = Self>, { - type CallbackTarget<'b> = Self; + type CallbackTarget<'b> = Pin<&'a T>; unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart { // `HrTimer` is `repr(C)` -- 2.43.0