Message-ID: <ZxkPC-dLRBqBKZ5J@Boquns-Mac-mini.local>
Date: Wed, 23 Oct 2024 07:58:19 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: Alice Ryhl <aliceryhl@...gle.com>, rust-for-linux@...r.kernel.org,
	dakr@...hat.com, linux-kernel@...r.kernel.org, airlied@...hat.com,
	miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

On Wed, Oct 23, 2024 at 01:26:14PM +0300, Abdiel Janulgue wrote:
> 
> 
> On 23/10/2024 12:28, Alice Ryhl wrote:
> > On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
> > <abdiel.janulgue@...il.com> wrote:
> > > 
> > > Add the 'Owned' type, a simple smart pointer type that owns the
> > > underlying data.
> > > 
> > > An object implementing `Ownable' can constructed by wrapping it in
> > > `Owned`, which has the advantage of allowing fine-grained control
> > > over it's resource allocation and deallocation.
> > > 
> > > Co-developed-by: Boqun Feng <boqun.feng@...il.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> > > ---
> > >   rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 62 insertions(+)
> > > 
> > > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > > index ced143600eb1..3f632916bd4d 100644
> > > --- a/rust/kernel/types.rs
> > > +++ b/rust/kernel/types.rs
> > > @@ -429,3 +429,65 @@ pub enum Either<L, R> {
> > >       /// Constructs an instance of [`Either`] containing a value of type `R`.
> > >       Right(R),
> > >   }
> > > +
> > > +/// A smart pointer that owns the underlying data `T`.
> > > +///
> > > +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> > > +/// returned as a wrapper for `T` in `T`'s constructor.
> > > +/// When an object adds an option of being constructed this way, in addition to implementing
> > > +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> > > +/// resource allocation and deallocation happens.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// The pointer is always valid and owns the underlying data.
> > > +pub struct Owned<T: Ownable> {
> > > +    ptr: NonNull<T>,
> > > +}
> > > +
> > > +impl<T: Ownable> Owned<T> {
> > > +    /// Creates a new smart pointer that owns `T`.
> > > +    ///
> > > +    /// # Safety
> > > +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> > > +    /// in other words, no other entity should free the underlying data.
> > > +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> > 
> > Please rename this function to from_raw to match the name used by
> > other similar functions.
> > 
> > Also, I don't love this wording. We don't really want to guarantee
> > that it is unique. For example, pages have one primary owner, but
> > there can be others who also have refcounts to the page, so it's not
> > really unique. I think you just want to say that `ptr` must point at a

But then when `Owned<Page>` dropped, it will call __free_pages() which
invalidate any other existing users. Do you assume that the users will
use pointers anyway, so it's their unsafe responsiblity to guarantee
that they don't use an invalid pointer?

Also I assume you mean the others have refcounts to the page *before* an
`Owned<Page>` is created, right? Because if we really have a use case
where we want to have multiple users of a page after `Owned<Page>`
created, we should better provide a `Owned<Page>` to `ARef<Page>`
function.

Regards,
Boqun

> > valid value of type `T`, and it must remain valid until `ptr_drop` is
> > called.
> > 
> > > +impl<T: Ownable> Deref for Owned<T> {
> > > +    type Target = T;
> > > +
> > > +    fn deref(&self) -> &Self::Target {
> > > +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> > > +        // safe to dereference it.
> > > +        unsafe { self.ptr.as_ref() }
> > > +    }
> > > +}
> > > +
> > > +impl<T: Ownable> DerefMut for Owned<T> {
> > > +    fn deref_mut(&mut self) -> &mut Self::Target {
> > > +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> > > +        // safe to dereference it.
> > > +        unsafe { self.ptr.as_mut() }
> > > +    }
> > > +}
> > 
> > We only want Deref, not DerefMut. DerefMut both requires uniqueness in
> > a way that is stronger than what we can really promise, and it also
> > implies that the value is *not* pinned, but we generally want to use
> > Owned with pinned things. Thus, we can't use DerefMut.
> > 
> > > +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
> > > +/// `ptr_drop` will be called.
> > > +pub unsafe trait Ownable {
> > > +    /// # Safety
> > > +    /// This could only be called in the `Owned::drop` function.
> > > +    unsafe fn ptr_drop(ptr: *mut Self);
> > > +}
> > > +
> > > +impl<T: Ownable> Drop for Owned<T> {
> > > +    fn drop(&mut self) {
> > > +       // SAFETY: In Owned<T>::drop.
> > > +       unsafe {
> > > +           <T as Ownable>::ptr_drop(self.ptr.as_mut());
> > 
> > This uses NonNull::as_mut which creates a mutable reference. You
> > should use NonNull::as_ptr instead.
> > 
> > Also this code will look better if you move the semicolon so it is
> > outside of the unsafe block.
> 
> Thanks for the feedback! Will do that in next revision.
> 
> /Abdiel

Powered by blists - more mailing lists