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

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

On Thu Jul 3, 2025 at 12:02 PM CEST, Alice Ryhl wrote:
> On Tue, Jul 01, 2025 at 05:10:47PM +0200, Danilo Krummrich wrote:
>> On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
>> > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
>> > > Instead this should just be:
>> > > 
>> > > 	struct GPU {
>> > > 	   fw: debugfs::File<Firmware>,
>> > > 	}
>> > > 
>> > > and then I would initialize it the following way:
>> > > 
>> > > 	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
>> > > 	let file = dir.create_file("firmware", fw);
>> > > 
>> > > 	// debugfs::File<Firmware> dereferences to Firmware
>> > > 	file.do_something();
>> > > 
>> > > 	// Access to fw is prevented by the compiler, since it has been moved
>> > > 	// into file.
>> > > 
>> > > This is much better, since now I have the guarantee that my Firmare instance
>> > > can't out-live the GPU instance.
>> > 
>> > That's better, yes, but how would multiple files for the same
>> > "structure" work here?  Like a debugfs-file-per-field of a structure
>> > that we often have?
>> 
>> That is a very good question and I thought about this as well, because with only
>> the current API this would require us to have more and more dynamic allocations
>> if we want to have a more fine grained filesystem representations of structures.
>> 
>> The idea I have for this is to use pin-init, which we do in quite some other
>> places as well.
>> 
>> I think we can add an additional API like this:
>> 
>> 	impl Dir {
>> 	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
>> 	      pin_init!(Self {
>> 	         data <- data,
>> 	         ...
>> 	      })
>> 	   }
>> 	}
>> 
>> This allows us to do things like:
>> 
>> 	#[pin_data]
>> 	struct Firmware {
>> 	   #[pin]
>> 	   minor: debugfs::File<u32>,
>> 	   #[pin]
>> 	   major: debugfs::File<u32>,
>> 	   #[pin]
>> 	   buffer: debugfs::File<[u8]>,
>> 	}
>> 
>> 	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:
>> 
>> 	struct GPU {
>> 	   fw: debugfs::File<Firmware>,
>> 	}
>> 
>> 	let file = dir.create_file("firmware", Firmware::new());
>> 	let file = KBox::pin_init(file, GFP_KERNEL)?;
>> 
>> 	// debugfs::File<Firmware> dereferences to Firmware
>> 	file.do_something();
>> 
>> Given that, I think we should change things to use pin-init right away for the
>> debugfs::File API.
>
> Does this actually work in practice for anything except immutable data?
> I mean, let's take Rust Binder as an example and lets say that I want to
> expose a directory for each Process object with some of the fields
> exposed. Let's just simplify Rust Binder a bit and only include some of
> the fields:
>
> #[pin_data]
> struct Process {
>     task: ARef<Task>,
>     #[pin]
>     inner: SpinLock<ProcessInner>,
> }
>
> pub(crate) struct ProcessInner {
>     threads: RBTree<i32, Arc<Thread>>,
>     nodes: RBTree<u64, DArc<Node>>,
>     requested_thread_count: u32,
>     max_threads: u32,
>     started_thread_count: u32,
> }
>
> Rust Binder already does expose some debugging data through a file
> system, though it doesn't do so using debugfs. It exposes a lot of data,
> but among them are the pid, the number of threads and nodes, as well as
> the values of requested_thread_count, started_thread_count, and
> max_threads.
>
> Now, we run into problem number one: pinning is not supported inside
> mutexes. But let's say we solved that and we could do this:
>
> #[pin_data]
> struct Process {
>     task: File<ARef<Task>>, // prints the pid
>     #[pin]
>     inner: SpinLock<ProcessInner>,
> }
>
> pub(crate) struct ProcessInner {
>     threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
>     nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
>     requested_thread_count: File<u32>,
>     max_threads: File<u32>,
>     started_thread_count: File<u32>,
> }
>
> However, this still doesn't work! Debugfs may get triggered at any time
> and need to read these fields, and there's no way for it to take the
> spinlock with the above design - it doesn't know where the spinlock is.
> For the integers I guess we could make them atomic to allow reading them
> in parallel with mutation, but that option is not available for the
> red/black trees.
>
> What is the intended solution in this case? If the argument is that this
> is a rare case, then keep in mind that this is a real-world example of
> debugging information that we actually expose today in a real driver.
> With Matt's current approach, it's relatively easy - just store a bunch
> of File<Arc<Process>> instances somewhere and define each one to take
> the mutex and print the relevant value.

How would your example look like with the current approach? IIUC, it
also wouldn't work, because the debugfs data can't be mutated?

---
Cheers,
Benno