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

[Markus Probst <markus.probst@posteo.de>]

On Mon, 2025-10-13 at 16:47 +0200, Benno Lossin wrote:
> 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.
There is no model that would be able to make use of this driver under 4
GB of RAM, but I assumed code should be as optimized as possible in the
linux kernel. Its only run once at driver initialization, so it doesn't
matter for me at least.
> 
> 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?
I did also notice the `CString::try_from_fmt` while writing the email.
There was once a part in the email which I removed (I would have dealt
with it later), in which I would have asked if it would also make sense
to add an abstraction for fixed-allocated c strings on the stack (as it
currently would also have been an unnecessary allocation). The link was
meant as reference to an existing implementation (which I forgot to
remove).

Thanks
- Markus Probst

> 
> ---
> Cheers,
> Benno