Re: [PATCH v2 1/2] rust: add abstraction for struct device

[Greg KH <gregkh@linuxfoundation.org>]

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.

> > 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 :)

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.

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.

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...