Message-ID: <aGZ3q0PEmZ7lV4I-@pollux>
Date: Thu, 3 Jul 2025 14:29:31 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Alice Ryhl <aliceryhl@...gle.com>, Benno Lossin <lossin@...nel.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 03, 2025 at 01:41:53PM +0200, Greg Kroah-Hartman wrote:
> Yes, we need to be able to have a debugfs file callback handle a mutable
> structure in order to lock things correctly.  We also need to have it be
> mutable so that it can MODIFY the value (everyone seems to forget that
> debugfs allows that...)

Well, that's possible with both approaches. Data behind a lock becomes mutable
once you grabbed the lock. That's the same in both cases.

The difference is that with the pin-init approach I propose you can't have what
Alice sketched up. And I think it's even desirable that you can't do it.

Let's me explain the difference on a simplified example, based on Alice'
example.

ForeignOwnable API
------------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: SpinLock<ProcessInner>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here we have to create an Arc<Process> (let's call it process) and create files
from it.

	let file_threads = dir.create_file("threads", process);
	let file_max_threads = dir.create_file("max_threads", process);

In the file system callback of both of these, we now have an Arc<Process>, hence
we can access:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

and in the other file:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

Pin-Init API
------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: File<SpinLock<ProcessInner>>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here Process does not need to be within an Arc and no separate file instances
need to be kept around, that happens already within the constructor of Process:

	pin_init!(Process {
	   inner <- dir.create_file("process_inner", ...),
	   [...]
	})

The file itself has a reference to SpinLock<ProcessInner>, hence we can access:

	let guard = inner.lock();

	read_or_write(guard.threads)
	read_or_write(guard.max_threads)

The difference is that with the ForeignOwnable API it was possible to have
separate files for threads and max_threads.

While with the pin-init one we either have to have a single file exposing
ProcessInner (which is what I did above) or protect threads and max_threads
with separate locks (of course max_threads could also just be an atomic).

(If you like I can sketch up this case as well.)

At a first glance this seems like an undesirable limitation, but I argue that
this is a good thing.

The reason I think so is what I also explained in [1], but let me adjust it a
bit for this reply:

threads and max_threads being protected by the same lock means that they are in
a certain relationship to each other. Meaning that they only really make sense
looking at them atomically.

So I argue it does not make sense to expose those values to userspace through
separate files.

For instance:

	$ cat max_threads && cat threads
	$ 5
	$ 10

This way you may read 5 max_threads, but 10 actual threads, because things may
have changed in between the two cat commands.

However, if instead, they are exposed through a single file, we get an atomic
view of them, such that the semantic relationship between them is preserved.

For instance:

	$ cat process_info
	$ threads: 2
	$ max_threads: 10

So, what I'm trying to say is, I think it's a good thing if fields that are
protected by the same lock can't be exposed through separate files, because it
means that the fields only make sense in the context of each other.

Or saying it the other way around, if it makes sense to expose fields through
separate files, it means they're unrelated to each other and hence should be
protected with separate locks, rather than a common one.

IMHO it's even a good thing beyond the scope of debugfs, because it forces
developers to really think about organizing structures properly, e.g. in a way
that only fields that really belong behind a certain lock are placed behind this
lock.

> So how about a platform driver that exposes values read from a platform
> device (i.e. a soc info driver), that also includes a
> local-to-the-device data structure that can be locked and modified?
> That should cover all the use cases that I can think of at the moment.

Yes, I also really like to have that.

But, again, both approaches can do this. It's just that I really discourage the
one that forces us to have an Arc instance on structures exposed through
debugfs, since this messes with the driver's lifetime and ownership
architecture in a bad way.

Powered by blists - more mailing lists