[PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Benno Lossin]

Sometimes (see [1]) it is necessary to drop the value inside of a
`Box<T>`, but retain the allocation. For example to reuse the allocation
in the future.
Introduce a new function `drop_contents` that turns a `Box<T>` into
`Box<MaybeUninit<T>>` by dropping the value.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Link: https://lore.kernel.org/rust-for-linux/20240418-b4-rbtree-v3-5-323e134390ce@google.com/ [1]
---
 rust/kernel/alloc/box_ext.rs | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
index 829cb1c1cf9e..076d5de5f47d 100644
--- a/rust/kernel/alloc/box_ext.rs
+++ b/rust/kernel/alloc/box_ext.rs
@@ -4,7 +4,7 @@
 
 use super::{AllocError, Flags};
 use alloc::boxed::Box;
-use core::mem::MaybeUninit;
+use core::{mem::MaybeUninit, ptr, result::Result};
 
 /// Extensions to [`Box`].
 pub trait BoxExt<T>: Sized {
@@ -17,6 +17,22 @@ pub trait BoxExt<T>: Sized {
     ///
     /// The allocation may fail, in which case an error is returned.
     fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
+
+    /// Drops the contents, but keeps the allocation.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::alloc::{flags, box_ext::BoxExt};
+    /// let value = Box::new([0; 32], flags::GFP_KERNEL)?;
+    /// assert_eq!(*value, [0; 32]);
+    /// let value = Box::drop_contents(value);
+    /// // Now we can re-use `value`:
+    /// let value = Box::write(value, [1; 32]);
+    /// assert_eq!(*value, [1; 32]);
+    /// # Ok::<(), Error>(())
+    /// ```
+    fn drop_contents(this: Self) -> Box<MaybeUninit<T>>;
 }
 
 impl<T> BoxExt<T> for Box<T> {
@@ -53,4 +69,12 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
         // zero-sized types, we use `NonNull::dangling`.
         Ok(unsafe { Box::from_raw(ptr) })
     }
+
+    fn drop_contents(this: Self) -> Box<MaybeUninit<T>> {
+        let ptr = Box::into_raw(this);
+        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
+        unsafe { ptr::drop_in_place(ptr) };
+        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
+        unsafe { Box::from_raw(ptr.cast()) }
+    }
 }
-- 
2.45.2

[PATCH v3 2/2] rust: init: add `write_[pin_]init` functions

[Benno Lossin]

Sometimes it is necessary to split allocation and initialization into
two steps. One such situation is when reusing existing allocations
obtained via `Box::drop_contents`. See [1] for an example.
In order to support this use case add `write_[pin_]init` functions to the
pin-init API. These functions operate on already allocated smart
pointers that wrap `MaybeUninit<T>`.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Link: https://lore.kernel.org/rust-for-linux/f026532f-8594-4f18-9aa5-57ad3f5bc592@proton.me/ [1]
---
 rust/kernel/init.rs    | 84 ++++++++++++++++++++++++++++++------------
 rust/kernel/prelude.rs |  2 +-
 2 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 495c09ebe3a3..1c3cfad89165 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -1154,13 +1154,7 @@ fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>,
     where
         E: From<AllocError>,
     {
-        let mut this = <Box<_> as BoxExt<_>>::new_uninit(flags)?;
-        let slot = this.as_mut_ptr();
-        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
-        // slot is valid and will not be moved, because we pin it later.
-        unsafe { init.__pinned_init(slot)? };
-        // SAFETY: All fields have been initialized.
-        Ok(unsafe { this.assume_init() }.into())
+        <Box<_> as BoxExt<_>>::new_uninit(flags)?.write_pin_init(init)
     }
 
     #[inline]
@@ -1168,13 +1162,7 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
     where
         E: From<AllocError>,
     {
-        let mut this = <Box<_> as BoxExt<_>>::new_uninit(flags)?;
-        let slot = this.as_mut_ptr();
-        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
-        // slot is valid.
-        unsafe { init.__init(slot)? };
-        // SAFETY: All fields have been initialized.
-        Ok(unsafe { this.assume_init() })
+        <Box<_> as BoxExt<_>>::new_uninit(flags)?.write_init(init)
     }
 }
 
