Message-ID: <ZmnKXoBYf0qOcPU4@pollux>
Date: Wed, 12 Jun 2024 18:18:38 +0200
From: Danilo Krummrich <dakr@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Boqun Feng <boqun.feng@...il.com>, rafael@...nel.org, mcgrof@...nel.org,
	russell.h.weight@...el.com, ojeda@...nel.org, alex.gaynor@...il.com,
	wedsonaf@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.com,
	aliceryhl@...gle.com, airlied@...il.com, fujita.tomonori@...il.com,
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] rust: add abstraction for struct device

On Wed, Jun 12, 2024 at 05:50:42PM +0200, Greg KH wrote:
> On Wed, Jun 12, 2024 at 05:35:21PM +0200, Danilo Krummrich wrote:
> > On Wed, Jun 12, 2024 at 05:02:52PM +0200, Greg KH wrote:
> > > On Wed, Jun 12, 2024 at 04:51:42PM +0200, Danilo Krummrich wrote:
> > > > On 6/11/24 18:13, Boqun Feng wrote:
> > > > > On Tue, Jun 11, 2024 at 03:29:22PM +0200, Greg KH wrote:
> > > > > > On Tue, Jun 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > > > ...hence, I agree we should indeed add to the #Invariants and #Safety section
> > > > > > > that `->release` must be callable  from any thread.
> > > > > > > 
> > > > > > > However, this is just theory, do we actually have cases where `device::release`
> > > > > 
> > > > > @Danilo, right, it's only theorical, but it's good to call it out since
> > > > > it's the requirement for a safe Rust abstraction.
> > > > 
> > > > Similar to my previous reply, if we want to call this out as safety requirement
> > > > in `Device::from_raw`, we probably want to add it to the documentation of the C
> > > > `struct device`, such that we can argue that this is an invariant of C's
> > > > `struct device`.
> > > > 
> > > > Otherwise we'd have to write something like:
> > > > 
> > > > "It must also be ensured that the `->release` function of a `struct device` can
> > > > be called from any non-atomic context. While not being officially documented this
> > > > is guaranteed by the invariant of `struct device`."
> > > 
> > > In the 20+ years of the driver model being part of the kernel, I don't
> > > think this has come up yet, so maybe you can call the release function
> > > in irq context.  I don't know, I was just guessing :)
> > 
> > Ah, I see. I thought you know and it's defined, but just not documented.
> > 
> > This means it's simply undefined what we expect to happen when the last
> > reference of a device is dropped from atomic context.
> > 
> > Now, I understand (and would even expect) that practically this has never been
> > an issue. You'd need two circumstances, release() actually does something that
> > is not allowed in atomic context plus the last device reference is dropped from
> > atomic context - rather unlikely.
> > 
> > > 
> > > So let's not go adding constraints that we just do not have please.
> > > Same goes for the C code, so the rust code is no different here.
> > 
> > I agree we shouldn't add random constraints, but for writing safe code we also
> > have to rely on defined behavior.
> 
> As the rust code is relying on C code that could change at any point in
> time, how can that ever be "safe"?  :)

That's the same as with any other API. If the logic of an API is changed the
users (e.g a Rust abstraction) of the API have to be adjusted.

> 
> Sorry, this type of definition annoys me.
> 
> > I see two options:
> > 
> > (1) We globally (for struct device) define from which context release() is
> >     allowed to be called.
> 
> If you want, feel free to do that work please.  And then find out how to
> enforce it in the driver core.

If we *would* define non-atomic context only, we could enforce it with
might_sleep() for instance.

If we *would* define any context, there is nothing to enforce, but we'd need to
validate that no implementer of release() voids that.

The former is a constaint you don't want to add, the latter a lot of work. What
if we at least define that implementers of release() must *minimally* make sure
that it can be call from any non-atomic context.

That'd be something we can rely on in Rust.

> 
> > (2) We define it for the Rust abstraction only and just constrain it to
> >     non-atomic context to be able to give a safety guarantee. We can't say
> >     "might be safe from any context, but we don't know".
> 
> Why can't you say that?  Your "saftey" barrier ends/begins at the
> interaction with the rust/c layer.  You can't "guarantee" anything on
> the C side...

Not guarantee as in "technically enforce it", but guarantee as in "we have rules
that we follow".

The former would be best, but it isn't always possible. The latter we can always
do (and should IMHO).

> 
> > But again, this is really just a formality, the C code does it all the way and
> > practically there never was an issue, which means we actually do follow some
> > rules, it's just about writing them down. :)
> 
> Again, this has NEVER come up in 20+ years, so maybe it's just not an
> issue?  Not to say it isn't, but maybe do some tests and see what
> happens...

Oh, I fully agree with that. Let me try to explain a bit what this is about:

In Rust we have the `Send` and `Sync` marker traits. If a type (e.g. `Device`)
implements `Send` it means that it's safe to pass an instance of this type
between threads. Which is clearly something we want to do with a `Device`.

If I don't implement `Sync` for `Device` the compiler will prevent me from
sending it between threads, e.g. by disallowing me to put an instance of
`Device` into another data structure that is potentially passed between threads.

If I implement `Sync` I have to add a safety comment on why it is safe to pass
`Device` between threads. And here we have what Boqun pointed out: `Device` can
only be passed between threads when we're allowed to drop the last reference
from any thread. In the case of the kernel this can be any non-atomic context,
any context or any other subset. But I have to write something here that is
a defined rule and can be relied on.

- Danilo

> 
> thanks,
> 
> greg k-h
> 

Powered by blists - more mailing lists