Re: [PATCH v1 2/3] rust: add pin-init API
From: y86-dev
Date: Thu Mar 16 2023 - 05:38:40 EST
On Wednesday, March 15th, 2023 at 21:07, Gary Guo <gary@xxxxxxxxxxx> wrote:
> On Mon, 13 Mar 2023 01:23:52 +0000
> y86-dev <y86-dev@xxxxxxxxxxxxxx> wrote:
>
> > +
> > +#[macro_export]
> > +macro_rules! pin_init {
> > + ($(&$this:ident in)? $t:ident $(<$($generics:ty),* $(,)?>)? {
>
> For this part here, I would suggest the syntax to be
>
> $t:ident $(:: <$($generics:ty),* $(,)?>)
>
> so that it's compatible with struct expression syntax.
Sure.
> > + (init_slot:
> > + @typ($ty:ty),
> > + @slot($slot:ident),
> > + // In-place initialization syntax.
> > + @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
> > + ) => {
> > + let $field = $val;
> > + // Call the initializer.
> > + //
> > + // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
> > + // return when an error/panic occurs.
> > + // We also use the `__PinData` to request the correct trait (`Init` or `PinInit`).
> > + unsafe {
> > + <$ty as $crate::init::__PinData>::__PinData::$field(
> > + ::core::ptr::addr_of_mut!((*$slot).$field),
> > + $field,
> > + )?;
> > + }
> > + // Create the drop guard.
> > + //
> > + // SAFETY: We forget the guard later when initialization has succeeded.
> > + let $field = unsafe {
> > + $crate::init::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> > + };
> > + // Only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> > + let $field = &$field;
>
> Instead of shadowing, you can use lifetime extension of `let x = &v` pattern directly:
>
> let $field = &unsafe {
> $crate::init::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> };
>
> (same for the value init case and the `init!` macro)
Sure.
> > +
> > + $crate::try_pin_init!(init_slot:
> > + @typ($ty),
> > + @slot($slot),
> > + @munch_fields($($rest)*),
> > + );
> > + };
> > + (init_slot:
> > + @typ($ty:ty),
> > + @slot($slot:ident),
> > + // Direct value init, this is safe for every field.
> > + @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
> > + ) => {
> > + $(let $field = $val;)?
> > + // Call the initializer.
> > + //
> > + // SAFETY: The memory at `slot` is uninitialized.
> > + unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
>
> Could this be
>
> unsafe { ::core::ptr::addr_of_mut!((*$slot).$field).write($field) };
Sure.
> ? Not sure if this has any implication on type inference though.
It should not.
> > +
> > +/// Trait facilitating pinned destruction.
> > +///
> > +/// Use [`pinned_drop`] to implement this trait safely:
> > +///
> > +/// ```rust
> > +/// # use kernel::sync::Mutex;
> > +/// use kernel::macros::pinned_drop;
> > +/// use core::pin::Pin;
> > +/// #[pin_data(PinnedDrop)]
> > +/// struct Foo {
> > +/// #[pin]
> > +/// mtx: Mutex<usize>,
> > +/// }
> > +///
> > +/// #[pinned_drop]
> > +/// impl PinnedDrop for Foo {
> > +/// fn drop(self: Pin<&mut Self>) {
> > +/// pr_info!("Foo is being dropped!");
> > +/// }
> > +/// }
> > +/// ```
> > +///
> > +/// # Safety
> > +///
> > +/// This trait must be implemented with [`pinned_drop`].
> > +///
> > +/// [`pinned_drop`]: kernel::macros::pinned_drop
> > +pub unsafe trait PinnedDrop: __PinData {
> > + /// Executes the pinned destructor of this type.
> > + ///
> > + /// # Safety
> > + ///
> > + /// Only call this from `<Self as Drop>::drop`.
> > + unsafe fn drop(self: Pin<&mut Self>);
> > +
> > + // Used by the `pinned_drop` proc-macro to ensure that only safe operations are used in `drop`.
> > + // the function should not be called.
> > + #[doc(hidden)]
> > + fn __ensure_no_unsafe_op_in_drop(self: Pin<&mut Self>);
>
> One idea to avoid this extra function is to have an unsafe token to the
> drop function.
>
> fn drop(self: Pin<&mut Self>, token: TokenThatCanOnlyBeCreatedUnsafely);
What is wrong with having this extra function? If the problem is that this
function might be called, then we could add a parameter with an
unconstructable type.
I think that `drop` should be `unsafe`, since it really does have
the requirement of only being called in the normal drop impl.
> > +}
> > +
> > +/// Smart pointer that can initialize memory in-place.
> > +pub trait InPlaceInit<T>: Sized {
> > + /// Use the given initializer to in-place initialize a `T`.
> > + ///
> > + /// If `T: !Unpin` it will not be able to move afterwards.
> > + fn pin_init<E>(init: impl PinInit<T, E>) -> error::Result<Pin<Self>>
> > + where
> > + Error: From<E>;
> > +
> > + /// Use the given initializer to in-place initialize a `T`.
> > + fn init<E>(init: impl Init<T, E>) -> error::Result<Self>
> > + where
> > + Error: From<E>;
> > +}
>
> Is this trait used? Or the methods could be inherent methods?
I need an extension trait for `Box`, since it is inside of the `alloc`
crate and so I figured that I might as well use it for other types. I do
not think we can avoid the extension trait for `Box`, but I could make the
functions for `UniqueArc` inherent. What do you think?
> > +/// An initializer that leaves the memory uninitialized.
> > +///
> > +/// The initializer is a no-op. The `slot` memory is not changed.
> > +#[inline]
> > +pub fn uninit<T>() -> impl Init<MaybeUninit<T>> {
> > + // SAFETY: The memory is allowed to be uninitialized.
> > + unsafe { init_from_closure(|_| Ok(())) }
> > +}
>
> Do you think there's a value to have a `Uninitable` which is
> implemented for both `MaybeUninit` and `Opaque`?
If we really need it for `Opaque`, then it probably should be an inherent
function. I do not really see additional use for an `Uninitable` trait.
> > +
> > +/// Convert a value into an initializer.
> > +///
> > +/// Directly moves the value into the given `slot`.
> > +///
> > +/// Note that you can just write `field: value,` in all initializer macros. This function's purpose
> > +/// is to provide compatibility with APIs that only have `PinInit`/`Init` as parameters.
> > +#[inline]
> > +pub fn from_value<T>(value: T) -> impl Init<T> {
>
> How about `unsafe impl<T> Init<T> for T`?
That should work, was worried about inference issues, but I tried it and
found none. I will change it.
> > +macro_rules! impl_zeroable {
> > + ($($t:ty),*) => {
> > + $(unsafe impl Zeroable for $t {})*
> > + };
> > +}
>
> I personally would do ($($t:ty,)*) and then we can have
>
> impl_zeroable!(
> // SAFETY: All primitives that are allowed to be zero.
> bool,
> char,
> u8, u16, u32, u64, u128, usize,
> i8, i16, i32, i64, i128, isize,
> f32, f64,
> // SAFETY: There is nothing to zero.
> core::marker::PhantomPinned, Infallible, (),
> ......
> );
Sure.
> > +// SAFETY: All primitives that are allowed to be zero.
> > +impl_zeroable!(
> > + bool, char, u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize, f32, f64
> > +);
> > +// SAFETY: There is nothing to zero.
> > +impl_zeroable!(core::marker::PhantomPinned, Infallible, ());
> > +
> > +// SAFETY: We are allowed to zero padding bytes.
> > +unsafe impl<const N: usize, T: Zeroable> Zeroable for [T; N] {}
> > +
> > +// SAFETY: There is nothing to zero.
> > +unsafe impl<T: ?Sized> Zeroable for PhantomData<T> {}
> > +
> > +// SAFETY: `null` pointer is valid.
> > +unsafe impl<T: ?Sized> Zeroable for *mut T {}
> > +unsafe impl<T: ?Sized> Zeroable for *const T {}
> > +
> > +macro_rules! impl_tuple_zeroable {
> > + ($(,)?) => {};
> > + ($first:ident, $($t:ident),* $(,)?) => {
> > + // SAFETY: All elements are zeroable and padding can be zero.
> > + unsafe impl<$first: Zeroable, $($t: Zeroable),*> Zeroable for ($first, $($t),*) {}
> > + impl_tuple_zeroable!($($t),* ,);
> > + }
> > +}
> > +
> > +impl_tuple_zeroable!(A, B, C, D, E, F, G, H, I, J);
> > +
> > +// This trait is only implemented via the `#[pin_data]` proc-macro. It is used to facilitate
> > +// the pin projections within the initializers.
> > +#[doc(hidden)]
> > +pub unsafe trait __PinData {
> > + type __PinData;
> > +}
> > +
> > +/// Stack initializer helper type. Use [`stack_pin_init`] instead of this primitive.
>
> `#[doc(hidden)]`?
This trait is implementation detail of the `#[pin_data]` macro. Why should
it be visible in the rust-docs?
> > diff --git a/rust/kernel/init/common.rs b/rust/kernel/init/common.rs
> > new file mode 100644
> > index 000000000000..f8c6e9dff786
> > --- /dev/null
> > +++ b/rust/kernel/init/common.rs
> > @@ -0,0 +1,42 @@
> > +// SPDX-License-Identifier: Apache-2.0 OR MIT
> > +
> > +//! Module containing common kernel initializer functions.
> > +
> > +use crate::{
> > + init::{self, PinInit},
> > + types::Opaque,
> > +};
> > +
> > +macro_rules! create_func {
> > + ($name:ident $(, $arg_name:ident: $arg_typ:ident)*) => {
> > + /// Create an initializer using the given initializer function from C.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The given function **must** under all circumstances initialize the memory location to a
> > + /// valid `T`. If it fails to do so it results in UB.
> > + ///
> > + /// If any parameters are given, those need to be valid for the function. Valid means that
> > + /// calling the function with those parameters complies with the above requirement **and**
> > + /// every other requirement on the function itself.
> > + pub unsafe fn $name<T $(, $arg_typ)*>(
> > + init_func: unsafe extern "C" fn(*mut T $(, $arg_name: $arg_typ)*),
> > + $($arg_name: $arg_typ,)*
> > + ) -> impl PinInit<Opaque<T>> {
> > + // SAFETY: The safety contract of this function ensures that `init_func` fully
> > + // initializes `slot`.
> > + unsafe {
> > + init::pin_init_from_closure(move |slot| {
> > + init_func(Opaque::raw_get(slot) $(, $arg_name)*);
> > + Ok(())
> > + })
> > + }
> > + }
> > + }
> > +}
> > +
> > +create_func!(ffi_init);
> > +create_func!(ffi_init1, arg1: A1);
> > +create_func!(ffi_init2, arg1: A1, arg2: A2);
> > +create_func!(ffi_init3, arg1: A1, arg2: A2, arg3: A3);
> > +create_func!(ffi_init4, arg1: A1, arg2: A2, arg3: A3, arg4: A4);
>
> This part can be a separate commit (or perhaps in a separate patch
> series when it's being used).
Will make a separate commit, since the sync module will use it.
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index f2f1c83d72ba..5b4f64dd3ac8 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -17,7 +17,8 @@
> >
> > use crate::{
> > bindings,
> > - error::Result,
> > + error::{Error, Result},
> > + init::{InPlaceInit, Init, PinInit},
> > types::{ForeignOwnable, Opaque},
> > };
> > use alloc::boxed::Box;
> > @@ -163,6 +164,28 @@ impl<T> Arc<T> {
> > // `Arc` object.
> > Ok(unsafe { Self::from_inner(Box::leak(inner).into()) })
> > }
> > +
> > + /// Use the given initializer to in-place initialize a `T`.
> > + ///
> > + /// If `T: !Unpin` it will not be able to move afterwards.
> > + #[inline]
> > + pub fn pin_init<E>(init: impl PinInit<T, E>) -> Result<Self>
> > + where
> > + Error: From<E>,
> > + {
> > + UniqueArc::pin_init(init).map(|u| u.into())
> > + }
> > +
> > + /// Use the given initializer to in-place initialize a `T`.
> > + ///
> > + /// This is equivalent to [`pin_init`], since an `Arc` is always pinned.
> > + #[inline]
> > + pub fn init<E>(init: impl Init<T, E>) -> Result<Self>
> > + where
> > + Error: From<E>,
> > + {
> > + UniqueArc::init(init).map(|u| u.into())
> > + }
> > }
> >
> > impl<T: ?Sized> Arc<T> {
> > @@ -489,6 +512,17 @@ impl<T> UniqueArc<MaybeUninit<T>> {
> > /// Converts a `UniqueArc<MaybeUninit<T>>` into a `UniqueArc<T>` by writing a value into it.
> > pub fn write(mut self, value: T) -> UniqueArc<T> {
> > self.deref_mut().write(value);
> > + // SAFETY: We just wrote the value to be initialized.
> > + unsafe { self.assume_init() }
> > + }
> > +
> > + /// Unsafely assume that `self` is initialized.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller guarantees that the value behind this pointer has been initialized. It is
> > + /// *immediate* UB to call this when the value is not initialized.
> > + pub unsafe fn assume_init(self) -> UniqueArc<T> {
>
> This part should be a separate commit.
Sure.
> > let inner = ManuallyDrop::new(self).inner.ptr;
> > UniqueArc {
> > // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (which won't be
>
> Best,
> Gary
Cheers,
Benno