@@ -1184,13 +1172,7 @@ fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>,
     where
         E: From<AllocError>,
     {
-        let mut this = UniqueArc::new_uninit(flags)?;
-        let slot = this.as_mut_ptr();
-        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
-        // slot is valid and will not be moved, because we pin it later.
-        unsafe { init.__pinned_init(slot)? };
-        // SAFETY: All fields have been initialized.
-        Ok(unsafe { this.assume_init() }.into())
+        UniqueArc::new_uninit(flags)?.write_pin_init(init)
     }
 
     #[inline]
@@ -1198,13 +1180,67 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
     where
         E: From<AllocError>,
     {
-        let mut this = UniqueArc::new_uninit(flags)?;
-        let slot = this.as_mut_ptr();
+        UniqueArc::new_uninit(flags)?.write_init(init)
+    }
+}
+
+/// Smart pointer containing uninitialized memory and that can write a value.
+pub trait InPlaceWrite<T> {
+    /// The type `Self` turns into when the contents are initialized.
+    type Initialized;
+
+    /// Use the given initializer to write a value into `self`.
+    ///
+    /// Does not drop the current value and considers it as uninitialized memory.
+    fn write_init<E>(self, init: impl Init<T, E>) -> Result<Self::Initialized, E>;
+
+    /// Use the given pin-initializer to write a value into `self`.
+    ///
+    /// Does not drop the current value and considers it as uninitialized memory.
+    fn write_pin_init<E>(self, init: impl PinInit<T, E>) -> Result<Pin<Self::Initialized>, E>;
+}
+
+impl<T> InPlaceWrite<T> for Box<MaybeUninit<T>> {
+    type Initialized = Box<T>;
+
+    fn write_init<E>(mut self, init: impl Init<T, E>) -> Result<Self::Initialized, E> {
+        let slot = self.as_mut_ptr();
         // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
         // slot is valid.
         unsafe { init.__init(slot)? };
         // SAFETY: All fields have been initialized.
-        Ok(unsafe { this.assume_init() })
+        Ok(unsafe { self.assume_init() })
+    }
+
+    fn write_pin_init<E>(mut self, init: impl PinInit<T, E>) -> Result<Pin<Self::Initialized>, E> {
+        let slot = self.as_mut_ptr();
+        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
+        // slot is valid and will not be moved, because we pin it later.
+        unsafe { init.__pinned_init(slot)? };
+        // SAFETY: All fields have been initialized.
+        Ok(unsafe { self.assume_init() }.into())
+    }
+}
+
+impl<T> InPlaceWrite<T> for UniqueArc<MaybeUninit<T>> {
+    type Initialized = UniqueArc<T>;
+
+    fn write_init<E>(mut self, init: impl Init<T, E>) -> Result<Self::Initialized, E> {
+        let slot = self.as_mut_ptr();
+        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
+        // slot is valid.
+        unsafe { init.__init(slot)? };
+        // SAFETY: All fields have been initialized.
+        Ok(unsafe { self.assume_init() })
+    }
+
+    fn write_pin_init<E>(mut self, init: impl PinInit<T, E>) -> Result<Pin<Self::Initialized>, E> {
+        let slot = self.as_mut_ptr();
+        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
+        // slot is valid and will not be moved, because we pin it later.
+        unsafe { init.__pinned_init(slot)? };
+        // SAFETY: All fields have been initialized.
+        Ok(unsafe { self.assume_init() }.into())
     }
 }
 
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index b37a0b3180fb..4571daec0961 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -37,6 +37,6 @@
 
 pub use super::{str::CStr, ThisModule};
 
-pub use super::init::{InPlaceInit, Init, PinInit};
+pub use super::init::{InPlaceInit, InPlaceWrite, Init, PinInit};
 
 pub use super::current;
-- 
2.45.2

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Benno Lossin]

