[PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Oliver Mangold]

Add `UniqueRef` as a variant of `ARef` that is guaranteed to be unique.
This is useful when mutable access to the underlying type is required
and we can guarantee uniqueness, and when APIs that would normally take
an `ARef` require uniqueness.

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
---
Changes in v4:
- Just a minor change in naming by request from Andreas Hindborg,
  try_shared_to_unique() -> try_from_shared(),
  unique_to_shared() -> into_shared(),
  which is more in line with standard Rust naming conventions.
- Link to v3: https://lore.kernel.org/r/Z8Wuud2UQX6Yukyr@mango
---
 rust/kernel/types.rs | 315 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 315 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 55ddd50e8aaa075ac33d5f1088a7f72df05f74f4..6f8f0b8863ffdac336985fbad60882ad0699f0fa 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -543,6 +543,12 @@ fn from(b: &T) -> Self {
     }
 }
 
+impl<T: UniqueRefCounted> From<UniqueRef<T>> for ARef<T> {
+    fn from(b: UniqueRef<T>) -> Self {
+        UniqueRefCounted::into_shared(b)
+    }
+}
+
 impl<T: AlwaysRefCounted> Drop for ARef<T> {
     fn drop(&mut self) {
         // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
@@ -551,6 +557,315 @@ fn drop(&mut self) {
     }
 }
 
+/// Types that are [`AlwaysRefCounted`] and can be safely converted to an [`UniqueRef`]
+///
+/// # Safety
+///
+/// Implementers must ensure that the methods of the trait
+/// change the reference count of the underlying object such that:
+/// - the uniqueness invariant is upheld, i.e. it is not possible
+///   to obtain another reference by any means (other than through the [`UniqueRef`])
+///   until the [`UniqueRef`] is dropped or converted to an [`ARef`].
+/// - [`dec_ref()`](UniqueRefCounted::dec_ref) correctly frees the underlying object.
+/// - [`into_shared()`](UniqueRefCounted::into_shared) set the reference count
+///   to the value which the returned [`ARef`] expects for an object with a single reference
+///   in existence. This implies that if [`into_shared()`](UniqueRefCounted::into_shared)
+///   is left on the default implementation, which just rewraps the underlying object,
+///   the reference count needs not to be
+///   modified when converting a [`UniqueRef`] to an [`ARef`].
+///
+/// # Examples
+///
+/// A minimal example implementation of [`AlwaysRefCounted`] and
+/// [`UniqueRefCounted`] and their usage
+/// with [`ARef`] and [`UniqueRef`] 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, AlwaysRefCounted, UniqueRef, UniqueRefCounted,
+/// };
+///
+/// struct Foo {
+///     refcount: Cell<usize>,
+/// }
+///
+/// impl Foo {
+///     fn new() -> Result<UniqueRef<Self>, AllocError> {
+///         // Use a KBox to handle the actual allocation
+///         let result = KBox::new(
+///             Foo {
+///                 refcount: Cell::new(1),
+///             },
+///             flags::GFP_KERNEL,
+///         )?;
+///         // SAFETY: we just allocated the `Foo`, thus it is valid
+///         Ok(unsafe { UniqueRef::from_raw(NonNull::new(KBox::into_raw(result)).unwrap()) })
+///     }
+/// }
+///
+/// // SAFETY: we increment and decrement correctly and only free the Foo
+/// // when the refcount reaches zero
+/// unsafe impl AlwaysRefCounted 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 {
+///             // 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 `UniqueRef` when the refcount is 1
+/// unsafe impl UniqueRefCounted for Foo {
+///     fn try_from_shared(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>> {
+///         if this.refcount.get() == 1 {
+///             // SAFETY: the `Foo` is still alive as the refcount is 1
+///             Ok(unsafe { UniqueRef::from_raw(ARef::into_raw(this)) })
+///         } else {
+///             Err(this)
+///         }
+///     }
+/// }
+///
+/// let foo = Foo::new().unwrap();
+/// let mut foo = ARef::from(foo);
+/// {
+///     let bar = foo.clone();
+///     assert!(UniqueRef::try_from(bar).is_err());
+/// }
+/// assert!(UniqueRef::try_from(foo).is_ok());
+/// ```
+pub unsafe trait UniqueRefCounted: AlwaysRefCounted + Sized {
+    /// Checks if the [`ARef`] is unique and convert it
+    /// to an [`UniqueRef`] it that is that case.
+    /// Otherwise it returns again an [`ARef`] to the same
+    /// underlying object.
+    fn try_from_shared(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
+    /// Converts the [`UniqueRef`] into an [`ARef`].
+    fn into_shared(this: UniqueRef<Self>) -> ARef<Self> {
+        // SAFETY: safe by the conditions on implementing the trait
+        unsafe { ARef::from_raw(UniqueRef::into_raw(this)) }
+    }
+    /// Decrements the reference count on the object when the [`UniqueRef`] is dropped.
+    ///
+    /// Frees the object when the count reaches zero.
+    ///
+    /// It defaults to [`AlwaysRefCounted::dec_ref`],
+    /// but overriding it may be useful, e.g. in case of non-standard refcounting
+    /// schemes.
+    ///
+    /// # Safety
+    ///
+    /// The same safety constraints as for [`AlwaysRefCounted::dec_ref`] apply,
+    /// but as the reference is unique, it can be assumed that the function
+    /// will not be called twice. In case the default implementation is not
+    /// overridden, it has to be ensured that the call to [`AlwaysRefCounted::dec_ref`]
+    /// can be used for an [`UniqueRef`], too.
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: correct by function safety requirements
+        unsafe { AlwaysRefCounted::dec_ref(obj) };
+    }
+}
+
+/// This trait allows to implement [`UniqueRefCounted`] in a simplified way,
+/// only requiring to provide an [`is_unique()`](SimpleUniqueRefCounted::is_unique) method.
+///
+/// # Safety
+///
+/// - The same safety requirements as for [`UniqueRefCounted`] apply.
+/// - [`is_unique`](SimpleUniqueRefCounted::is_unique) must only return `true`
+///   in case only one [`ARef`] exists and it is impossible for one to be obtained
+///   other than by cloning an existing [`ARef`] or converting an [`UniqueRef`] to an [`ARef`].
+/// - It is safe to convert an unique [`ARef`] into an [`UniqueRef`]
+///   simply by re-wrapping the underlying object without modifying the refcount.
+///
+/// # Examples
+///
+/// A minimal example implementation of [`AlwaysRefCounted`] and
+/// [`SimpleUniqueRefCounted`] and their usage
+/// with [`ARef`] and [`UniqueRef`] 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, AlwaysRefCounted, SimpleUniqueRefCounted, UniqueRef,
+/// };
+///
+/// struct Foo {
+///     refcount: Cell<usize>,
+/// }
+///
+/// impl Foo {
+///     fn new() -> Result<UniqueRef<Self>, AllocError> {
+///         // Use a KBox to handle the actual allocation
+///         let result = KBox::new(
+///             Foo {
+///                 refcount: Cell::new(1),
+///             },
+///             flags::GFP_KERNEL,
+///         )?;
+///         // SAFETY: we just allocated the `Foo`, thus it is valid
+///         Ok(unsafe { UniqueRef::from_raw(NonNull::new(KBox::into_raw(result)).unwrap()) })
+///     }
+/// }
+///
+/// // SAFETY: we just allocated the `Foo`, thus it is valid
+/// unsafe impl AlwaysRefCounted 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 {
+///             // 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 check the refcount as required. Races are impossible as the object is not `Sync`.
+/// unsafe impl SimpleUniqueRefCounted for Foo {
+///     fn is_unique(&self) -> bool {
+///         self.refcount.get() == 1
+///     }
+/// }
+///
+/// let foo = Foo::new().unwrap();
+/// let mut foo = ARef::from(foo);
+/// {
+///     let bar = foo.clone();
+///     assert!(UniqueRef::try_from(bar).is_err());
+/// }
+/// assert!(UniqueRef::try_from(foo).is_ok());
+/// ```
+pub unsafe trait SimpleUniqueRefCounted: AlwaysRefCounted + Sized {
+    /// Checks if exactly one [`ARef`] to the object exists.
+    /// In case the object is [`Sync`] the check needs to be race-free.
+    fn is_unique(&self) -> bool;
+}
+
+// SAFETY: safe by the requirements on implementation of `[SimpleUniqueRefCounted`]
+unsafe impl<T: SimpleUniqueRefCounted> UniqueRefCounted for T {
+    fn try_from_shared(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>> {
+        if this.is_unique() {
+            // SAFETY: safe by the requirements on implementation of `[SimpleUniqueRefCounted`]
+            Ok(unsafe { UniqueRef::from_raw(ARef::into_raw(this)) })
+        } else {
+            Err(this)
+        }
+    }
+}
+
+/// An unique, owned reference to an [`AlwaysRefCounted`] object.
+///
+/// It works the same ways as [`ARef`] but ensures that the reference is unique
+/// and thus can be dereferenced mutably.
+///
+/// # Invariants
+///
+/// - The pointer stored in `ptr` is non-null and valid for the lifetime of the [`UniqueRef`]
+///   instance. In particular, the [`UniqueRef`] instance owns an increment
+///   on the underlying object's reference count.
+/// - No other [`UniqueRef`] or [`ARef`] to the underlying object exist
+///   while the [`UniqueRef`] is live.
+pub struct UniqueRef<T: UniqueRefCounted> {
+    ptr: NonNull<T>,
+    _p: PhantomData<T>,
+}
+
+// SAFETY: It is safe to send `UniqueRef<T>` to another thread
+// when the underlying `T` is `Send` because it effectively means a transfer of ownership,
+// equivalently to sending a `Box<T>`.
+unsafe impl<T: UniqueRefCounted + Send> Send for UniqueRef<T> {}
+
+// SAFETY: It is safe to send `&UniqueRef<T>` to another thread when the underlying `T` is `Sync`
+// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
+unsafe impl<T: UniqueRefCounted + Sync> Sync for UniqueRef<T> {}
+
+impl<T: UniqueRefCounted> UniqueRef<T> {
+    /// Creates a new instance of [`UniqueRef`].
+    ///
+    /// It takes over an increment of the reference count on the underlying object.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the reference count is set to such a value
+    /// that a call to [`dec_ref()`](UniqueRefCounted::dec_ref) will release the underlying object
+    /// in the way which is expected when the last reference is dropped.
+    /// Callers must not use the underlying object anymore --
+    /// it is only safe to do so via the newly created [`UniqueRef`].
+    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
+        // INVARIANT: The safety requirements guarantee that the new instance now owns the
+        // increment on the refcount.
+        Self {
+            ptr,
+            _p: PhantomData,
+        }
+    }
+
+    /// Consumes the [`UniqueRef`], returning a raw pointer.
+    ///
+    /// This function does not change the refcount. After calling this function, the caller is
+    /// responsible for the refcount previously managed by the [`UniqueRef`].
+    pub fn into_raw(me: Self) -> NonNull<T> {
+        ManuallyDrop::new(me).ptr
+    }
+}
+
+impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: The type invariants guarantee that the object is valid.
+        unsafe { self.ptr.as_ref() }
+    }
+}
+
+impl<T: UniqueRefCounted> DerefMut for UniqueRef<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        // SAFETY: The type invariants guarantee that the object is valid.
+        unsafe { self.ptr.as_mut() }
+    }
+}
+
+impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
+    type Error = ARef<T>;
+    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
+    /// by calling [`try_from_shared()`](UniqueRefCounted::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<UniqueRef<T>, Self::Error> {
+        UniqueRefCounted::try_from_shared(b)
+    }
+}
+
+impl<T: UniqueRefCounted> Drop for UniqueRef<T> {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that the [`UniqueRef`] owns the reference
+        // we're about to decrement.
+        unsafe { UniqueRefCounted::dec_ref(self.ptr) };
+    }
+}
+
 /// A sum type that always holds either a value of type `L` or `R`.
 ///
 /// # Examples

