Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

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

On Tue Jul 1, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
> On 7/1/25 9:46 PM, Benno Lossin wrote:
>> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
>>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
>>>> On Tue, Jul 1, 2025 at 8:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>          impl Firmware {
>>>>>             pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
>>>>>                pin_init!(Self {
>>>>>                   minor <- dir.create_file("minor", 1),
>>>>>                   major <- dir.create_file("major", 2),
>>>>>                   buffer <- dir.create_file("buffer", buffer),
>>>>>                })
>>>>>             }
>>>>>          }
>>>>>
>>>>>          // This is the only allocation we need.
>>>>>          let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>>>>>
>>>>> With this everything is now in a single allocation and since we're using
>>>>> pin-init, Dir::create_file() can safely store pointers of the corresponding data
>>>>> in debugfs_create_file(), since this structure is guaranteed to be pinned in
>>>>> memory.
>>>>>
>>>>> Actually, we can also implement *only this*, since with this my previous example
>>>>> would just become this:
>>>>
>>>> If we implement *only* pinned files, we run into an additional problem
>>>> - you can't easily extend a pinned vector. This means that you cannot
>>>> have dynamically created devices unless you're willing to put every
>>>> new `File` into its own `Box`, because you aren't allowed to move any
>>>> of the previously allocated `File`s for a resize.
>>>>
>>>> Where previously you would have had
>>>>
>>>> ```
>>>> debug_files: Vec<File>
>>>> ```
>>>>
>>>> you would now have
>>>>
>>>> ```
>>>> debug_files: Vec<PinBox<File<T>>>
>>>> ```
>>>
>>> Stuffing single File instances into a Vec seems like the wrong thing to do.
>>>
>>> Instead you may have instances of some data structure that is created
>>> dynamically in your driver that you want to expose through debugfs.
>>>
>>> Let's say you have (userspace) clients that can be registered arbitrarily, then
>>> you want a Vec<Client>, which contains the client instances. In order to provide
>>> information about the Client in debugfs you then have the client embed things as
>>> discussed above.
>>>
>>> 	struct Client {
>>> 	   id: File<ClientId>,
>>> 	   data: File<ClientData>,
>>> 	   ...
>>> 	}
>>>
>>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
>>> Vec<File> separately. Also, note that with the above, your Client instances
>>> don't need to be reference counted anymore.
>>>
>>> I think this addresses the concerns below.
>> 
>> You still have the issue that `Client` now needs to be pinned and the
>> vector can't be resized. But if you know that it's bounded, then we
>> could just make `Pin<Vec<T>>` work as expected (not relocating the
>> underlying allocation by not exposing `push`, only
>> `push_within_capacity`).
>> 
>> We also could have a `SegmentedVec<T>` that doesn't move elements.
>> Essentially it is
>>      
>>      enum SegmentedVec<T> {
>>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
>>          Nul,
>>      }
>> 
>>      struct Segment<T> {
>>          elements: [T; 16]
>>      }
>> 
>> or make the segments variable-sized and grow them accordingly.
>
> That sounds a lot like the perfect application for XArray. :)

Haha I didn't know this already existed in the kernel :) Yeah then we
should make XArray work for this use-case.

---
Cheers,
Benno