Message-ID: <aGzrZqIrStGD_UBp@mango>
Date: Tue, 08 Jul 2025 09:56:57 +0000
From: Oliver Mangold <oliver.mangold@...me>
To: Benno Lossin <lossin@...nel.org>
Cc: 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>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Asahi Lina <lina+kernel@...hilina.net>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 1/4] rust: types: Add Ownable/Owned types

On 250707 1123, Benno Lossin wrote:
> On Mon Jul 7, 2025 at 8:58 AM CEST, Oliver Mangold wrote:
> > On 250702 1303, Benno Lossin wrote:
> >> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> >> > From: Asahi Lina <lina+kernel@...hilina.net>
> >> >
> >> > By analogy to `AlwaysRefCounted` and `ARef`, an `Ownable` type is a
> >> > (typically C FFI) type that *may* be owned by Rust, but need not be. Unlike
> >> > `AlwaysRefCounted`, this mechanism expects the reference to be unique
> >> > within Rust, and does not allow cloning.
> >> >
> >> > Conceptually, this is similar to a `KBox<T>`, except that it delegates
> >> > resource management to the `T` instead of using a generic allocator.
> >> >
> >> > Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> >> > Signed-off-by: Asahi Lina <lina@...hilina.net>
> >> > [ om:
> >> >   - split code into separate file and `pub use` it from types.rs
> >> >   - make from_raw() and into_raw() public
> >> >   - fixes to documentation and commit message
> >> > ]
> >> > Signed-off-by: Oliver Mangold <oliver.mangold@...me>
> >> > Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> >> > ---
> >> >  rust/kernel/types.rs         |   7 +++
> >> >  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
> >>
> >> I think we should name this file `owned.rs` instead. It's also what
> >> we'll have for `ARef` when that is moved to `sync/`.
> >>
> >> Also, I do wonder does this really belong into the `types` module? I
> >> feel like it's becoming our `utils` module and while it does fit, I
> >> think we should just make this a top level module. So
> >> `rust/kernel/owned.rs`. Thoughts?
> >
> > I don't have much of an opinion on on that. But maybe refactoring types.rs
> > should be an independent task?
> 
> But you're adding the new file there? Just add it under
> `rust/kernel/owned.rs` instead.

To be honest, I don't really mind.

Note, though, that I already moved it from types.rs to types/ownable.rs on
request. It seems to me different people here have different ideas where it
should be placed. I feel now, that it would make sense to come to an
agreement between the interested parties about where it should finally be
placed, before I move it again. Could I ask that we settle that question
once and for all before my next revision?

> >> > +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> >> > +///   (i.e. most kernel types).
> >>
> >> Can't we implicitly pin `Owned`?
> >
> > I have been thinking about that. But this would mean that the blanket
> > implementation for `Deref` would conflict with the one for `OwnableMut`.
> 
> Yeah we could not implement `DerefMut` in that case (or only for `T:
> Unpin`).
> 
> >> > +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> >> > +///   the usual rules) while Rust owns it.
> >> > +pub unsafe trait OwnableMut: Ownable {}
> >> > +
> >> > +/// An owned reference to an ownable kernel object.
> >>
> >> How about
> >>
> >>     An owned `T`.
> >
> >     A wrapper around `T`.
> >
> > maybe?
> 
> "wrapper" seems wrong, since a wrapper in my mind is a newtype. So it
> would be `struct Owned(T)` which is wrong. The `T` is stored elsewhere.
> 
> >> > +///
> >> > +/// The object is automatically freed or released when an instance of [`Owned`] is
> >> > +/// dropped.
> >>
> >> I don't think we need to say this, I always assume this for all Rust
> >> types except they document otherwise (eg `ManuallyDrop`, `MaybeUninit`
> >> and thus also `Opaque`.)
> >
> > Hmm, it is an important feature of the wrapper that it turns the `*Ownable`
> > into an object that is automatically dropped. So shouldn't that be
> > mentioned here?
> 
> I would expect that that happens, but sure we can leave it here.
> 
> >> How about we provide some examples here?
> >>
> >> > +///
> >> > +/// # Invariants
> >> > +///
> >> > +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
> >>
> >> What exactly is "owned" supposed to mean? It depends on the concrete `T`
> >> and that isn't well-defined (since it's a generic)...
> >
> > "owned" means that access to the `T` is exclusive through the `Owned<T>`,
> > so normal Rust semantics can be applied.
> 
> Okay, in that case just say that `ptr` has exclusive access.

Or, ehm, sorry, I forgot, ownership also implies that the allocation of the
underlying resource/object is now under the responsibility of the owner,
i.e. the owner should free it at the appropriate time.

In short, just the standard meaning of ownership in Rust.

https://doc.rust-lang.org/book/ch04-01-what-is-ownership.html

> >> Maybe we should give `Ownable` the task to document the exact ownership
> >> semantics of `T`?
> >>
> >> > +pub struct Owned<T: Ownable> {
> >> > +    ptr: NonNull<T>,
> >> > +    _p: PhantomData<T>,
> >> > +}
> >> > +
> >> > +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> >> > +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
> >>
> >> How does this amount to sending a `&mut T`?
> >
> > Good point. That documentation hasn't been updated since `&mut T` access
> > has been split out into `OwnableMut`. Not sure how to phrase it now.
> 
> I'm also unsure, also for the correct trait bounds on this impl.
> 
> ---
> Cheers,
> Benno
> 
> >> I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
> >> list grows...
> >>
> >> I'll try to come up with something to simplify this design a bit wrt the
> >> safety docs.
> >>
> >> > +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> >> > +
> >> > +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> >> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
> >>
> >> Same here.
> >>
> >> > +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}

Powered by blists - more mailing lists