* Re: [PATCH] rust: alloc: implement Box::pin_slice()
2025-08-08 15:07 [PATCH] rust: alloc: implement Box::pin_slice() Vitaly Wool
@ 2025-08-09 12:26 ` Danilo Krummrich
0 siblings, 0 replies; 2+ messages in thread
From: Danilo Krummrich @ 2025-08-09 12:26 UTC (permalink / raw)
To: Vitaly Wool
Cc: rust-for-linux, linux-kernel, Uladzislau Rezki, Alice Ryhl,
Vlastimil Babka, Lorenzo Stoakes, Liam R . Howlett, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Bjorn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross
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
^ permalink raw reply [flat|nested] 2+ messages in thread