On 03.08.24 16:16, Benno Lossin wrote:
> @@ -53,4 +69,12 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
>          // zero-sized types, we use `NonNull::dangling`.
>          Ok(unsafe { Box::from_raw(ptr) })
>      }
> +
> +    fn drop_contents(this: Self) -> Box<MaybeUninit<T>> {
> +        let ptr = Box::into_raw(this);
> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> +        unsafe { ptr::drop_in_place(ptr) };
> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.

I just noticed that I missed another comment from Boqun here. Got
confused with the two mails. I would replace the comment above with

    // CAST: `T` and `MaybeUninit<T>` have the same layout.
    let ptr = ptr.cast::<MaybeUninit<T>>();
    // SAFETY: `ptr` is valid for writes, because it came from `Box::into_raw` and it is valid for
    // reads, since the pointer came from `Box::into_raw` and the type is `MaybeUninit<T>`.

Let me know if you want another version.

---
Cheers,
Benno

> +        unsafe { Box::from_raw(ptr.cast()) }
> +    }
>  }
> --
> 2.45.2
> 
> 
> 

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Boqun Feng]

On Sat, Aug 03, 2024 at 02:23:42PM +0000, Benno Lossin wrote:
> On 03.08.24 16:16, Benno Lossin wrote:
> > @@ -53,4 +69,12 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
> >          // zero-sized types, we use `NonNull::dangling`.
> >          Ok(unsafe { Box::from_raw(ptr) })
> >      }
> > +
> > +    fn drop_contents(this: Self) -> Box<MaybeUninit<T>> {
> > +        let ptr = Box::into_raw(this);
> > +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> > +        unsafe { ptr::drop_in_place(ptr) };
> > +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> 
> I just noticed that I missed another comment from Boqun here. Got
> confused with the two mails. I would replace the comment above with
> 
>     // CAST: `T` and `MaybeUninit<T>` have the same layout.
>     let ptr = ptr.cast::<MaybeUninit<T>>();
>     // SAFETY: `ptr` is valid for writes, because it came from `Box::into_raw` and it is valid for
>     // reads, since the pointer came from `Box::into_raw` and the type is `MaybeUninit<T>`.
> 
> Let me know if you want another version.

Looks good to me, please do send an updated version.

Although, I would expect the "CAST" comment already explains that if
`ptr` is a valid, then the casting result is also valid, i.e. we put
"CAST" comments on the casting that matters to safety. But that seems
not matching what you use CAST for?

Regards,
Boqun

> 
> ---
> Cheers,
> Benno
> 
> > +        unsafe { Box::from_raw(ptr.cast()) }
> > +    }
> >  }
> > --
> > 2.45.2
> > 
> > 
> > 
> 

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Benno Lossin]

On 03.08.24 17:11, Boqun Feng wrote:
> On Sat, Aug 03, 2024 at 02:23:42PM +0000, Benno Lossin wrote:
>> On 03.08.24 16:16, Benno Lossin wrote:
>>> @@ -53,4 +69,12 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
>>>          // zero-sized types, we use `NonNull::dangling`.
>>>          Ok(unsafe { Box::from_raw(ptr) })
>>>      }
>>> +
>>> +    fn drop_contents(this: Self) -> Box<MaybeUninit<T>> {
>>> +        let ptr = Box::into_raw(this);
>>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>> +        unsafe { ptr::drop_in_place(ptr) };
>>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>
>> I just noticed that I missed another comment from Boqun here. Got
>> confused with the two mails. I would replace the comment above with
>>
>>     // CAST: `T` and `MaybeUninit<T>` have the same layout.
>>     let ptr = ptr.cast::<MaybeUninit<T>>();
>>     // SAFETY: `ptr` is valid for writes, because it came from `Box::into_raw` and it is valid for
>>     // reads, since the pointer came from `Box::into_raw` and the type is `MaybeUninit<T>`.
>>
>> Let me know if you want another version.
> 
> Looks good to me, please do send an updated version.
> 
> Although, I would expect the "CAST" comment already explains that if
> `ptr` is a valid, then the casting result is also valid, i.e. we put
> "CAST" comments on the casting that matters to safety. But that seems
> not matching what you use CAST for?

Well the pointer is no longer valid for reads, since the value has been
dropped. Only through the cast, it becomes again read-valid.

CAST comments must justify why the layouts are the same. On that note,
this comment might be better:

    // CAST: `MaybeUninit<T>` is a transparent wrapper of `T`.

---
Cheers,
Benno

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Boqun Feng]

