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

[Danilo Krummrich <dakr@kernel.org>]

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:
> > On Tue, Jul 01, 2025 at 03:58:45PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> > > > On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > > > > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > >
> > > > > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > >>
> > > > > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > > > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > > > > >>> +    where
> > > > > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > > > > >>> +    {
> > > > > > >>> +        File {
> > > > > > >>> +            _foreign: ForeignHolder::new(data),
> > > > > > >>> +        }
> > > > > > >>>        }
> > > > > > >>
> > > > > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > > > > >> File<D> and store data directly?
> > > > > > >
> > > > > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > > > > unless all your files contain the *same* backing type.
> > > > > >
> > > > > > That sounds reasonable.
> > > > > >
> > > > > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > > > > `File` is different depending on the backing type, making it
> > > > > > > polymorphic is just needlessly confusing.
> > > > > >
> > > > > > What if I want to access file.data() and do something with the data? Then I'd
> > > > > > necessarily need to put my data in an Arc and reference count it to still be
> > > > > > able to access it.
> > > > > >
> > > > > > That doesn't seem like a reasonable requirement to be able to access data
> > > > > > exposed via debugfs.
> > > > > 
> > > > > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > > > > request for DebugFS files to not really support anything other than
> > > > > delete. I was even considering making `D` not be retained in the
> > > > > disabled debugfs case, but left it in for now for so that the
> > > > > lifecycles wouldn't change.
> > > > 
> > > > Well, that's because the C side does not have anything else. But the C side has
> > > > no type system that deals with ownership:
> > > > 
> > > > In C you just stuff a pointer of your private data into debugfs_create_file()
> > > > without any implication of ownership. debugfs has a pointer, the driver has a
> > > > pointer. The question of the ownership semantics is not answered by the API, but
> > > > by the implementation of the driver.
> > > > 
> > > > The Rust API is different, and it's even implied by the name of the trait you
> > > > expect the data to implement: ForeignOwnable.
> > > > 
> > > > The File *owns* the data, either entirely or a reference count of the data.
> > > > 
> > > > If the *only* way to access the data the File now owns is by making it reference
> > > > counted, it:
> > > > 
> > > >   1) Is additional overhead imposed on users.
> > > > 
> > > >   2) It has implications on the ownership design of your driver. Once something
> > > >      is reference counted, you loose the guarantee the something can't out-live
> > > >      some event.
> > > > 
> > > > I don't want that people have to stuff their data structures into Arc (i.e.
> > > > reference count them), even though that's not necessary. It makes it easy to
> > > > make mistakes. Things like:
> > > > 
> > > > 	let foo = bar.clone();
> > > > 
> > > > can easily be missed in reviews, whereas some contributor falsely changing a
> > > > KBox to an Arc is much harder to miss.
> > > > 
> > > > > If you want a `.data()` function, I can add it in,
> > > > 
> > > > I think it could even be an implementation of Deref.
> > > > 
> > > > > but I don't think
> > > > > it'll improve flexibility in most cases. If you want to do something
> > > > > with the data and it's not in an `Arc` / behind a handle of some kind,
> > > > > you'll need something providing threadsafe interior mutability in the
> > > > > data structure. If that's a lock, then I have a hard time believing
> > > > > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > > > > is why I added that in the stack) is so much more expensive than
> > > > > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > > > > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > > > > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > > > > simpler "`File` is just a handle to how long the file stays alive, it
> > > > > doesn't let you do anything else" API makes sense.
> > > > 
> > > > I don't really see what is complicated about File<T> -- it's a File and it owns
> > > > data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> > > > 
> > > > Maybe the performance cost is not a huge argument here, but maintainability in
> > > > terms of clarity about ownership and lifetime of an object as explained above
> > > > clearly is.
> > > 
> > > I'm agreeing here.  As one of the primary users of this api is going to
> > > be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
> > > an example driver to emulate that file with just a local structure, but
> > > the reference counting and access logic just didn't seem to work out
> > > properly.  Odds are I'm doing something stupid though...
> > 
> > I think it technically works, but it imposes semantics on drivers that we
> > shouldn't do; see the example below.
> > 
> > > So a file callback IS going to want to have access to the data of type T
> > > that is exposed somehow.
> > 
> > With the current API we would need this:
> > 
> > 	struct GPU {
> > 	   fw: Arc<Firmware>,
> > 	   fw_file: debugfs::File,
> > 	}
> > 
> > and then I would initialize it the following way:
> > 
> > 	let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
> > 	let fw_file = dir.create_file("firmware", fw.clone());
> > 
> > 	fw.do_something();
> > 
> > This is bad, because now my Firmware instance in GPU needs to be reference
> > counted, even though it should *never* out-live the GPU instance. This is error
> > prone.
> 
> Agreed, AND you just created a new fw structure that you really didn't
> need, wasting memory.

In case you refer to fw.clone(), since fw is an Arc<Firmware>, clone() just
creates a new reference count to the same object.

> > 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.