Re: [PATCH 16/22] rust: pin-init: add `std` and `alloc` support from the user-space version

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Wed Mar 5, 2025 at 1:22 PM CET, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> To synchronize the kernel's version of pin-init with the user-space
>>> version, introduce support for `std` and `alloc`. While the kernel uses
>>> neither, the user-space version has to support both. Thus include the
>>> required `#[cfg]`s and additional code.
>>>
>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>>> ---
>>>  rust/pin-init/src/__internal.rs |  27 ++++++
>>>  rust/pin-init/src/alloc.rs      | 158 ++++++++++++++++++++++++++++++++
>>>  rust/pin-init/src/lib.rs        |  17 ++--
>>>  3 files changed, 196 insertions(+), 6 deletions(-)
>>>  create mode 100644 rust/pin-init/src/alloc.rs
>>>
>>> diff --git a/rust/pin-init/src/__internal.rs b/rust/pin-init/src/__internal.rs
>>> index 74086365a18a..27d4a8619c04 100644
>>> --- a/rust/pin-init/src/__internal.rs
>>> +++ b/rust/pin-init/src/__internal.rs
>>> @@ -186,6 +186,33 @@ pub fn init<E>(self: Pin<&mut Self>, init: impl PinInit<T, E>) -> Result<Pin<&mu
>>>      }
>>>  }
>>>
>>> +#[test]
>>
>> I think the kunit support we have in the pipeline will pick this up?
>
> Is that support also enabled for crates outside of the `kernel` crate?
> I would argue it shouldn't and then this isn't a problem.

Re conversation about moving pin_init out of the kernel, we should
distinguish between vendored crates and crates that is part of the
kernel. This one is now vendored and tests are not meant to be run by
the kernel build system and infrastructure. Other crates will be
different, living in the kernel.

>
>>> +fn stack_init_reuse() {
>>> +    use ::std::{borrow::ToOwned, println, string::String};
>>> +    use core::pin::pin;
>>> +
>>> +    #[derive(Debug)]
>>> +    struct Foo {
>>> +        a: usize,
>>> +        b: String,
>>> +    }
>>> +    let mut slot: Pin<&mut StackInit<Foo>> = pin!(StackInit::uninit());
>>> +    let value: Result<Pin<&mut Foo>, core::convert::Infallible> =
>>> +        slot.as_mut().init(crate::init!(Foo {
>>> +            a: 42,
>>> +            b: "Hello".to_owned(),
>>> +        }));
>>> +    let value = value.unwrap();
>>> +    println!("{value:?}");
>>> +    let value: Result<Pin<&mut Foo>, core::convert::Infallible> =
>>> +        slot.as_mut().init(crate::init!(Foo {
>>> +            a: 24,
>>> +            b: "world!".to_owned(),
>>> +        }));
>>> +    let value = value.unwrap();
>>> +    println!("{value:?}");
>>> +}
>>> +
>>
>> [...]
>>
>>> diff --git a/rust/pin-init/src/lib.rs b/rust/pin-init/src/lib.rs
>>> index 55d8953620f0..1fdca35906a0 100644
>>> --- a/rust/pin-init/src/lib.rs
>>> +++ b/rust/pin-init/src/lib.rs
>>> @@ -204,8 +204,8 @@
>>>  //! [structurally pinned fields]:
>>>  //!     https://doc.rust-lang.org/std/pin/index.html#pinning-is-structural-for-field
>>>  //! [stack]: crate::stack_pin_init
>>> -//! [`Arc<T>`]: ../kernel/sync/struct.Arc.html
>>> -//! [`Box<T>`]: ../kernel/alloc/struct.KBox.html
>>> +//! [`Arc<T>`]: https://doc.rust-lang.org/stable/alloc/sync/struct.Arc.html
>>> +//! [`Box<T>`]: https://doc.rust-lang.org/stable/alloc/boxed/struct.Box.html
>>
>> Now these will render incorrect in the kernel docs, right?
>
> What do you mean by that? The link will resolve to the std versions of
> `Arc` and `Box`. But that is also what this crate will support, as it
> doesn't know about the kernel's own alloc.

I mean that if I render the kernel documentation, go to `pin_init` and
click the `Arc<T>` link, I end up in `std`. But I am in the kernel, so
not what I would expect.

But I guess there is no easy solution? Being a kernel developer, I would
prefer a kernel first approach. Can't have it all, I guess.

>
>>>  //! [`impl PinInit<Foo>`]: PinInit
>>>  //! [`impl PinInit<T, E>`]: PinInit
>>>  //! [`impl Init<T, E>`]: Init
>>> @@ -239,6 +239,11 @@
>>>  #[doc(hidden)]
>>>  pub mod macros;
>>>
>>> +#[cfg(any(feature = "std", feature = "alloc"))]
>>> +mod alloc;
>>> +#[cfg(any(feature = "std", feature = "alloc"))]
>>> +pub use alloc::InPlaceInit;
>>
>> Do we really need to have this entire file sitting dead in the kernel
>> tree? If you are not building the user space version from the kernel
>> sources, I don't think we need it here. Even when you want to sync
>> between the two repositories, it should be easy to handle an entire file
>> being not present on one side.
>
> I do have a script that does the commit porting, you can find it at [1].
> So I could easily add that file there. However, I think it also is
> important that it's easy to remember which files are synchronized and
> which files aren't. At the moment it's very simple, the synchronized
> files are:
> - README.md
> - CONTRIBUTING.md
> - src/*
> - internal/src/*
> - examples/*
>
> If I introduce special cases for files in src, I fear that I might get
> confused at some point, making a change that shouldn't be done etc.
>
> I understand your worry about the dead file, but at the same time, I
> think it's vital to keep the pattern of synchronized files as simple as
> possible.

I don't agree about this one - but I am not the one that has to do the
work. I would prefer we don't keep dead user space code in the kernel
tree, and I would ask that you consider if you can find a solution for
that which works for you. If not, I will live with the dead code.


Best regards,
Andreas Hindborg