On Sat, Aug 03, 2024 at 03:32:06PM +0000, Benno Lossin wrote:
> On 03.08.24 17:11, Boqun Feng wrote:
> > On Sat, Aug 03, 2024 at 02:23:42PM +0000, Benno Lossin wrote:
> >> On 03.08.24 16:16, Benno Lossin wrote:
> >>> @@ -53,4 +69,12 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
> >>>          // zero-sized types, we use `NonNull::dangling`.
> >>>          Ok(unsafe { Box::from_raw(ptr) })
> >>>      }
> >>> +
> >>> +    fn drop_contents(this: Self) -> Box<MaybeUninit<T>> {
> >>> +        let ptr = Box::into_raw(this);
> >>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> >>> +        unsafe { ptr::drop_in_place(ptr) };
> >>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> >>
> >> I just noticed that I missed another comment from Boqun here. Got
> >> confused with the two mails. I would replace the comment above with
> >>
> >>     // CAST: `T` and `MaybeUninit<T>` have the same layout.
> >>     let ptr = ptr.cast::<MaybeUninit<T>>();
> >>     // SAFETY: `ptr` is valid for writes, because it came from `Box::into_raw` and it is valid for
> >>     // reads, since the pointer came from `Box::into_raw` and the type is `MaybeUninit<T>`.
> >>
> >> Let me know if you want another version.
> > 
> > Looks good to me, please do send an updated version.
> > 
> > Although, I would expect the "CAST" comment already explains that if
> > `ptr` is a valid, then the casting result is also valid, i.e. we put
> > "CAST" comments on the casting that matters to safety. But that seems
> > not matching what you use CAST for?
> 
> Well the pointer is no longer valid for reads, since the value has been
> dropped. Only through the cast, it becomes again read-valid.
> 

Fair enough, the past Boqun who made that suggestion might also realise
this and that's why he brought this up ;-)

> CAST comments must justify why the layouts are the same. On that note,
> this comment might be better:
> 
>     // CAST: `MaybeUninit<T>` is a transparent wrapper of `T`.
> 

Looks good to me.

Regards,
Boqun

> ---
> Cheers,
> Benno
> 

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Danilo Krummrich]

On 8/3/24 4:16 PM, Benno Lossin wrote:
> Sometimes (see [1]) it is necessary to drop the value inside of a
> `Box<T>`, but retain the allocation. For example to reuse the allocation
> in the future.
> Introduce a new function `drop_contents` that turns a `Box<T>` into
> `Box<MaybeUninit<T>>` by dropping the value.

Is this (and the stuff in patch 2) used somewhere? Otherwise, I think it
would probably make sense to base this work on top of my allocator work.

> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> Link: https://lore.kernel.org/rust-for-linux/20240418-b4-rbtree-v3-5-323e134390ce@google.com/ [1]
> ---
>   rust/kernel/alloc/box_ext.rs | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
> index 829cb1c1cf9e..076d5de5f47d 100644
> --- a/rust/kernel/alloc/box_ext.rs
> +++ b/rust/kernel/alloc/box_ext.rs
> @@ -4,7 +4,7 @@
>   
>   use super::{AllocError, Flags};
>   use alloc::boxed::Box;
> -use core::mem::MaybeUninit;
> +use core::{mem::MaybeUninit, ptr, result::Result};
>   
>   /// Extensions to [`Box`].
>   pub trait BoxExt<T>: Sized {
> @@ -17,6 +17,22 @@ pub trait BoxExt<T>: Sized {
>       ///
>       /// The allocation may fail, in which case an error is returned.
>       fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> +
> +    /// Drops the contents, but keeps the allocation.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::alloc::{flags, box_ext::BoxExt};
> +    /// let value = Box::new([0; 32], flags::GFP_KERNEL)?;
> +    /// assert_eq!(*value, [0; 32]);
> +    /// let value = Box::drop_contents(value);
> +    /// // Now we can re-use `value`:
> +    /// let value = Box::write(value, [1; 32]);
> +    /// assert_eq!(*value, [1; 32]);
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    fn drop_contents(this: Self) -> Box<MaybeUninit<T>>;
>   }
>   
>   impl<T> BoxExt<T> for Box<T> {
> @@ -53,4 +69,12 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
>           // zero-sized types, we use `NonNull::dangling`.
>           Ok(unsafe { Box::from_raw(ptr) })
>       }
> +
> +    fn drop_contents(this: Self) -> Box<MaybeUninit<T>> {
> +        let ptr = Box::into_raw(this);
> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> +        unsafe { ptr::drop_in_place(ptr) };
> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> +        unsafe { Box::from_raw(ptr.cast()) }
> +    }
>   }

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Alice Ryhl]