---
base-commit: 4b2ee22fe32ea9b255926effbb6f26450607c391
change-id: 20250305-unique-ref-29fcd675f9e9

Best regards,
-- 
Oliver Mangold <oliver.mangold@pm.me>

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Alice Ryhl]

On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
> Add `UniqueRef` as a variant of `ARef` that is guaranteed to be unique.
> This is useful when mutable access to the underlying type is required
> and we can guarantee uniqueness, and when APIs that would normally take
> an `ARef` require uniqueness.
> 
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>

[...]

> +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}

What stops people from doing this?

let my_unique: UniqueRef<T> = ...;
let my_ref: &T = &*my_unique;
let my_shared: ARef<T> = ARef::from(my_ref);

Now it is no longer unique.

> +impl<T: UniqueRefCounted> DerefMut for UniqueRef<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}

This DerefMut will make it almost impossible for C types to implement
UniqueRefCounted because it is incompatible with pinning. You probably
want `T: UniqueRefCounted + Unpin` here.

For `T: !Unpin` (i.e. almost all C types), you can at most produce an
`Pin<&mut Self>`.

Alice

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Oliver Mangold]

Hi Alice,

On 250305 1339, Alice Ryhl wrote:
> On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
> 
> > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
> > +    type Target = T;
> > +
> > +    fn deref(&self) -> &Self::Target {
> > +        // SAFETY: The type invariants guarantee that the object is valid.
> > +        unsafe { self.ptr.as_ref() }
> > +    }
> > +}
> 
> What stops people from doing this?
> 
> let my_unique: UniqueRef<T> = ...;
> let my_ref: &T = &*my_unique;
> let my_shared: ARef<T> = ARef::from(my_ref);
> 
> Now it is no longer unique.
>
Oh, indeed. That's a serious problem. I see 2 options to deal with that:

