Message-ID: <2025070131-icon-quarters-0c16@gregkh>
Date: Tue, 1 Jul 2025 15:58:45 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Matthew Maurer <mmaurer@...gle.com>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Timur Tabi <ttabi@...dia.com>, Benno Lossin <lossin@...nel.org>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	Dirk Behme <dirk.behme@...bosch.com>
Subject: Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing
 for File

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@...nel.org> wrote:
> > >
> > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@...nel.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...

So a file callback IS going to want to have access to the data of type T
that is exposed somehow.

And debugfs is NOT for performance things, but really, how bad could it
be?  :)

thanks,

greg k-h

Powered by blists - more mailing lists