Message-ID: <20230316173848.18b45232.gary@garyguo.net>
Date:   Thu, 16 Mar 2023 17:38:48 +0000
From:   Gary Guo <gary@...yguo.net>
To:     y86-dev <y86-dev@...tonmail.com>
Cc:     "ojeda@...nel.org" <ojeda@...nel.org>,
        "alex.gaynor@...il.com" <alex.gaynor@...il.com>,
        "wedsonaf@...il.com" <wedsonaf@...il.com>,
        "boqun.feng@...il.com" <boqun.feng@...il.com>,
        "bjorn3_gh@...tonmail.com" <bjorn3_gh@...tonmail.com>,
        "rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: Re: [PATCH v1 2/3] rust: add pin-init API

On Thu, 16 Mar 2023 09:38:16 +0000
y86-dev <y86-dev@...tonmail.com> wrote:

> > > +
> > > +/// 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.

The point to avoid having two functions with the same body. This would
require double the amount of checks needed by the compiler (and make
error message worth if anything's wrong in the body of `drop`).

This current approach is really just a hack to avoid code from doing
unsafe stuff without using `unsafe` block -- and the best solution is
just to avoid make `drop` function unsafe. However we don't want drop
function to be actually called from safe code, and that's the point of
a token that can only be created unsafely is force `drop` to *not* be
called by safe code. The token is a proof that `unsafe` is being used.

This way the `__ensure_no_unsafe_op_in_drop` function would not be
needed.

> 
> > > +}
> > > +
> > > +/// 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?

Good point, I forget about `Box`.

> 
> > > +/// 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.

Fair.

> > > +// 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?

I am commenting about `stack_pin_init` (note the doc comment above my
comment). `StackInit` is an implementation detail of `stack_pin_init`
and shouldn't be exposed, IMO. Or do you think manual use of
`StackInit` is needed?

Best,
Gary

Powered by blists - more mailing lists