1. remove ARef::From<&T>

I checked the users of this, and it looks to me like there is rather
a limited number and they are easy to fix by replacing the &T with ARef<T>.
But I assume that wouldn't be welcome as it is intrusive nonetheless
and of course there is ergonomic value in having the function around.

2. add some new traits so implementers can opt in/out of that function.

Basically one would have to pick if one wants to ARef::From<&T> or
UniqueRef<T> for one's type.

> > +impl<T: UniqueRefCounted> DerefMut for UniqueRef<T> {
> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> > +        // SAFETY: The type invariants guarantee that the object is valid.
> > +        unsafe { self.ptr.as_mut() }
> > +    }
> > +}
> 
> This DerefMut will make it almost impossible for C types to implement
> UniqueRefCounted because it is incompatible with pinning. You probably
> want `T: UniqueRefCounted + Unpin` here.
> 
> For `T: !Unpin` (i.e. almost all C types), you can at most produce an
> `Pin<&mut Self>`.

I think I only understand 70% of that incompatiblity, but I will do a
bit more reading.

In any case I think I can work around that, but doing as you say,
and maybe adding an extra method deref_pin() to get a pinned reference.

Best,

Oliver

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Alice Ryhl]

On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> Hi Alice,
>
> On 250305 1339, Alice Ryhl wrote:
> > On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
> >
> > > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
> > > +    type Target = T;
> > > +
> > > +    fn deref(&self) -> &Self::Target {
> > > +        // SAFETY: The type invariants guarantee that the object is valid.
> > > +        unsafe { self.ptr.as_ref() }
> > > +    }
> > > +}
> >
> > What stops people from doing this?
> >
> > let my_unique: UniqueRef<T> = ...;
> > let my_ref: &T = &*my_unique;
> > let my_shared: ARef<T> = ARef::from(my_ref);
> >
> > Now it is no longer unique.
> >
> Oh, indeed. That's a serious problem. I see 2 options to deal with that:
>
> 1. remove ARef::From<&T>
>
> I checked the users of this, and it looks to me like there is rather
> a limited number and they are easy to fix by replacing the &T with ARef<T>.
> But I assume that wouldn't be welcome as it is intrusive nonetheless
> and of course there is ergonomic value in having the function around.

