Message-ID: <CAGSQo02S3z2oyaXkJdg6FjMOFL=0DZVK-gg=JrNOPS5TehkF-A@mail.gmail.com>
Date: Wed, 14 May 2025 15:14:09 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Benno Lossin <lossin@...nel.org>, 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>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "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
Subject: Re: [PATCH v5 4/4] rust: samples: Add debugfs sample

On Wed, May 14, 2025 at 4:24 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
> > On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > >> > +impl kernel::Module for RustDebugFs {
> > >> > +    fn init(_this: &'static ThisModule) -> Result<Self> {
> > >> > +        // Create a debugfs directory in the root of the filesystem called "sample_debugfs".
> > >> > +        let debugfs = Dir::new(c_str!("sample_debugfs"));
> > >> > +
> > >> > +        {
> > >> > +            // Create a subdirectory, so "sample_debugfs/subdir" now exists.
> > >> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > >> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > >> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> > >>
> > >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > >> support of the wrapped type. But in this case, `Dir` is sometimes
> > >> intended to not be dropped, so I'd rather have a `.keep()` function I
> > >> saw mentioned somewhere.
> > >
> > > I agree, if we really want to "officially" support to forget() (sub-)directories
> > > and files in order to rely on the recursive cleanup of the "root" directory, it
> > > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > > have some kind of keep() and / or forget() method, to make it clear that this is
> > > supported and by design and won't lead to any leaks.
> > >
> > > Consequently, this would mean that we'd need something like your proposed const
> > > generic on the Dir type, such that keep() / forget() cannot be called on the
> > > "root" directory.
> > >
> > > However, I really think we should keep the code as it is in this version and
> > > just don't provide an example that utilizes ManuallyDrop and forget().
> > >
> > > I don't see how the idea of "manually dropping" (sub-)directories and files
> > > provides any real value compared to just storing their instance in a driver
> > > structure as long as they should stay alive, which is much more intuitive
> > > anyways.
> >
> > Yeah that's whats normally done in Rust anyways. But I think that
> > lifetimes bite us in this case:
> >
> >     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
> >
> >     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
> >     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
> >
> >     /* code for creating the file etc */
> >
> >     Ok(Self { _debugfs: debugfs, _sub: sub })
> >     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
> >
> > This code won't compile, since we can't store the "root" dir in the same
> > struct that we want to store the subdir, because the subdir borrows from
> > the root dir.
> >
> > Essentially this would require self-referential structs like the
> > `ouroboros` crate [1] from user-space Rust. We should rather have the
> > `.keep()` function in the API than use self-referential structs.
>
> Fair enough -- I think we should properly document those limitations, recommend
> using keep() for those cases and ensure that we can't call keep() on the "root"
> directory then.
>
> Unless, we can find a better solution, which, unfortunately, I can't think of
> one. The only thing I can think of is to reference count (parent) directories,
> which would be contrary to how the C API works and not desirable.
>
> > [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
> >
> > Another problem that only affects complicated debugfs structures is that
> > you would have to store all subdirs & files somewhere. If the structure
> > is dynamic and changes over the lifetime of the driver, then you'll need
> > a `Vec` or store the dirs in `Arc` or similar, leading to extra
> > allocations.
>
> If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem.
>
> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
>
>         root.subdir("foo").keep()
>
> effectively are leaks.
>
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
>
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).

Yes, this is one of the things I don't currently have a good safe
solution for without introducing something with similar power to
`self_cell` or a bespoke type implemented unsafely like the `Tree` I
mentioned earlier in the chain.

>
> >
> > > It either just adds complexity to the API (due to the need to distingish between
> > > the "root" directory and sub-directories) or makes the API error prone by
> > > providing a *valid looking* option to users to leak the "root" directory.
> >
> > I agree with this, I want that `ManuallyDrop` & `forget` are only used
> > rarely mostly for low-level operations.
> >
> > ---
> > Cheers,
> > Benno

Powered by blists - more mailing lists