| Message ID | 20240313110515.70088-4-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust block device driver API and null block driver | expand |
On 3/13/24 12:05, Andreas Hindborg wrote:> From: Andreas Hindborg <a.hindborg@samsung.com> > > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> > --- > rust/kernel/block/mq/request.rs | 67 ++++++++++++++++++++++++++++++++- > 1 file changed, 66 insertions(+), 1 deletion(-) > > diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs > index cccffde45981..8b7f08f894be 100644 > --- a/rust/kernel/block/mq/request.rs > +++ b/rust/kernel/block/mq/request.rs > @@ -4,13 +4,16 @@ > //! > //! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h) > > +use kernel::hrtimer::RawTimer; > + > use crate::{ > bindings, > block::mq::Operations, > error::{Error, Result}, > + hrtimer::{HasTimer, TimerCallback}, > types::{ARef, AlwaysRefCounted, Opaque}, > }; > -use core::{ffi::c_void, marker::PhantomData, ops::Deref}; > +use core::{ffi::c_void, marker::PhantomData, ops::Deref, ptr::NonNull}; > > use crate::block::bio::Bio; > use crate::block::bio::BioIterator; > @@ -175,6 +178,68 @@ fn deref(&self) -> &Self::Target { > } > } > > +impl<T> RawTimer for RequestDataRef<T> > +where > + T: Operations, > + T::RequestData: HasTimer<T::RequestData>, > + T::RequestData: Sync, > +{ > + fn schedule(self, expires: u64) { > + let self_ptr = self.deref() as *const T::RequestData; > + core::mem::forget(self); > + > + // SAFETY: `self_ptr` is a valid pointer to a `T::RequestData` > + let timer_ptr = unsafe { T::RequestData::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 as *mut _, > + expires as i64, > + 0, > + bindings::hrtimer_mode_HRTIMER_MODE_REL, > + ); > + } > + } > +} > + > +impl<T> kernel::hrtimer::RawTimerCallback for RequestDataRef<T> > +where > + T: Operations, > + T: Sync, Why is this needed? Shouldn't this be `T::RequestData: Sync`? Is the `run` function below executed on a different thread compared to the `schedule` function above? If yes, then `T::RequestData` probably also needs to be `Send`. You also would need to adjust the bounds in the impl above.
Benno Lossin <benno.lossin@proton.me> writes: > On 3/13/24 12:05, Andreas Hindborg wrote:> From: Andreas Hindborg <a.hindborg@samsung.com> >> >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> >> --- >> rust/kernel/block/mq/request.rs | 67 ++++++++++++++++++++++++++++++++- >> 1 file changed, 66 insertions(+), 1 deletion(-) >> >> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs >> index cccffde45981..8b7f08f894be 100644 >> --- a/rust/kernel/block/mq/request.rs >> +++ b/rust/kernel/block/mq/request.rs >> @@ -4,13 +4,16 @@ >> //! >> //! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h) >> >> +use kernel::hrtimer::RawTimer; >> + >> use crate::{ >> bindings, >> block::mq::Operations, >> error::{Error, Result}, >> + hrtimer::{HasTimer, TimerCallback}, >> types::{ARef, AlwaysRefCounted, Opaque}, >> }; >> -use core::{ffi::c_void, marker::PhantomData, ops::Deref}; >> +use core::{ffi::c_void, marker::PhantomData, ops::Deref, ptr::NonNull}; >> >> use crate::block::bio::Bio; >> use crate::block::bio::BioIterator; >> @@ -175,6 +178,68 @@ fn deref(&self) -> &Self::Target { >> } >> } >> >> +impl<T> RawTimer for RequestDataRef<T> >> +where >> + T: Operations, >> + T::RequestData: HasTimer<T::RequestData>, >> + T::RequestData: Sync, >> +{ >> + fn schedule(self, expires: u64) { >> + let self_ptr = self.deref() as *const T::RequestData; >> + core::mem::forget(self); >> + >> + // SAFETY: `self_ptr` is a valid pointer to a `T::RequestData` >> + let timer_ptr = unsafe { T::RequestData::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 as *mut _, >> + expires as i64, >> + 0, >> + bindings::hrtimer_mode_HRTIMER_MODE_REL, >> + ); >> + } >> + } >> +} >> + >> +impl<T> kernel::hrtimer::RawTimerCallback for RequestDataRef<T> >> +where >> + T: Operations, >> + T: Sync, > > Why is this needed? Shouldn't this be `T::RequestData: Sync`? > > Is the `run` function below executed on a different thread compared to > the `schedule` function above? > If yes, then `T::RequestData` probably also needs to be `Send`. > You also would need to adjust the bounds in the impl above. It's a typo, thanks for spotting. It should be `T::RequestData: Sync`. BR Andreas
diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs index cccffde45981..8b7f08f894be 100644 --- a/rust/kernel/block/mq/request.rs +++ b/rust/kernel/block/mq/request.rs @@ -4,13 +4,16 @@ //! //! C header: [`include/linux/blk-mq.h`](srctree/include/linux/blk-mq.h) +use kernel::hrtimer::RawTimer; + use crate::{ bindings, block::mq::Operations, error::{Error, Result}, + hrtimer::{HasTimer, TimerCallback}, types::{ARef, AlwaysRefCounted, Opaque}, }; -use core::{ffi::c_void, marker::PhantomData, ops::Deref}; +use core::{ffi::c_void, marker::PhantomData, ops::Deref, ptr::NonNull}; use crate::block::bio::Bio; use crate::block::bio::BioIterator; @@ -175,6 +178,68 @@ fn deref(&self) -> &Self::Target { } } +impl<T> RawTimer for RequestDataRef<T> +where + T: Operations, + T::RequestData: HasTimer<T::RequestData>, + T::RequestData: Sync, +{ + fn schedule(self, expires: u64) { + let self_ptr = self.deref() as *const T::RequestData; + core::mem::forget(self); + + // SAFETY: `self_ptr` is a valid pointer to a `T::RequestData` + let timer_ptr = unsafe { T::RequestData::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 as *mut _, + expires as i64, + 0, + bindings::hrtimer_mode_HRTIMER_MODE_REL, + ); + } + } +} + +impl<T> kernel::hrtimer::RawTimerCallback for RequestDataRef<T> +where + T: Operations, + T: Sync, + T::RequestData: HasTimer<T::RequestData>, + T::RequestData: TimerCallback<Receiver = Self>, +{ + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart { + // `Timer` is `repr(transparent)` + let timer_ptr = ptr.cast::<kernel::hrtimer::Timer<T::RequestData>>(); + + // SAFETY: By C API contract `ptr` is the pointer we passed when + // enqueing the timer, so it is a `Timer<T::RequestData>` embedded in a `T::RequestData` + let receiver_ptr = unsafe { T::RequestData::timer_container_of(timer_ptr) }; + + // SAFETY: The pointer was returned by `T::timer_container_of` so it + // points to a valid `T::RequestData` + let request_ptr = unsafe { bindings::blk_mq_rq_from_pdu(receiver_ptr.cast::<c_void>()) }; + + // SAFETY: We own a refcount that we leaked during `RawTimer::schedule()` + let dref = RequestDataRef::new(unsafe { + ARef::from_raw(NonNull::new_unchecked(request_ptr.cast::<Request<T>>())) + }); + + T::RequestData::run(dref); + + bindings::hrtimer_restart_HRTIMER_NORESTART + } +} + // SAFETY: All instances of `Request<T>` are reference counted. This // implementation of `AlwaysRefCounted` ensure that increments to the ref count // keeps the object alive in memory at least until a matching reference count