Message-ID: <aGzmoMWvy1v7ayyn@mango>
Date: Tue, 08 Jul 2025 09:36:38 +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 4/4] rust: Add `OwnableRefCounted`

On 250707 1133, Benno Lossin wrote:
> On Mon Jul 7, 2025 at 10:07 AM CEST, Oliver Mangold wrote:
> > On 250702 1524, Benno Lossin wrote:
> >> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> >> > @@ -132,3 +134,124 @@ fn drop(&mut self) {
> >> >          unsafe { T::release(self.ptr) };
> >> >      }
> >> >  }
> >> > +
> >> > +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
> >> > +/// [`ARef`].
> >> > +///
> >> > +/// # Safety
> >> > +///
> >> > +/// Implementers must ensure that:
> >> > +///
> >> > +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
> >> > +///   exactly one [`ARef<Self>`] exists.
> >>
> >> This shouldn't be required?
> >
> > Ehm, why not? `Owned<T>` is supposed to be unique.
> 
> It's not needed as a safety requirement for implementing the trait. If
> the implementation only contains sound code, then `Owned::from_raw`
> should already ensure that `Owned<Self>` is only created if there is
> exactly one reference to it.

Okay, got it now. I guess you are right, it is not strictly needed. If the
requirement should be removed, not sure, though. Isn't it error-prone if it
explicitly stated here (again) that it is required?

> >> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
> >> > +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
> >> > +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
> >> > +///   implementation, which just rewraps the underlying object, the reference count needs not to be
> >> > +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
> >>
> >> This also seems pretty weird...
> >>
> >> I feel like `OwnableRefCounted` is essentially just a compatibility
> >> condition between `Ownable` and `RefCounted`. It ensures that the
> >> ownership declared in `Ownable` corresponds to exactly one refcount
> >> declared in `RefCounted`.
> >>
> >> That being said, I think a `RefCounted` *always* canonically is
> >> `Ownable` by the following impl:
> >>
> >>     unsafe impl<T: RefCounted> Ownable for T {
> >>         unsafe fn release(this: NonNull<Self>) {
> >>             T::dec_ref(this)
> >>         }
> >>     }
> >>
> >> So I don't think that we need this trait at all?
> >
> > No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
> > `try_from_shared()` implementation. It is not even a given that the
> > function can implemented, if all the kernel exposes are some kind of
> > `inc_ref()` and `dec_ref()`.
> 
> I don't understand this paragraph.

What I mean is, to convert from an `ARef` to an `Owned`, it is necessary to
check that there is only one reference. The API of the underlying object
might not provide that.

About the above documentation, it is a bit convoluted, because I had the
case of `mq::Request` in mind, where the refcount needs to be changed
during conversion.

> > Also there are more complicated cases like with `Mq::Request`, where the
> > existence of an `Owned<T>` cannot be represented by the same refcount value
> > as the existence of exactly one `ARef<T>`.
> 
> Ah right, I forgot about this. What was the refcount characteristics of
> this again?
> 
> *  1 = in flight, owned by C
> *  2 = in flight, owned by Rust
> * >2 = in flight, owned by Rust + additional references used by Rust
>        code
> 
> Correct? Maybe @Andreas can check.
> 
> >> > +///
> >> > +/// # Examples
> >>
> >> If we're having an example here, then we should also have on on `Owned`.
> >
> > Yes, maybe. I mostly felt the need to create one for `OwnableRefCounted`
> > because it is a more complex idea than `Ownable`.
> >
> > If I remember correctly, I didn't create one for `Owned`, as it should
> > probably more or less the same as for `ARef` and the one there has even
> > more problems of the kind you are pointing out. So maybe it would be best
> > to wait until someone fixes that and have the fixed version copied over to
> > `Owned` in the process?
> 
> Wait which problems on `ARef` do you mean? I disagree that `Owned` and
> `ARef` have the same example. `Owned` should expose operations that
> `ARef` can't otherwise there would be no value in using `Owned`.

I mean it uses a `struct Empty {}`, which doesn't do any refcounting and
the safety requirements of `ARef::from_raw()` are also not fulfilled.

The point of `Owned` is not that it provides more operations than `ARef`
but rather that it provides less. The reference cannot be cloned. Actually
it is not supposed to provide much features at all, except for freeing the
underlying object when it is dropped.

It is supposed to just be a safe wrapper around a `*T`, marking that the
reference is Owned/Unique. Safe functions defined elsewhere can then take a
`Owned<T>` or `&mut Owned<T>` which provides the assurance of
ownership/uniqueness.

> >> > +///
> >> > +/// A minimal example implementation of [`OwnableRefCounted`], [`Ownable`] and its usage with
> >> > +/// [`ARef`] and [`Owned`] looks like this:
> >> > +///
> >> > +/// ```
> >> > +/// # #![expect(clippy::disallowed_names)]
> >> > +/// use core::cell::Cell;
> >> > +/// use core::ptr::NonNull;
> >> > +/// use kernel::alloc::{flags, kbox::KBox, AllocError};
> >> > +/// use kernel::types::{
> >> > +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
> >> > +/// };
> >> > +///
> >> > +/// struct Foo {
> >> > +///     refcount: Cell<usize>,
> >> > +/// }
> >> > +///
> >> > +/// impl Foo {
> >> > +///     fn new() -> Result<Owned<Self>, AllocError> {
> >> > +///         // Use a `KBox` to handle the actual allocation.
> >> > +///         let result = KBox::new(
> >> > +///             Foo {
> >> > +///                 refcount: Cell::new(1),
> >> > +///             },
> >> > +///             flags::GFP_KERNEL,
> >> > +///         )?;
> >> > +///         let result = NonNull::new(KBox::into_raw(result))
> >> > +///             .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> >>
> >> I'm not really convinced that an example using `KBox` is a good one...
> >> Maybe we should just have a local invisible `bindings` module that
> >> exposes a `-> *mut foo`. (internally it can just create a KBox`)
> >
> > The example would become quite a bit more complicated then, no?
> 
> Just hide those parts behind `#` lines in the example.

I don't know about this method. Can you give an example on how that works?

> ---
> Cheers,
> Benno

Powered by blists - more mailing lists