Definitely not an option. There are many users of this function that
are in the process of being upstreamed. The ability to go &T ->
ARef<T> is pretty fundamental for ARef.

> 2. add some new traits so implementers can opt in/out of that function.
>
> Basically one would have to pick if one wants to ARef::From<&T> or
> UniqueRef<T> for one's type.

I do think that you essentially need two structs to use this at all -
one for the shared and one for the unique case. Sounds pretty
unergonomic.

What is the use-case for these abstractions?

Alice

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>
>> Hi Alice,
>>
>> On 250305 1339, Alice Ryhl wrote:
>> > On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
>> >
>> > > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
>> > > +    type Target = T;
>> > > +
>> > > +    fn deref(&self) -> &Self::Target {
>> > > +        // SAFETY: The type invariants guarantee that the object is valid.
>> > > +        unsafe { self.ptr.as_ref() }
>> > > +    }
>> > > +}
>> >
>> > What stops people from doing this?
>> >
>> > let my_unique: UniqueRef<T> = ...;
>> > let my_ref: &T = &*my_unique;
>> > let my_shared: ARef<T> = ARef::from(my_ref);
>> >
>> > Now it is no longer unique.
>> >
>> Oh, indeed. That's a serious problem. I see 2 options to deal with that:
>>
>> 1. remove ARef::From<&T>
>>
>> I checked the users of this, and it looks to me like there is rather
>> a limited number and they are easy to fix by replacing the &T with ARef<T>.
>> But I assume that wouldn't be welcome as it is intrusive nonetheless
>> and of course there is ergonomic value in having the function around.
>
> Definitely not an option. There are many users of this function that
> are in the process of being upstreamed. The ability to go &T ->
> ARef<T> is pretty fundamental for ARef.

Not having `impl From<&T> for UniqueArc` seems to work out fine.

It would be unfortunate if `impl From<&T> for ARef<T>` would prevent us
from having a unique version of `ARef`. I would say that is a valid
reason to consider removing that impl.


Best regards,
Andreas Hindborg

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>

[...]

>
> What is the use-case for these abstractions?

Missed this Q. It is for asserting uniqueness of a reference to a block
layer request [1].


Best regards,
Andreas Hindborg



