Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

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