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

[Benno Lossin]

On Wed Mar 5, 2025 at 3:29 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>> On Wed Mar 5, 2025 at 1:22 PM CET, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@xxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>>  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.

Yes, but I wouldn't necessarily call this category "vendored"; e.g. we
could write a crate that is kernel-only, but doesn't actually have any
code that requires kernel infrastructure. How about we call these
kernel-agnostic crates? :)

>>>> +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.

I could change the link depending on the `kernel` cfg, so

    #![cfg_attr(kernel, doc = "[`Arc<T>`]: https://rust.docs.kernel.org...";)]
    #![cfg_attr(not(kernel), doc = "[`Arc<T>`]: https://doc.rust-lang.org...";)]

But if anyone visits the documentation on `docs.rs`, then they will get
the user-space one... What do you think?

>>>>  //! [`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.

I will see if I can come up with a solution.

---
Cheers,
Benno