[1] https://github.com/metaspace/linux/blob/44dd1c6eff584475f13e2a73eb4ad25df17d0d55/rust/kernel/block/mq/request.rs#L406

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Alice Ryhl]

On Wed, Mar 5, 2025 at 4:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
> >>
> >> Hi Alice,
> >>
> >> On 250305 1339, Alice Ryhl wrote:
> >> > On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
> >> >
> >> > > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
> >> > > +    type Target = T;
> >> > > +
> >> > > +    fn deref(&self) -> &Self::Target {
> >> > > +        // SAFETY: The type invariants guarantee that the object is valid.
> >> > > +        unsafe { self.ptr.as_ref() }
> >> > > +    }
> >> > > +}
> >> >
> >> > What stops people from doing this?
> >> >
> >> > let my_unique: UniqueRef<T> = ...;
> >> > let my_ref: &T = &*my_unique;
> >> > let my_shared: ARef<T> = ARef::from(my_ref);
> >> >
> >> > Now it is no longer unique.
> >> >
> >> Oh, indeed. That's a serious problem. I see 2 options to deal with that:
> >>
> >> 1. remove ARef::From<&T>
> >>
> >> I checked the users of this, and it looks to me like there is rather
> >> a limited number and they are easy to fix by replacing the &T with ARef<T>.
> >> But I assume that wouldn't be welcome as it is intrusive nonetheless
> >> and of course there is ergonomic value in having the function around.
> >
> > Definitely not an option. There are many users of this function that
> > are in the process of being upstreamed. The ability to go &T ->
> > ARef<T> is pretty fundamental for ARef.
>
> Not having `impl From<&T> for UniqueArc` seems to work out fine.
>
> It would be unfortunate if `impl From<&T> for ARef<T>` would prevent us
> from having a unique version of `ARef`. I would say that is a valid
> reason to consider removing that impl.

I think the impl is really important. It's required to do things such as:

let mm = ARef::from(&*current!().mm());

Without the impl (or something equivalent), it's not possible to
increment the refcount of the &Mm returned by `current!().mm()`. There
are many other examples of this.

Alice

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Oliver Mangold]

On 250305 1613, Alice Ryhl wrote:
> On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
> > >
> > > What stops people from doing this?
> > >
> > > let my_unique: UniqueRef<T> = ...;
> > > let my_ref: &T = &*my_unique;
> > > let my_shared: ARef<T> = ARef::from(my_ref);
> > >
> > > Now it is no longer unique.
> > >
> > Oh, indeed. That's a serious problem. I see 2 options to deal with that:
> >
> > 1. remove ARef::From<&T>
> >
> > I checked the users of this, and it looks to me like there is rather
> > a limited number and they are easy to fix by replacing the &T with ARef<T>.
> > But I assume that wouldn't be welcome as it is intrusive nonetheless
> > and of course there is ergonomic value in having the function around.
> 
> Definitely not an option. There are many users of this function that
> are in the process of being upstreamed. The ability to go &T ->
> ARef<T> is pretty fundamental for ARef.

Ok. Suspected something like that.
> 
> I do think that you essentially need two structs to use this at all -
> one for the shared and one for the unique case. Sounds pretty
> unergonomic.
>
Sorry, but can you explain? Why does one need structs?

> What is the use-case for these abstractions?
>
It came up in the block subsystem. For mq::Request it is rather essential
to be able to ensure that one has a unique reference.

Maybe Andreas can explain a bit more.

We can work around that there, with special wrappers specific for block,
sure.

But are you sure it isn't much more widely useful? Correct me if I'm wrong,
but as I understand, currently for AlwaysRefCounted objects you can never
safely obtain an &mut. So you cannot use the static safety checks
that come with '&' vs. '&mut'. This means you have to rely on interior
mutability, requiring runtime checks in the wrapper functions, no?

Isn't that, for example, the reason that Page is not AlwaysRefCounted,
as write access is protected by requiring an '&mut'?

Best,

