Re: [PATCH v4 1/2] rust: add basic Pin<Vec<T, A>> abstractions

["Benno Lossin" <lossin@kernel.org>]

On Mon Oct 13, 2025 at 3:43 PM CEST, Markus Probst wrote:
> On Mon, 2025-10-13 at 10:03 +0200, Benno Lossin wrote:
>> On Mon Oct 13, 2025 at 12:11 AM CEST, Markus Probst wrote:
>> > On Sun, 2025-10-12 at 23:31 +0200, Benno Lossin wrote:
>> > > On Sun Oct 12, 2025 at 6:57 PM CEST, Markus Probst wrote:
>> > > > From what I can tell, there is no way to get a `Pin<&mut Vec<T,
>> > > > A>>`
>> > > > from a `&mut Pin<Vec<T, A>>`. We can only get `Pin<&mut [T]>`
>> > > > which
>> > > > is
>> > > > not usable in our case.
>> > > 
>> > > Hmm yeah that's true.
>> > > 
>> > > > If there is way, without the extension trait or an extra
>> > > > struct, I
>> > > > would be happy to implement it.
>> > > 
>> > > So I tried to look for the usage site of this and I found this
>> > > usage
>> > > in
>> > > your v1:
>> > > 
>> > >     +        let mut leds = KPinnedVec::with_capacity(
>> > >     +            Atmega1608LedAddress::VALUES.len() *
>> > > Atmega1608LedId::VALUES.len(),
>> > >     +            GFP_KERNEL,
>> > >     +        )?;
>> > >     +
>> > >     +        let mut i = 0;
>> > >     +        for addr in Atmega1608LedAddress::VALUES {
>> > >     +            let mode_lock = Arc::pin_init(new_mutex!(()),
>> > > GFP_KERNEL)?;
>> > >     +
>> > >     +            for id in Atmega1608LedId::VALUES {
>> > >     +                let Some(child) =
>> > >     +                   
>> > > fwnode.get_child_by_name(&CString::try_from_fmt(fmt!("led@{i}"))?
>> > > )
>> > >     +                else {
>> > >     +                    continue;
>> > >     +                };
>> > >     +
>> > >     +                let client = ARef::clone(&client);
>> > >     +                let mode_lock = Arc::clone(&mode_lock);
>> > >     +
>> > >     +                leds.push_pin_init(LedClassDev::new(
>> > >     +                    Some(idev),
>> > >     +                    None,
>> > >     +                    LedInitData::new().fwnode(&child),
>> > >     +                    Atmega1608Led {
>> > >     +                        addr,
>> > >     +                        id,
>> > >     +                        client,
>> > >     +
>> > >     +                        mode_lock,
>> > >     +                    },
>> > >     +                ))?;
>> > >     +                i += 1;
>> > >     +            }
>> > >     +        }
>> > >     +        Ok(KBox::new(Self { client, leds },
>> > > GFP_KERNEL)?.into())
>> > > 
>> > > And I think using `Vec` for this is just wrong. `Vec` is a data
>> > > structure that supports growing and shrinking the allocation. But
>> > > you
>> > > just need a fixed size buffer that holds all your data. Do you
>> > > think
>> > > that `Pin<Box<[LedClassDev]>>` would suffice if it had proper
>> > > support
>> > > from pin-init?
>> > As you can see in v1, the number of leds (or vec entries) depends
>> > on
>> > the fwnode (see the continue statement there). I don't think that
>> > counts as fixed size. `Pin<KBox<[Option<LedClassDev>]>>` could
>> > potentially be used instead of `Pin<KVec<LedClassDev>>` in my
>> > scenario,
>> > but that would require an extra byte of allocation for the max leds
>> > of
>> > 24 each and the code would look more ugly. At the point I use
>> > Option in
>> > the slice, its basically an unoptimized Vec (instead of storing the
>> > length, it stores if an item in the buffer is present or not).
>> 
>> You can just make the length of the slice be the desired length?
> That would work, but creates another allocation on the heap (Vec<I>)
> that could have been avoided. I don't think it would make `Pin<Vec<T,
> A>>` obsolete.
>
> Or would you rather say, such allocations don't matter?

No, but you're already allocating once per inner loop invocation, the
`CString::try_from_fmt` function allocates :)

I don't know the kind of application that you're writing, does
performance matter? If yes, then just run your benchmark suite on both
versions and look at the difference. If you don't have a benchmark
suite, then perf probably isn't important enough.

Also if you really want to avoid the allocation, then you probably could
first query the length and store only that in a local var and then
create the initializers on-demand. But then again to query that you're
creating a string every loop iteration, which allocates :)

>> (also,
>> `i` is never incremented in the `continue` case, so it will act like
>> a
>> `break`?)
> You just found a bug in v1.

:)

> Thanks
> - Markus Probst
>
> [1] https://docs.rs/arrayvec/latest/arrayvec/struct.ArrayString.html

Did you forget to put a reference to this?

---
Cheers,
Benno