On Sun, Aug 4, 2024 at 1:22 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On 8/3/24 4:16 PM, Benno Lossin wrote:
> > Sometimes (see [1]) it is necessary to drop the value inside of a
> > `Box<T>`, but retain the allocation. For example to reuse the allocation
> > in the future.
> > Introduce a new function `drop_contents` that turns a `Box<T>` into
> > `Box<MaybeUninit<T>>` by dropping the value.
>
> Is this (and the stuff in patch 2) used somewhere? Otherwise, I think it
> would probably make sense to base this work on top of my allocator work.

Yes, please see the red/black tree.
https://lore.kernel.org/rust-for-linux/20240727-b4-rbtree-v8-0-951600ada434@google.com/

Alice

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Danilo Krummrich]

On Mon, Aug 05, 2024 at 10:43:57AM +0200, Alice Ryhl wrote:
> On Sun, Aug 4, 2024 at 1:22 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On 8/3/24 4:16 PM, Benno Lossin wrote:
> > > Sometimes (see [1]) it is necessary to drop the value inside of a
> > > `Box<T>`, but retain the allocation. For example to reuse the allocation
> > > in the future.
> > > Introduce a new function `drop_contents` that turns a `Box<T>` into
> > > `Box<MaybeUninit<T>>` by dropping the value.
> >
> > Is this (and the stuff in patch 2) used somewhere? Otherwise, I think it
> > would probably make sense to base this work on top of my allocator work.
> 
> Yes, please see the red/black tree.
> https://lore.kernel.org/rust-for-linux/20240727-b4-rbtree-v8-0-951600ada434@google.com/

Ok, unless preferred otherwise, I'll pick it up in the corresponding patches of
my allocator series then.

> 
> Alice
> 

Re: [PATCH v3 1/2] rust: kernel: add `drop_contents` to `BoxExt`

[Benno Lossin]

On 05.08.24 11:07, Danilo Krummrich wrote:
> On Mon, Aug 05, 2024 at 10:43:57AM +0200, Alice Ryhl wrote:
>> On Sun, Aug 4, 2024 at 1:22 AM Danilo Krummrich <dakr@kernel.org> wrote:
>>>
>>> On 8/3/24 4:16 PM, Benno Lossin wrote:
>>>> Sometimes (see [1]) it is necessary to drop the value inside of a
>>>> `Box<T>`, but retain the allocation. For example to reuse the allocation
>>>> in the future.
>>>> Introduce a new function `drop_contents` that turns a `Box<T>` into
>>>> `Box<MaybeUninit<T>>` by dropping the value.
>>>
>>> Is this (and the stuff in patch 2) used somewhere? Otherwise, I think it
>>> would probably make sense to base this work on top of my allocator work.
>>
>> Yes, please see the red/black tree.
>> https://lore.kernel.org/rust-for-linux/20240727-b4-rbtree-v8-0-951600ada434@google.com/
> 
> Ok, unless preferred otherwise, I'll pick it up in the corresponding patches of
> my allocator series then.

I am not sure if we want to do that, since the allocator series is
already rather long and the red-black tree has been in review since
February...
Another way of fixing this would be if you base your allocator series on
top of this one, then you can just include it when you remove the
`BoxExit` trait.

---
Cheers,
Benno