[PATCH v2] rust: Implement the smart pointer `InPlaceInit` for `Arc`

[PATCH v2] rust: Implement the smart pointer `InPlaceInit` for `Arc`

[Alex Mantel]

For pinned and unpinned initialization of structs, a trait named
`InPlaceInit` exists for uniform access. `Arc` did not implement
`InPlaceInit` yet, although the functions already existed. The main
reason for that, was that the trait itself returned a `Pin<Self>`. The
`Arc` implementation of the kernel is already implicitly pinned.

To enable `Arc` to implement `InPlaceInit` and to have uniform access,
for in-place and pinned in-place initialization, an associated type is
introduced for `InPlaceInit`. The new implementation of `InPlaceInit`
for `Arc` sets `Arc` as the associated type. Older implementations use
an explicit `Pin<T>` as the associated type. The implemented methods for
`Arc` are mostly moved from a direct implementation on `Arc`. There
should be no user impact. The implementation for `ListArc` is omitted,
because it is not merged yet.

Link: https://github.com/Rust-for-Linux/linux/issues/1079
Signed-off-by: Alex Mantel <alexmantel93@mailbox.org>
---
Hello again!

This is the 2nd version of my very first patch. In comparison to the
first version, this version changes the format of the commit
message only, as suggested by Miguel Ojeda. Thank you for taking the
time. Any further feedback is more than welcome!

v1: 
  * https://lore.kernel.org/rust-for-linux/20240717034801.262343-2-alexmantel93@mailbox.org/

v2: 
  * remove the `From:` from the patch.
  * add the prefix `rust: ` to the subject.
  * Remove the empty line between `Link` and `Signed-off-by`.


 rust/kernel/init.rs     | 37 +++++++++++++++++++++++++++++++++----
 rust/kernel/sync/arc.rs | 25 ++-----------------------
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 68605b633..46f50cf12 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -213,6 +213,7 @@
 use crate::{
     alloc::{box_ext::BoxExt, AllocError, Flags},
     error::{self, Error},
+    sync::Arc,
     sync::UniqueArc,
     types::{Opaque, ScopeGuard},
 };
@@ -1112,11 +1113,15 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
 
 /// Smart pointer that can initialize memory in-place.
 pub trait InPlaceInit<T>: Sized {
+    /// A type might be pinned implicitly. An addtional `Pin<ImplicitlyPinned>` is useless. In
+    /// doubt, the type can just be set to `Pin<Self>`.
+    type PinnedResult;
+
     /// Use the given pin-initializer to pin-initialize a `T` inside of a new smart pointer of this
     /// type.
     ///
     /// If `T: !Unpin` it will not be able to move afterwards.
-    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
+    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self::PinnedResult, E>
     where
         E: From<AllocError>;
 
@@ -1124,7 +1129,7 @@ fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>,
     /// type.
     ///
     /// If `T: !Unpin` it will not be able to move afterwards.
-    fn pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> error::Result<Pin<Self>>
+    fn pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> error::Result<Self::PinnedResult>
     where
         Error: From<E>,
     {
@@ -1153,9 +1158,31 @@ fn init<E>(init: impl Init<T, E>, flags: Flags) -> error::Result<Self>
     }
 }
 