Oliver

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Mar 5, 2025 at 4:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>> >>
>> >> Hi Alice,
>> >>
>> >> On 250305 1339, Alice Ryhl wrote:
>> >> > On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
>> >> >
>> >> > > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
>> >> > > +    type Target = T;
>> >> > > +
>> >> > > +    fn deref(&self) -> &Self::Target {
>> >> > > +        // SAFETY: The type invariants guarantee that the object is valid.
>> >> > > +        unsafe { self.ptr.as_ref() }
>> >> > > +    }
>> >> > > +}
>> >> >
>> >> > What stops people from doing this?
>> >> >
>> >> > let my_unique: UniqueRef<T> = ...;
>> >> > let my_ref: &T = &*my_unique;
>> >> > let my_shared: ARef<T> = ARef::from(my_ref);
>> >> >
>> >> > Now it is no longer unique.
>> >> >
>> >> Oh, indeed. That's a serious problem. I see 2 options to deal with that:
>> >>
>> >> 1. remove ARef::From<&T>
>> >>
>> >> I checked the users of this, and it looks to me like there is rather
>> >> a limited number and they are easy to fix by replacing the &T with ARef<T>.
>> >> But I assume that wouldn't be welcome as it is intrusive nonetheless
>> >> and of course there is ergonomic value in having the function around.
>> >
>> > Definitely not an option. There are many users of this function that
>> > are in the process of being upstreamed. The ability to go &T ->
>> > ARef<T> is pretty fundamental for ARef.
>>
>> Not having `impl From<&T> for UniqueArc` seems to work out fine.
>>
>> It would be unfortunate if `impl From<&T> for ARef<T>` would prevent us
>> from having a unique version of `ARef`. I would say that is a valid
>> reason to consider removing that impl.
>
> I think the impl is really important. It's required to do things such as:
>
> let mm = ARef::from(&*current!().mm());
>
> Without the impl (or something equivalent), it's not possible to
> increment the refcount of the &Mm returned by `current!().mm()`. There
> are many other examples of this.

Right. Let's see what we can figure out of other solutions then.


Best regards,
Andreas Hindborg

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Alice Ryhl]

On Wed, Mar 05, 2025 at 06:24:56PM +0100, Andreas Hindborg wrote:
> "Alice Ryhl" <aliceryhl@google.com> writes:
> 
> > On Wed, Mar 5, 2025 at 4:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> "Alice Ryhl" <aliceryhl@google.com> writes:
> >>
> >> > On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
> >> >>
> >> >> Hi Alice,
> >> >>
> >> >> On 250305 1339, Alice Ryhl wrote:
> >> >> > On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
> >> >> >
> >> >> > > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
> >> >> > > +    type Target = T;
> >> >> > > +
> >> >> > > +    fn deref(&self) -> &Self::Target {
> >> >> > > +        // SAFETY: The type invariants guarantee that the object is valid.
> >> >> > > +        unsafe { self.ptr.as_ref() }
> >> >> > > +    }
> >> >> > > +}
> >> >> >
> >> >> > What stops people from doing this?
> >> >> >
> >> >> > let my_unique: UniqueRef<T> = ...;
> >> >> > let my_ref: &T = &*my_unique;
> >> >> > let my_shared: ARef<T> = ARef::from(my_ref);
> >> >> >
> >> >> > Now it is no longer unique.
> >> >> >
> >> >> Oh, indeed. That's a serious problem. I see 2 options to deal with that:
> >> >>
> >> >> 1. remove ARef::From<&T>
> >> >>
> >> >> I checked the users of this, and it looks to me like there is rather
> >> >> a limited number and they are easy to fix by replacing the &T with ARef<T>.
> >> >> But I assume that wouldn't be welcome as it is intrusive nonetheless
> >> >> and of course there is ergonomic value in having the function around.
> >> >
> >> > Definitely not an option. There are many users of this function that
> >> > are in the process of being upstreamed. The ability to go &T ->
> >> > ARef<T> is pretty fundamental for ARef.
> >>
> >> Not having `impl From<&T> for UniqueArc` seems to work out fine.
> >>
> >> It would be unfortunate if `impl From<&T> for ARef<T>` would prevent us
> >> from having a unique version of `ARef`. I would say that is a valid
> >> reason to consider removing that impl.
> >
> > I think the impl is really important. It's required to do things such as:
> >
> > let mm = ARef::from(&*current!().mm());
> >
> > Without the impl (or something equivalent), it's not possible to
> > increment the refcount of the &Mm returned by `current!().mm()`. There
> > are many other examples of this.
> 
> Right. Let's see what we can figure out of other solutions then.

Ultimately, if a struct implements AlwaysRefcounted, then you can always
increments its refcount. If you want a version of the struct where that
is not the case, then you need a different struct that does *not*
implement AlwaysRefcounted.

I do things like that in the mm_struct series. The VmaNew struct is an
example of that.

