Re: [PATCH] rust: alloc: implement Box::pin_slice()

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Aug 8, 2025 at 5:07 PM CEST, Vitaly Wool wrote:
> From: Alice Ryhl <aliceryhl@google.com>
>
> Add a new constructor to Box to facilitate Box creation from a pinned
> slice of elements. This allows to efficiently allocate memory for e.g.
> slices of structrures containing spinlocks or mutexes. Such slices may
> be used in kmemcache like or zpool API implementations.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
> ---
>
> This patch supersedes "rust: extend kbox with a new constructor" posted
> a day earlier.

It is still the same patch, just with the review feedback applied, hence it
should be a normal v2. I assume the change of commit subject got you confused on
what's the correct thing to do process wise. :)

I have a few more comments below, when you send a new version, please declare it
as v3.

>  rust/kernel/alloc/kbox.rs | 61 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index 1fef9beb57c8..f0be307f7242 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -290,6 +290,67 @@ pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
>          Ok(Self::new(x, flags)?.into())
>      }
>  
> +    /// Construct a pinned slice of elements `Pin<Box<[T], A>>`.
> +    ///
> +    /// This is a convenient means for creation of e.g. slices of structrures containing spinlocks
> +    /// or mutexes.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// #[pin_data]
> +    /// struct Example {
> +    ///     c: u32,
> +    ///     #[pin]
> +    ///     d: SpinLock<Inner>,

I did not compile the patch (yet), but I'm pretty sure this can't work; I don't
see a definition of Inner anywhere.

Do you test with CONFIG_RUST_KERNEL_DOCTESTS=y? If not, you should enable this
config.

> +    /// }
> +    ///
> +    /// impl Example {
> +    ///     fn new() -> impl PinInit<Self> {
> +    ///         pin_init!(Self {
> +    ///             c: 10,
> +    ///             d <- new_spinlock!(Inner { a: 20, b: 30 }),
> +    ///         })
> +    ///     }
> +    /// }
> +    /// // Allocate a boxed slice of 10 `Example`s.
> +    /// let s = KBox::pin_slice(
> +    ///     | _i | Example::new(),
> +    ///     10,
> +    ///     GFP_KERNEL
> +    /// )?;
> +    /// assert_eq!(s[5].c, 10);
> +    /// assert_eq!(s[3].d.lock().a, 20),
> +    /// ```

I think this is missing `# Ok::<(), Error>(())`.

> +    pub fn pin_slice<Func, Item, E>(
> +        mut init: Func,
> +        len: usize,
> +        flags: Flags,
> +    ) -> Result<Pin<Box<[T], A>>, E>
> +    where
> +        Func: FnMut(usize) -> Item,
> +        Item: PinInit<T, E>,
> +        E: From<AllocError>,
> +    {
> +        let mut buffer = super::Vec::<T, A>::with_capacity(len, flags)?;
> +        for i in 0..len {
> +            let ptr = buffer.spare_capacity_mut().as_mut_ptr().cast();
> +            // SAFETY:
> +            // - `ptr` is a valid pointer to uninitialized memory.
> +            // - `ptr` is not used if an error is returned.
> +            // - `ptr` won't be moved until it is dropped, i.e. it is pinned.
> +            unsafe { init(i).__pinned_init(ptr)? };

NIT: Please add an empty line here.

> +            // SAFETY:
> +            // - `i + 1 <= len` => we don't exceed the capacity

Please make this a sentence, e.g. "`i + 1 <= len`, hence we don't exceed the
capacity due to the call to with_capacity() above."

> +            // - this new value is initialized

Same here, please make this a full sentence.

Also, it is unclear what "this new value" refers to. I know what you mean, but
it would be better to be more clear about it, e.g. "With the above call to
`init(i).__pinned_init(ptr)` we just initialized the element at index
`buffer.len() + 1`.

> +            unsafe { buffer.inc_len(1) };
> +        }
> +        let (ptr, _, _) = buffer.into_raw_parts();
> +        let slice = core::ptr::slice_from_raw_parts_mut(ptr, len);
> +        // SAFETY: `slice` is not a NULL pointer because it is a valid pointer to [T]

Please have a look at the safety requirements of Box::from_raw() and try to come
up with a sentence that explains why the pointer you pass in fulfills the safety
requirements.

> +        Ok(Pin::from(unsafe { Box::from_raw(slice) }))
> +    }
> +
>      /// Convert a [`Box<T,A>`] to a [`Pin<Box<T,A>>`]. If `T` does not implement
>      /// [`Unpin`], then `x` will be pinned in memory and can't be moved.
>      pub fn into_pin(this: Self) -> Pin<Self> {
> -- 
> 2.39.2