+impl<T> InPlaceInit<T> for Arc<T> {
+    type PinnedResult = Self;
+
+    #[inline]
+    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self::PinnedResult, E>
+    where
+        E: From<AllocError>,
+    {
+        UniqueArc::try_pin_init(init, flags).map(|u| u.into())
+    }
+
+    #[inline]
+    fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
+    where
+        E: From<AllocError>,
+    {
+        UniqueArc::try_init(init, flags).map(|u| u.into())
+    }
+}
+
 impl<T> InPlaceInit<T> for Box<T> {
+    type PinnedResult = Pin<Self>;
+
     #[inline]
-    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
+    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self::PinnedResult, E>
     where
         E: From<AllocError>,
     {
@@ -1184,8 +1211,10 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
 }
 
 impl<T> InPlaceInit<T> for UniqueArc<T> {
+    type PinnedResult = Pin<Self>;
+
     #[inline]
-    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
+    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self::PinnedResult, E>
     where
         E: From<AllocError>,
     {
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3673496c2..3021f30fd 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -12,12 +12,13 @@
 //! 2. It does not support weak references, which allows it to be half the size.
 //! 3. It saturates the reference count instead of aborting when it goes over a threshold.
 //! 4. It does not provide a `get_mut` method, so the ref counted object is pinned.
+//! 5. The object in [`Arc`] is pinned implicitly.
 //!
 //! [`Arc`]: https://doc.rust-lang.org/std/sync/struct.Arc.html
 
 use crate::{
     alloc::{box_ext::BoxExt, AllocError, Flags},
-    error::{self, Error},
+    bindings,
     init::{self, InPlaceInit, Init, PinInit},
     try_init,
     types::{ForeignOwnable, Opaque},
@@ -209,28 +210,6 @@ pub fn new(contents: T, flags: Flags) -> Result<Self, AllocError> {
         // `Arc` object.
         Ok(unsafe { Self::from_inner(Box::leak(inner).into()) })
     }
-
-    /// Use the given initializer to in-place initialize a `T`.
-    ///
-    /// If `T: !Unpin` it will not be able to move afterwards.
-    #[inline]
-    pub fn pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> error::Result<Self>
-    where
-        Error: From<E>,
-    {
-        UniqueArc::pin_init(init, flags).map(|u| u.into())
-    }
-
-    /// Use the given initializer to in-place initialize a `T`.
-    ///
-    /// This is equivalent to [`Arc<T>::pin_init`], since an [`Arc`] is always pinned.
-    #[inline]
-    pub fn init<E>(init: impl Init<T, E>, flags: Flags) -> error::Result<Self>
-    where
-        Error: From<E>,
-    {
-        UniqueArc::init(init, flags).map(|u| u.into())
-    }
 }
 
 impl<T: ?Sized> Arc<T> {
-- 
2.39.2

Re: [PATCH v2] rust: Implement the smart pointer `InPlaceInit` for `Arc`

[Alice Ryhl]

On Fri, Jul 19, 2024 at 9:22 PM Alex Mantel <alexmantel93@mailbox.org> wrote:
>
> For pinned and unpinned initialization of structs, a trait named
> `InPlaceInit` exists for uniform access. `Arc` did not implement
> `InPlaceInit` yet, although the functions already existed. The main
> reason for that, was that the trait itself returned a `Pin<Self>`. The
> `Arc` implementation of the kernel is already implicitly pinned.
>
> To enable `Arc` to implement `InPlaceInit` and to have uniform access,
> for in-place and pinned in-place initialization, an associated type is
> introduced for `InPlaceInit`. The new implementation of `InPlaceInit`
> for `Arc` sets `Arc` as the associated type. Older implementations use
> an explicit `Pin<T>` as the associated type. The implemented methods for
> `Arc` are mostly moved from a direct implementation on `Arc`. There
> should be no user impact. The implementation for `ListArc` is omitted,
> because it is not merged yet.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1079
> Signed-off-by: Alex Mantel <alexmantel93@mailbox.org>
> [...]
>  /// Smart pointer that can initialize memory in-place.
>  pub trait InPlaceInit<T>: Sized {
> +    /// A type might be pinned implicitly. An addtional `Pin<ImplicitlyPinned>` is useless. In
> +    /// doubt, the type can just be set to `Pin<Self>`.
> +    type PinnedResult;
> +

It's unfortunate that we can't use an associated type default here.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Alice

Re: [PATCH v2] rust: Implement the smart pointer `InPlaceInit` for `Arc`

[Valentin Obst]

[snip]

> @@ -1112,11 +1113,15 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>
>  /// Smart pointer that can initialize memory in-place.
>  pub trait InPlaceInit<T>: Sized {
> +    /// A type might be pinned implicitly. An addtional `Pin<ImplicitlyPinned>` is useless. In
> +    /// doubt, the type can just be set to `Pin<Self>`.
> +    type PinnedResult;
> +

[snip]

nit: Typo 's/addtional/additional'

Apart from that, I think "When in doubt, ..." or "If in doubt, ..." are
more common choices than "In doubt, ...". But that's a question of style so
feel free to ignore this comment.

Rest looks good to me.

	Best
	Valentin

Re: [PATCH v2] rust: Implement the smart pointer `InPlaceInit` for `Arc`

[Benno Lossin]

On 19.07.24 21:22, Alex Mantel wrote:
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 68605b633..46f50cf12 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -213,6 +213,7 @@
>  use crate::{
>      alloc::{box_ext::BoxExt, AllocError, Flags},
>      error::{self, Error},
> +    sync::Arc,
>      sync::UniqueArc,
>      types::{Opaque, ScopeGuard},
>  };
> @@ -1112,11 +1113,15 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
> 
>  /// Smart pointer that can initialize memory in-place.
>  pub trait InPlaceInit<T>: Sized {
> +    /// A type might be pinned implicitly. An addtional `Pin<ImplicitlyPinned>` is useless. In
> +    /// doubt, the type can just be set to `Pin<Self>`.

This comment should better describe the purpose of this associated type,
the first line could be "Pinned version of `Self`" then (with an empty
line in between) you could write more explanatory stuff. I would also
rephrase what you have above, for example: "If a type already implicitly
pins its pointee, `Pin<Self>` is unnecessary. In this case use `Self`,
otherwise just use `Pin<Self>`.".

> +    type PinnedResult;

I don't really like the name for this, since it is not a result. What do
you think of `PinnedSelf`?

Otherwise this looks good!

---
Cheers,
Benno

> +
>      /// Use the given pin-initializer to pin-initialize a `T` inside of a new smart pointer of this
>      /// type.
>      ///
>      /// If `T: !Unpin` it will not be able to move afterwards.
> -    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
> +    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self::PinnedResult, E>
>      where
>          E: From<AllocError>;
> 

Re: [PATCH v2] rust: Implement the smart pointer `InPlaceInit` for `Arc`

[Alex Mantel]

Submitted v3:

https://lore.kernel.org/rust-for-linux/20240727042442.682109-1-alexmantel93@mailbox.org/

Thanks for the feedback!

On 25.07.24 11:06, Benno Lossin wrote:
> On 19.07.24 21:22, Alex Mantel wrote:
>> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
>> index 68605b633..46f50cf12 100644
>> --- a/rust/kernel/init.rs
>> +++ b/rust/kernel/init.rs
>> @@ -213,6 +213,7 @@
>>   use crate::{
>>       alloc::{box_ext::BoxExt, AllocError, Flags},
>>       error::{self, Error},
>> +    sync::Arc,
>>       sync::UniqueArc,
>>       types::{Opaque, ScopeGuard},
>>   };
>> @@ -1112,11 +1113,15 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>>
>>   /// Smart pointer that can initialize memory in-place.
>>   pub trait InPlaceInit<T>: Sized {
>> +    /// A type might be pinned implicitly. An addtional `Pin<ImplicitlyPinned>` is useless. In
>> +    /// doubt, the type can just be set to `Pin<Self>`.
> 
> This comment should better describe the purpose of this associated type,
> the first line could be "Pinned version of `Self`" then (with an empty
> line in between) you could write more explanatory stuff. I would also
> rephrase what you have above, for example: "If a type already implicitly
> pins its pointee, `Pin<Self>` is unnecessary. In this case use `Self`,
> otherwise just use `Pin<Self>`.".
> 
>> +    type PinnedResult;
> 
> I don't really like the name for this, since it is not a result. What do
> you think of `PinnedSelf`?
> 
> Otherwise this looks good!
> 
> ---
> Cheers,
> Benno
> 
>> +
>>       /// Use the given pin-initializer to pin-initialize a `T` inside of a new smart pointer of this
>>       /// type.
>>       ///
>>       /// If `T: !Unpin` it will not be able to move afterwards.
>> -    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
>> +    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self::PinnedResult, E>
>>       where
>>           E: From<AllocError>;
>>
> 
>