Alice

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Mar 05, 2025 at 06:24:56PM +0100, Andreas Hindborg wrote:
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > On Wed, Mar 5, 2025 at 4:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> "Alice Ryhl" <aliceryhl@google.com> writes:
>> >>
>> >> > On Wed, Mar 5, 2025 at 3:56 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>> >> >>
>> >> >> Hi Alice,
>> >> >>
>> >> >> On 250305 1339, Alice Ryhl wrote:
>> >> >> > On Wed, Mar 05, 2025 at 11:31:44AM +0000, Oliver Mangold wrote:
>> >> >> >
>> >> >> > > +impl<T: UniqueRefCounted> Deref for UniqueRef<T> {
>> >> >> > > +    type Target = T;
>> >> >> > > +
>> >> >> > > +    fn deref(&self) -> &Self::Target {
>> >> >> > > +        // SAFETY: The type invariants guarantee that the object is valid.
>> >> >> > > +        unsafe { self.ptr.as_ref() }
>> >> >> > > +    }
>> >> >> > > +}
>> >> >> >
>> >> >> > What stops people from doing this?
>> >> >> >
>> >> >> > let my_unique: UniqueRef<T> = ...;
>> >> >> > let my_ref: &T = &*my_unique;
>> >> >> > let my_shared: ARef<T> = ARef::from(my_ref);
>> >> >> >
>> >> >> > Now it is no longer unique.
>> >> >> >
>> >> >> Oh, indeed. That's a serious problem. I see 2 options to deal with that:
>> >> >>
>> >> >> 1. remove ARef::From<&T>
>> >> >>
>> >> >> I checked the users of this, and it looks to me like there is rather
>> >> >> a limited number and they are easy to fix by replacing the &T with ARef<T>.
>> >> >> But I assume that wouldn't be welcome as it is intrusive nonetheless
>> >> >> and of course there is ergonomic value in having the function around.
>> >> >
>> >> > Definitely not an option. There are many users of this function that
>> >> > are in the process of being upstreamed. The ability to go &T ->
>> >> > ARef<T> is pretty fundamental for ARef.
>> >>
>> >> Not having `impl From<&T> for UniqueArc` seems to work out fine.
>> >>
>> >> It would be unfortunate if `impl From<&T> for ARef<T>` would prevent us
>> >> from having a unique version of `ARef`. I would say that is a valid
>> >> reason to consider removing that impl.
>> >
>> > I think the impl is really important. It's required to do things such as:
>> >
>> > let mm = ARef::from(&*current!().mm());
>> >
>> > Without the impl (or something equivalent), it's not possible to
>> > increment the refcount of the &Mm returned by `current!().mm()`. There
>> > are many other examples of this.
>>
>> Right. Let's see what we can figure out of other solutions then.
>
> Ultimately, if a struct implements AlwaysRefcounted, then you can always
> increments its refcount. If you want a version of the struct where that
> is not the case, then you need a different struct that does *not*
> implement AlwaysRefcounted.
>
> I do things like that in the mm_struct series. The VmaNew struct is an
> example of that.

Yea, I see your point. I think `AlwaysRefcounted` is just not going to
work for this use case. We can invent something new that suits our
needs.


Best regards,
Andreas Hindborg

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Oliver Mangold]

On 250306 0935, Alice Ryhl wrote:
> 
> Ultimately, if a struct implements AlwaysRefcounted, then you can always
> increments its refcount.

> If you want a version of the struct where that
> is not the case, then you need a different struct that does *not*
> implement AlwaysRefcounted.
>
I guess so, but it would be possible to make 'From<&T> for ARef<T>' opt-in,
by requiring a separate marker trait.

That you can call 'AlwaysRefCounted::inc_ref()' directly doesn't seem a
problem to me, as it will only leak the object, not create a reference.

A quick grep shows me that there are currently 7 implementers:

