| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4297a3f3028bd463ed5de38568e87d8ccf82aaa6.camel@posteo.de>
Date: Tue, 14 Oct 2025 14:15:19 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Benno Lossin <lossin@...nel.org>, Danilo Krummrich <dakr@...nel.org>,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Lee
Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Vlastimil Babka
<vbabka@...e.cz>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, Uladzislau
Rezki <urezki@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
<gary@...yguo.net>, bjorn3_gh@...tonmail.com, Andreas Hindborg
<a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross
<tmgross@...ch.edu>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v4 1/2] rust: add basic Pin<Vec<T, A>> abstractions
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
Powered by blists - more mailing lists