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@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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 {
+	// SAFETY: Per function safety requirement.
+	Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
+    }
+}
+
+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() }
+    }
+}
+
+/// 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());
+	}
+    }
+}
-- 
2.43.0

On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
<abdiel.janulgue@gmail.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@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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
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.


Alice

On 23/10/2024 12:28, Alice Ryhl wrote:
> On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
> <abdiel.janulgue@gmail.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@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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
> 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

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@gmail.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@gmail.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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

On Wed, Oct 23, 2024 at 4:58 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> 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@gmail.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@gmail.com>
> > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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.

The __free_pages function just decrements a refcount. If there are
other references to it, it's not actually freed.

Alice

On Wed, Oct 23, 2024 at 07:52:23PM +0200, Alice Ryhl wrote:
[...]
> > > > > +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.
> 
> The __free_pages function just decrements a refcount. If there are
> other references to it, it's not actually freed.
> 

Then why don't we use page_put() there? ;-) And instead of
`Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?

Regards,
Boqun

> Alice

On Wed, Oct 23, 2024 at 8:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Oct 23, 2024 at 07:52:23PM +0200, Alice Ryhl wrote:
> [...]
> > > > > > +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.
> >
> > The __free_pages function just decrements a refcount. If there are
> > other references to it, it's not actually freed.
> >
>
> Then why don't we use page_put() there? ;-) And instead of
> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?

I don't think there's a function called page_put?

Alice

On Thu, Oct 24, 2024 at 9:23 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Oct 23, 2024 at 8:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Wed, Oct 23, 2024 at 07:52:23PM +0200, Alice Ryhl wrote:
> > [...]
> > > > > > > +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.
> > >
> > > The __free_pages function just decrements a refcount. If there are
> > > other references to it, it's not actually freed.
> > >
> >
> > Then why don't we use page_put() there? ;-) And instead of
> > `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
>
> I don't think there's a function called page_put?

Sorry I confused myself. It's because it's called put_page.

Alice

Hi Alice, Boqun:

On 24/10/2024 10:33, Alice Ryhl wrote:
>>>>>>>
>>>>>>> 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.
>>>>
>>>> The __free_pages function just decrements a refcount. If there are
>>>> other references to it, it's not actually freed.
>>>>
>>>
>>> Then why don't we use page_put() there? ;-) And instead of
>>> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
>>
>> I don't think there's a function called page_put?
> 
> Sorry I confused myself. It's because it's called put_page.
> 

How do I proceed with this? Should we use the page's reference count to 
decide when to free the allocation and use put_page() instead of 
__free_pages() in Page::Drop?.

In that case, there would be no need for `Ownable`, right? As we could 
just return ARef<Page> in both vmalloc_to_page() case and in 
Page::alloc_page(), letting the kernel handle ownership internally.

Regards,
Abdiel

On Fri, Nov 1, 2024 at 2:38 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Hi Alice, Boqun:
>
> On 24/10/2024 10:33, Alice Ryhl wrote:
> >>>>>>>
> >>>>>>> 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.
> >>>>
> >>>> The __free_pages function just decrements a refcount. If there are
> >>>> other references to it, it's not actually freed.
> >>>>
> >>>
> >>> Then why don't we use page_put() there? ;-) And instead of
> >>> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
> >>
> >> I don't think there's a function called page_put?
> >
> > Sorry I confused myself. It's because it's called put_page.
> >
>
> How do I proceed with this? Should we use the page's reference count to
> decide when to free the allocation and use put_page() instead of
> __free_pages() in Page::Drop?.
>
> In that case, there would be no need for `Ownable`, right? As we could
> just return ARef<Page> in both vmalloc_to_page() case and in
> Page::alloc_page(), letting the kernel handle ownership internally.

Yes, it seems like we don't need Ownable for Page in the end. You can
use ARef together with put_page() and get_page().

Alice

On Wed, Oct 23, 2024 at 01:44:45AM +0300, Abdiel Janulgue 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@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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 {
> +	// SAFETY: Per function safety requirement.
> +	Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
> +    }

I wonder if this should just be

   pub fn new(ptr: NonNull<T>) -> Self

This way the caller could decide whether to use the fallible variant
`NonNull::new` or `NonNull::new_unchecked`.

Alternatively, you could have your own `new` and `new_unchecked` methods, but
that seems a bit redundant.

Sometimes this might be more elegant. For instance in the page code, as it is
now, you have to give up on

   let page = NonNull::new(page).ok_or(AllocError)?;

and instead have to do a NULL check by hand for the subsequent unsafe call to
`Owned::to_owned`.

> +}
> +
> +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() }
> +    }
> +}
> +
> +/// 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());
> +	}
> +    }
> +}
> -- 
> 2.43.0
>