unsafe impl AlwaysRefCounted for Credential {
unsafe impl AlwaysRefCounted for File {
unsafe impl AlwaysRefCounted for LocalFile {
unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
unsafe impl crate::types::AlwaysRefCounted for Device {
unsafe impl crate::types::AlwaysRefCounted for Task {
unsafe impl AlwaysRefCounted for PidNamespace {

So it looks doable to me.

Oliver

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Alice Ryhl]

On Thu, Mar 06, 2025 at 10:14:05AM +0000, Oliver Mangold wrote:
> On 250306 0935, Alice Ryhl wrote:
> > 
> > Ultimately, if a struct implements AlwaysRefcounted, then you can always
> > increments its refcount.
> 
> > If you want a version of the struct where that
> > is not the case, then you need a different struct that does *not*
> > implement AlwaysRefcounted.
> >
> I guess so, but it would be possible to make 'From<&T> for ARef<T>' opt-in,
> by requiring a separate marker trait.
> 
> That you can call 'AlwaysRefCounted::inc_ref()' directly doesn't seem a
> problem to me, as it will only leak the object, not create a reference.
> 
> A quick grep shows me that there are currently 7 implementers:
> 
> unsafe impl AlwaysRefCounted for Credential {
> unsafe impl AlwaysRefCounted for File {
> unsafe impl AlwaysRefCounted for LocalFile {
> unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
> unsafe impl crate::types::AlwaysRefCounted for Device {
> unsafe impl crate::types::AlwaysRefCounted for Task {
> unsafe impl AlwaysRefCounted for PidNamespace {
> 
> So it looks doable to me.

How about this:

* Rename AlwaysRefCounted to RefCounted.
* Introduce a new AlwaysRefCounted trait with no methods and gate
  `From<&T>` on it. It has RefCounted as a sub-trait.
* Introduce an Ownable trait with an Owned type like in [1].
* Given an Owned<T> where T:RefCounted you can convert from Owned<T> to
  ARef<T>.

And there needs to be a safety requirement on Ownable or
AlwaysRefCounted which requires that a type cannot implement both
traits. Alternatively, if a type implements both, it needs to be safe to
have both Owned<T> and ARef<T> references at the same time, which could
make sense for a type that has one "special" reference and many normal
references.

If you want conversions ARef<T> to Owned<T>, you should add a new trait
TryIntoOwned that's a super-trait of both RefCounted and Owned and has
the `try` method for the conversion.

Thoughts?

Alice

[1]: https://lore.kernel.org/rust-for-linux/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Oliver Mangold]

On 250306 1131, Alice Ryhl wrote:
> 
> How about this:
> 
> * Rename AlwaysRefCounted to RefCounted.
> * Introduce a new AlwaysRefCounted trait with no methods and gate
>   `From<&T>` on it. It has RefCounted as a sub-trait.
> * Introduce an Ownable trait with an Owned type like in [1].
> * Given an Owned<T> where T:RefCounted you can convert from Owned<T> to
>   ARef<T>.
> 
> And there needs to be a safety requirement on Ownable or
> AlwaysRefCounted which requires that a type cannot implement both
> traits. Alternatively, if a type implements both, it needs to be safe to
> have both Owned<T> and ARef<T> references at the same time, which could
> make sense for a type that has one "special" reference and many normal
> references.
> 
> If you want conversions ARef<T> to Owned<T>, you should add a new trait
> TryIntoOwned that's a super-trait of both RefCounted and Owned and has
> the `try` method for the conversion.
> 
> Thoughts?
> 
Yes. Sounds good to me. Basically what I had in mind. Only the naming
is different.

I will build an implementation like this and post it as v5.
I won't change the names of UniqueRef and UniqueRefCounted for now,
but more out of laziness than because of having strong feelings about it.
I like UniqueRef a bit better as our focus is on pointing out that
it is unique. But if you or other prefers Owned I can change it.
> 
> [1]: https://lore.kernel.org/rust-for-linux/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/

Oh, really very similar.

Re: [PATCH v4] rust: adding UniqueRefCounted and UniqueRef types

[Alice Ryhl]

On Thu, Mar 6, 2025 at 1:03 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> On 250306 1131, Alice Ryhl wrote:
> >
> > How about this:
> >
> > * Rename AlwaysRefCounted to RefCounted.
> > * Introduce a new AlwaysRefCounted trait with no methods and gate
> >   `From<&T>` on it. It has RefCounted as a sub-trait.
> > * Introduce an Ownable trait with an Owned type like in [1].
> > * Given an Owned<T> where T:RefCounted you can convert from Owned<T> to
> >   ARef<T>.
> >
> > And there needs to be a safety requirement on Ownable or
> > AlwaysRefCounted which requires that a type cannot implement both
> > traits. Alternatively, if a type implements both, it needs to be safe to
> > have both Owned<T> and ARef<T> references at the same time, which could
> > make sense for a type that has one "special" reference and many normal
> > references.
> >
> > If you want conversions ARef<T> to Owned<T>, you should add a new trait
> > TryIntoOwned that's a super-trait of both RefCounted and Owned and has
> > the `try` method for the conversion.
> >
> > Thoughts?
> >
> Yes. Sounds good to me. Basically what I had in mind. Only the naming
> is different.
>
> I will build an implementation like this and post it as v5.
> I won't change the names of UniqueRef and UniqueRefCounted for now,
> but more out of laziness than because of having strong feelings about it.
> I like UniqueRef a bit better as our focus is on pointing out that
> it is unique. But if you or other prefers Owned I can change it.

Advantage of the Owned naming is that it also makes sense for types
that *only* support Owned pointers. The UniqueRef name kind of assumes
that it's refcounted, but the design I proposed does not have that
limitation.

Alice