Types implementing one of these traits can safely convert between an
`ARef<T>` and an `Owned<T>`.

This is useful for types which generally are accessed through an `ARef`
but have methods which can only safely be called when the reference is
unique, like e.g. `block::mq::Request::end_ok()`.

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/types.rs         |  10 +++-
 rust/kernel/types/ownable.rs | 127 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 134 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index a22d5d39d89edfe357fdfe903ef42e5d5404237f..af0bda4ea50f54aed3c51327db0e43f960868501 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -12,7 +12,7 @@
 use pin_init::{PinInit, Zeroable};
 
 pub mod ownable;
-pub use ownable::{Ownable, OwnableMut, Owned};
+pub use ownable::{Ownable, OwnableMut, OwnableRefCounted, Owned};
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
@@ -429,6 +429,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
 /// Note: Implementing this trait allows types to be wrapped in an [`ARef<Self>`]. It requires an
 /// internal reference count and provides only shared references. If unique references are required
 /// [`Ownable`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
+/// Implementing the trait [`OwnableRefCounted`] allows to convert between unique and shared
+/// references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
 ///
 /// # Safety
 ///
@@ -572,6 +574,12 @@ fn from(b: &T) -> Self {
     }
 }
 
+impl<T: OwnableRefCounted> From<Owned<T>> for ARef<T> {
+    fn from(b: Owned<T>) -> Self {
+        T::into_shared(b)
+    }
+}
+
 impl<T: RefCounted> Drop for ARef<T> {
     fn drop(&mut self) {
         // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
--- a/rust/kernel/types/ownable.rs
+++ b/rust/kernel/types/ownable.rs
@@ -2,6 +2,7 @@
 
 //! Owned reference types.
 
+use crate::types::{ARef, RefCounted};
 use core::{
     marker::PhantomData,
     mem::ManuallyDrop,
@@ -18,8 +19,9 @@
 ///
 /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
 /// provide reference counting but represents a unique, owned reference. If reference counting is
-/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
-/// wrapped in an [`ARef<Self>`](crate::types::ARef).
+/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
+/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
+/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
 ///
 /// # Safety
 ///
@@ -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.
+/// - [`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>`].
+///
+/// # Examples
+///
+/// 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.");
+///         // SAFETY: We just allocated the `Foo`, thus it is valid.
+///         Ok(unsafe { Owned::from_raw(result) })
+///     }
+/// }
+///
+/// // SAFETY: We increment and decrement each time the respective function is called and only free
+/// // the `Foo` when the refcount reaches zero.
+/// unsafe impl RefCounted for Foo {
+///     fn inc_ref(&self) {
+///         self.refcount.replace(self.refcount.get() + 1);
+///     }
+///
+///     unsafe fn dec_ref(this: NonNull<Self>) {
+///         // SAFETY: The underlying object is always valid when the function is called.
+///         let refcount = unsafe { &this.as_ref().refcount };
+///         let new_refcount = refcount.get() - 1;
+///         if new_refcount == 0 {
+///             // The `Foo` will be dropped when `KBox` goes out of scope.
+///             // SAFETY: The `Box<Foo>` is still alive as the old refcount is 1.
+///             unsafe { KBox::from_raw(this.as_ptr()) };
+///         } else {
+///             refcount.replace(new_refcount);
+///         }
+///     }
+/// }
+///
+/// // SAFETY: We only convert into an `Owned` when the refcount is 1.
+/// unsafe impl OwnableRefCounted for Foo {
+///     fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
+///         if this.refcount.get() == 1 {
+///             // SAFETY: The `Foo` is still alive as the refcount is 1.
+///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
+///         } else {
+///             Err(this)
+///         }
+///     }
+/// }
+///
+/// // SAFETY: We are not `AlwaysRefCounted`.
+/// unsafe impl Ownable for Foo {
+///     unsafe fn release(this: NonNull<Self>) {
+///         // SAFETY: Using `dec_ref()` from `RefCounted` to release is okay, as the refcount is
+///         // always 1 for an `Owned<Foo>`.
+///         unsafe{ Foo::dec_ref(this) };
+///     }
+/// }
+///
+/// let foo = Foo::new().unwrap();
+/// let mut foo = ARef::from(foo);
+/// {
+///     let bar = foo.clone();
+///     assert!(Owned::try_from(bar).is_err());
+/// }
+/// assert!(Owned::try_from(foo).is_ok());
+/// ```
+pub unsafe trait OwnableRefCounted: RefCounted + Ownable + Sized {
+    /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
+    /// Otherwise it returns again an [`ARef`] to the same underlying object.
+    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
+
+    /// Converts the [`Owned`] into an [`ARef`].
+    fn into_shared(this: Owned<Self>) -> ARef<Self> {
+        // SAFETY: Safe by the requirements on implementing the trait.
+        unsafe { ARef::from_raw(Owned::into_raw(this)) }
+    }
+}
+
+impl<T: OwnableRefCounted> TryFrom<ARef<T>> for Owned<T> {
+    type Error = ARef<T>;
+    /// Tries to convert the [`ARef`] to an [`Owned`] by calling
+    /// [`try_from_shared()`](OwnableRefCounted::try_from_shared). In case the [`ARef`] is not
+    /// unique, it returns again an [`ARef`] to the same underlying object.
+    fn try_from(b: ARef<T>) -> Result<Owned<T>, Self::Error> {
+        T::try_from_shared(b)
+    }
+}

-- 
2.49.0

On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
> --- a/rust/kernel/types/ownable.rs
> +++ b/rust/kernel/types/ownable.rs
> @@ -2,6 +2,7 @@
>  
>  //! Owned reference types.
>  
> +use crate::types::{ARef, RefCounted};
>  use core::{
>      marker::PhantomData,
>      mem::ManuallyDrop,
> @@ -18,8 +19,9 @@
>  ///
>  /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
>  /// provide reference counting but represents a unique, owned reference. If reference counting is
> -/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
> -/// wrapped in an [`ARef<Self>`](crate::types::ARef).
> +/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
> +/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
> +/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).

This change should probably be in the earlier patch.

>  ///
>  /// # Safety
>  ///
> @@ -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?

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

> +///
> +/// # Examples

If we're having an example here, then we should also have on on `Owned`.

> +///
> +/// 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`)

> +///         // SAFETY: We just allocated the `Foo`, thus it is valid.

This isn't going through all the requirements on `from_raw`...

---
Cheers,
Benno

> +///         Ok(unsafe { Owned::from_raw(result) })
> +///     }
> +/// }
> +///
> +/// // SAFETY: We increment and decrement each time the respective function is called and only free
> +/// // the `Foo` when the refcount reaches zero.
> +/// unsafe impl RefCounted for Foo {
> +///     fn inc_ref(&self) {
> +///         self.refcount.replace(self.refcount.get() + 1);
> +///     }
> +///
> +///     unsafe fn dec_ref(this: NonNull<Self>) {
> +///         // SAFETY: The underlying object is always valid when the function is called.
> +///         let refcount = unsafe { &this.as_ref().refcount };
> +///         let new_refcount = refcount.get() - 1;
> +///         if new_refcount == 0 {
> +///             // The `Foo` will be dropped when `KBox` goes out of scope.
> +///             // SAFETY: The `Box<Foo>` is still alive as the old refcount is 1.
> +///             unsafe { KBox::from_raw(this.as_ptr()) };
> +///         } else {
> +///             refcount.replace(new_refcount);
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: We only convert into an `Owned` when the refcount is 1.
> +/// unsafe impl OwnableRefCounted for Foo {
> +///     fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> +///         if this.refcount.get() == 1 {
> +///             // SAFETY: The `Foo` is still alive as the refcount is 1.
> +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> +///         } else {
> +///             Err(this)
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: We are not `AlwaysRefCounted`.
> +/// unsafe impl Ownable for Foo {
> +///     unsafe fn release(this: NonNull<Self>) {
> +///         // SAFETY: Using `dec_ref()` from `RefCounted` to release is okay, as the refcount is
> +///         // always 1 for an `Owned<Foo>`.
> +///         unsafe{ Foo::dec_ref(this) };
> +///     }
> +/// }
> +///
> +/// let foo = Foo::new().unwrap();
> +/// let mut foo = ARef::from(foo);
> +/// {
> +///     let bar = foo.clone();
> +///     assert!(Owned::try_from(bar).is_err());
> +/// }
> +/// assert!(Owned::try_from(foo).is_ok());
> +/// ```
> +pub unsafe trait OwnableRefCounted: RefCounted + Ownable + Sized {
> +    /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
> +    /// Otherwise it returns again an [`ARef`] to the same underlying object.
> +    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
> +
> +    /// Converts the [`Owned`] into an [`ARef`].
> +    fn into_shared(this: Owned<Self>) -> ARef<Self> {
> +        // SAFETY: Safe by the requirements on implementing the trait.
> +        unsafe { ARef::from_raw(Owned::into_raw(this)) }
> +    }
> +}
> +
> +impl<T: OwnableRefCounted> TryFrom<ARef<T>> for Owned<T> {
> +    type Error = ARef<T>;
> +    /// Tries to convert the [`ARef`] to an [`Owned`] by calling
> +    /// [`try_from_shared()`](OwnableRefCounted::try_from_shared). In case the [`ARef`] is not
> +    /// unique, it returns again an [`ARef`] to the same underlying object.
> +    fn try_from(b: ARef<T>) -> Result<Owned<T>, Self::Error> {
> +        T::try_from_shared(b)
> +    }
> +}

On 250702 1524, Benno Lossin wrote:
> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> > diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> > index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
> > --- a/rust/kernel/types/ownable.rs
> > +++ b/rust/kernel/types/ownable.rs
> > @@ -2,6 +2,7 @@
> >
> >  //! Owned reference types.
> >
> > +use crate::types::{ARef, RefCounted};
> >  use core::{
> >      marker::PhantomData,
> >      mem::ManuallyDrop,
> > @@ -18,8 +19,9 @@
> >  ///
> >  /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> >  /// provide reference counting but represents a unique, owned reference. If reference counting is
> > -/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
> > -/// wrapped in an [`ARef<Self>`](crate::types::ARef).
> > +/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
> > +/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
> > +/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
> 
> This change should probably be in the earlier patch.

Ah, yes. Must have happened while splitting the patches.

> >  ///
> >  /// # Safety
> >  ///
> > @@ -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.

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

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

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

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

> > +///         // SAFETY: We just allocated the `Foo`, thus it is valid.
> 
> This isn't going through all the requirements on `from_raw`...

Yes, this comment should probably be detailed.

Best,

Oliver

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.

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

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

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

---
Cheers,
Benno

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

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

"Benno Lossin" <lossin@kernel.org> writes:

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

[...]

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

We have been a bit back and forth on this. This is how we would like it
going forward:


/// There are three states for a request that the Rust bindings care about:
///
/// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
/// - 1: The request is owned by Rust abstractions but is not referenced.
/// - 2+: There is one or more [`ARef`] instances referencing the request.


Best regards,
Andreas Hindborg

On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> 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.
>
> We have been a bit back and forth on this. This is how we would like it
> going forward:
>
>
> /// There are three states for a request that the Rust bindings care about:
> ///
> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
> /// - 1: The request is owned by Rust abstractions but is not referenced.
> /// - 2+: There is one or more [`ARef`] instances referencing the request.

Huh, now I'm more confused... Could you go into the details again?

---
Cheers,
Benno

"Benno Lossin" <lossin@kernel.org> writes:

> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> 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.
>>
>> We have been a bit back and forth on this. This is how we would like it
>> going forward:
>>
>>
>> /// There are three states for a request that the Rust bindings care about:
>> ///
>> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
>> /// - 1: The request is owned by Rust abstractions but is not referenced.
>> /// - 2+: There is one or more [`ARef`] instances referencing the request.
>
> Huh, now I'm more confused... Could you go into the details again?

Well, there is not much to it. We found out we can alias "unique" and
"owned by C".

We initialize the refcount to 0 when we initialize the request
structure. This happens at queue creation time.

When C block layer hands over a request for processing to a Rust driver,
we `debug_assert!` that the refcount is 0. We unsafely invent an
`Owned<Request<_>>` and pass that to the driver.

The driver has the option of `into_shared` to obtain an
`ARef<Request<_>>`. We use this for remote completion and timer
completion in rnull.

In most drivers, when the driver hands off the request to the hardware,
the driver will stop accounting for the request. The `Owned<Request<_>>`
is consumed when issuing to the driver, or the driver could simply drop
it. Refcount goes to 1. An ID is passed along to hardware.

When a completion comes back from hardware, it carries the ID. We use
the C block layer `tag_to_rq` machinery to turn this ID back into an
`Owned<Request<_>>`. In that process, we check that `refcount == 1` and
if so, we set refcount to 0 and invent an `Owned<Request<_>>`.

There was simply no need to have two separate values for "owned by C"
and "owned by Rust, not referenced".

Best regards,
Andreas Hindborg

On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> 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.
>>>
>>> We have been a bit back and forth on this. This is how we would like it
>>> going forward:
>>>
>>>
>>> /// There are three states for a request that the Rust bindings care about:
>>> ///
>>> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
>>> /// - 1: The request is owned by Rust abstractions but is not referenced.
>>> /// - 2+: There is one or more [`ARef`] instances referencing the request.
>>
>> Huh, now I'm more confused... Could you go into the details again?
>
> Well, there is not much to it. We found out we can alias "unique" and
> "owned by C".
>
> We initialize the refcount to 0 when we initialize the request
> structure. This happens at queue creation time.

And IIRC this refcount is only on the Rust side, since you store it in
some private data, right?

> When C block layer hands over a request for processing to a Rust driver,
> we `debug_assert!` that the refcount is 0. We unsafely invent an
> `Owned<Request<_>>` and pass that to the driver.

And you don't increment the refcount?

> The driver has the option of `into_shared` to obtain an
> `ARef<Request<_>>`. We use this for remote completion and timer
> completion in rnull.

This operation will set the refcount to 2?

> In most drivers, when the driver hands off the request to the hardware,
> the driver will stop accounting for the request. The `Owned<Request<_>>`
> is consumed when issuing to the driver, or the driver could simply drop
> it. Refcount goes to 1. An ID is passed along to hardware.

And with the current API design it must let go of all `ARef<Request<_>>`
because otherwise it can't call `end_ok` (or some other method?).

> When a completion comes back from hardware, it carries the ID. We use
> the C block layer `tag_to_rq` machinery to turn this ID back into an
> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.

(because if not, this would be wrong)

I have some idea what we should do for the safety requirements of
`Ownable`, but still need some time to write it down.

---
Cheers,
Benno

> There was simply no need to have two separate values for "owned by C"
> and "owned by Rust, not referenced".
>
> Best regards,
> Andreas Hindborg

"Benno Lossin" <lossin@kernel.org> writes:

> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> 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.
>>>>
>>>> We have been a bit back and forth on this. This is how we would like it
>>>> going forward:
>>>>
>>>>
>>>> /// There are three states for a request that the Rust bindings care about:
>>>> ///
>>>> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
>>>> /// - 1: The request is owned by Rust abstractions but is not referenced.
>>>> /// - 2+: There is one or more [`ARef`] instances referencing the request.
>>>
>>> Huh, now I'm more confused... Could you go into the details again?
>>
>> Well, there is not much to it. We found out we can alias "unique" and
>> "owned by C".
>>
>> We initialize the refcount to 0 when we initialize the request
>> structure. This happens at queue creation time.
>
> And IIRC this refcount is only on the Rust side, since you store it in
> some private data, right?

Yes.

>
>> When C block layer hands over a request for processing to a Rust driver,
>> we `debug_assert!` that the refcount is 0. We unsafely invent an
>> `Owned<Request<_>>` and pass that to the driver.
>
> And you don't increment the refcount?

No, we leave it at 0.

>
>> The driver has the option of `into_shared` to obtain an
>> `ARef<Request<_>>`. We use this for remote completion and timer
>> completion in rnull.
>
> This operation will set the refcount to 2?

Yes.

>
>> In most drivers, when the driver hands off the request to the hardware,
>> the driver will stop accounting for the request. The `Owned<Request<_>>`
>> is consumed when issuing to the driver, or the driver could simply drop
>> it. Refcount goes to 1. An ID is passed along to hardware.
>
> And with the current API design it must let go of all `ARef<Request<_>>`
> because otherwise it can't call `end_ok` (or some other method?).

Exactly. That one will take an `Owned<Request<_>>` (previously `Unique`).

>
>> When a completion comes back from hardware, it carries the ID. We use
>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>
> (because if not, this would be wrong)

I hope it is not wrong 😆 You can see the latest and greatest here [1].

>
> I have some idea what we should do for the safety requirements of
> `Ownable`, but still need some time to write it down.

Great!


Best regards,
Andreas Hindborg


[1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398

On Tue Jul 8, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>>> When a completion comes back from hardware, it carries the ID. We use
>>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>>
>> (because if not, this would be wrong)
>
> I hope it is not wrong 😆 You can see the latest and greatest here [1].

Thanks for the pointer, the "implementation details" in the docs on
`Request` were very helpful! This is another argument for not having the
blanket impl for `Ownable` when `T: Refcount` I mentioned in the other
thread.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398

The comment in line 424 looks wrong?

---
Cheers,
Benno

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Jul 8, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>>>> When a completion comes back from hardware, it carries the ID. We use
>>>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>>>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>>>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>>>
>>> (because if not, this would be wrong)
>>
>> I hope it is not wrong 😆 You can see the latest and greatest here [1].
>
> Thanks for the pointer, the "implementation details" in the docs on
> `Request` were very helpful! This is another argument for not having the
> blanket impl for `Ownable` when `T: Refcount` I mentioned in the other
> thread.
>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398
>
> The comment in line 424 looks wrong?

Yes, nice catch. The assert is correct though.


Best regards,
Andreas Hindborg