Message-ID: <CAH5fLgjaNzOHNxa+XY1c2V5A1H2RhWP9gHAAmHx=9LN9CbHq=Q@mail.gmail.com>
Date: Thu, 3 Jul 2025 12:54:18 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Danilo Krummrich <dakr@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	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>, Trevor Gross <tmgross@...ch.edu>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Sami Tolvanen <samitolvanen@...gle.com>, 
	Timur Tabi <ttabi@...dia.com>, 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 Thu, Jul 3, 2025 at 12:33 PM Benno Lossin <lossin@...nel.org> wrote:
>
> 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?

I would store a bunch of `File<Arc<Process>>` instances somewhere.
Each one has a closure that takes the spinlock and prints the
appropriate value.

Alice

Powered by blists - more mailing lists