Message-Id: <DB6PU07CUFZK.9JEG3423922O@kernel.org>
Date: Tue, 08 Jul 2025 15:42:11 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Oliver Mangold" <oliver.mangold@...me>
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 Tue Jul 8, 2025 at 11:36 AM CEST, Oliver Mangold wrote:
> 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?

You can move it to the normal docs of the trait, but it shouldn't be a
safety requirement.

>> >> > +/// - [`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.

So I think I got a bit confused with my initial question. We do need
this trait in order to enable the `ARef -> Owned` conversion. But the
other way is going to be fine if we choose to add the blanket impl
above.

Now the question is do we want `T: RefCounted` to imply `T: Ownable`? I
think the answer is yes, but it sadly conflicts with `AlwaysRefCounted`,
since if `T: AlwaysRefCounted` it must not implement `Ownable`.

So to me it seems this point has become moot.

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

Right.

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

Both our statements are true at the same time: `Owned<T>` itself has
less operations available compared to `ARef`, but `Owned<Specific>` has
potentially more operations than `ARef<Speceific>`. So eg in the
`Request` case, one can only call `end_ok` if you have an `Owned` one.

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

Well it should also have compatibility with `RefCounted` if it
implements `OwnableRefCounted`.

>> >> > +///
>> >> > +/// 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?

See the second code block in [1].

[1]: https://doc.rust-lang.org/rustdoc/write-documentation/what-to-include.html#examples

---
Cheers,
Benno

Powered by blists - more mailing lists