Message-ID: <Zmrkg7oOceWWV4nz@pollux>
Date: Thu, 13 Jun 2024 14:22:27 +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 Thu, Jun 13, 2024 at 07:47:13AM +0200, Greg KH wrote:
> On Wed, Jun 12, 2024 at 10:56:43PM +0200, Danilo Krummrich wrote:
> > > > 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.
> > > 
> > > You really have two things here, a matrix of:
> > > 	can transfer between threads
> > > 	can call in irq context
> > > that are independent and not related to each other at all.
> > > 
> > > Looks like Rust has built in support for the first.  And nothing for the
> > > second as that is a very kernel-specific thing.
> > 
> > The language documentation defines `Send` as "can be transferred between
> > threads", likely because it's written from a userspace perspective. But in
> > the kernel context it actually means can be transferred between any context,
> > thread, IRQ, etc.
> > 
> > If this isn't true, then we have to add a comment what is allowed (e.g. any
> > non-atomic context) and what's not allowed.
> 
> That isn't good, you are going to have to change how `Send` works here
> if you think it's safe to do stuff in all of those different contexts,
> as that is almost never going to be true.
> 
> Why not just stick with "accessed from another thread" and leave it at
> that, as again, the combinations here are a matrix, not a boolean yes/no
> type of thing.

I probably didn't phrase this very well, let me try again.

What the compiler can do for us currently is to check whether a data structure
is kept only in it's current context or whether it is send to another one - it
can't distinguish context types though.

So, the actual definition of `Send` isn't really "can be send across threads",
but "is not restricted to be kept in a single context".

This means that if something is "is not restricted to be kept in a single
context", but limited to certain context type, we, unfortunately, just have to
make it `Send`, but document the restriction.

> 
> > > So let's not confuse the two please.  `Send` and `Sync` should be fine
> > > for a device pointer to be passed around, as long as the reference is
> > > incremented, as that's what all of the kernel C code does today.  Let's
> > > not worry about irq context at all, that's independent and can be
> > > handled at a later time, if at all, with a different "marking" as it's
> > > independent of the current two things.
> > 
> > That'd be great, but as mentioned above, we only have `Send`, but nothing like
> > `SendIrq`, hence `Send` really means any context.
> > 
> > Given your proposal, to just say it's fine to pass between (actual) threads and
> > ignore IRQ context for now, we have to implement `Send`, but document that IRQ
> > context is not covered.
> > 
> > We can either do that in the Rust abstraction as safety requirement, or we can,
> > as proposed previously, add a comment to the C `struct device` documentation
> > that implementers of release() must *at least* make sure that it can be called
> > from any non-atomic context. We can then refer to that.
> 
> As someone who has written comments in code for functions that are
> always ignored, I am happy to see you think that this is actually going
> to do anything :)

One advantage we have in Rust is, that if something has a specific requirement
we mark it as `unsafe` and the user of the API has to put it in an `unsafe`
block, which we require a sfety comment for, where the user of the API has to
explain why this is the correct thing to do in this case.

In other words we can technically enforce that people read the comment and
explain how they ensure to fulfill what the comment asks for.

> 
> Heck, I have put huge messages in kernel code for when people implement
> the api wrong[1], and they still ignore that at runtime.  Only way to get
> it noticed is if you break someone's build.

You only see the ones who still do it wrong. You probably don't have visibility
of the ones who did it right due to your effort to write it down. :-)

> 
> So you all need to really define what `Send` means, as it sounds like a
> userspace thing that isn't going to fit in well within the kernel model.

Please see the explanation above.

> 
> thanks,
> 
> greg k-h
> 
> [1] See the pr_debug() calls in kobject_cleanup() as proof, people just
>     create an "empty" release function to shut them up, thinking that is
>     the correct solution when obviously it is not...
> 

Powered by blists - more mailing lists