rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rust: adding UniqueRefCounted and UniqueRef types
@ 2025-02-28 14:43 Oliver Mangold
  2025-02-28 15:21 ` Miguel Ojeda
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Oliver Mangold @ 2025-02-28 14:43 UTC (permalink / raw)
  To: Andreas Hindborg
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

Hi,

For usage with block-mq, we found that having a variant of ARef
which is guaranteed to be unique being useful.
As chances are it is useful in general, I implemented it
as kernel::types::UniqueRef.
The difference between ARef and UniqueRef
is basically the same as between Arc and UniqueArc.

This v2 of the patch, addressing the issues raised by Andreas Hindborg.

On 250228 1417, Miguel Ojeda wrote:
> 
> I think should be caught by Clippy -- Oliver, please try building with
> `CLIPPY=1` (we would like Clippy-clean builds as much as reasonably
> possible),

Got it. This version should be okay for rustfmt, clippy and checkpatch :)

Best regards,

Oliver

---
 rust/kernel/types.rs | 153 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 55ddd50e8aaa..72a973d9e1c7 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::unique_to_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,153 @@ 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`].
+/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
+/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
+/// - that the returned [`ARef`] expects for an object with a single reference
+///   in existence.
+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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
+    /// Converts the [`UniqueRef`] into an [`ARef`].
+    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;
+    /// 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) };
+    }
+}
+
+/// 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 references 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 `Sync` because
+// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
+// `T` to be `Send` because any thread that has an `UniqueRef<T>` may ultimately access `T` using a
+// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: UniqueRefCounted + Sync + 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`); additionally,
+// it needs `T` to be `Send` because any thread that has a `&UniqueRef<T>` may clone it and get an
+// `UniqueRef<T>` on that thread, so the thread may ultimately access `T`
+// using a mutable reference, for example, when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: UniqueRefCounted + Sync + Send> 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 [`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> From<&T> for UniqueRef<T> {
+    /// Converts the [`UniqueRef`] into an [`ARef`]
+    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
+    fn from(b: &T) -> Self {
+        b.inc_ref();
+        // SAFETY: We just incremented the refcount above.
+        unsafe { Self::from_raw(NonNull::from(b)) }
+    }
+}
+
+impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
+    type Error = ARef<T>;
+    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
+    /// by calling [`UniqueRefCounted::try_shared_to_unique`].
+    /// 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_shared_to_unique(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
-- 
2.48.1

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 14:43 [PATCH] rust: adding UniqueRefCounted and UniqueRef types Oliver Mangold
@ 2025-02-28 15:21 ` Miguel Ojeda
  2025-02-28 15:54 ` Boqun Feng
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Miguel Ojeda @ 2025-02-28 15:21 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

On Fri, Feb 28, 2025 at 3:43 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> This v2 of the patch, addressing the issues raised by Andreas Hindborg.

Hmm... Something went wrong here -- this patch looks broken, e.g. the
SoB is at the end, the title does not say v2, these replies seem to be
part of the patch, etc.

(Also, by the way, in case it helps, the message got marked as spam in
my side for some reason).

> Got it. This version should be okay for rustfmt, clippy and checkpatch :)

Thanks for doing that!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 14:43 [PATCH] rust: adding UniqueRefCounted and UniqueRef types Oliver Mangold
  2025-02-28 15:21 ` Miguel Ojeda
@ 2025-02-28 15:54 ` Boqun Feng
  2025-02-28 18:01 ` [PATCH v2] " Oliver Mangold
  2025-02-28 23:41 ` [PATCH] " Benoît du Garreau
  3 siblings, 0 replies; 16+ messages in thread
From: Boqun Feng @ 2025-02-28 15:54 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

Hi Oliver,

On Fri, Feb 28, 2025 at 02:43:03PM +0000, Oliver Mangold wrote:
> Hi,
> 
> For usage with block-mq, we found that having a variant of ARef
> which is guaranteed to be unique being useful.
> As chances are it is useful in general, I implemented it
> as kernel::types::UniqueRef.
> The difference between ARef and UniqueRef
> is basically the same as between Arc and UniqueArc.
> 

One meta comment: could you add examples for the usage of UniqueRef?
Ideally, we should have example code for all the new APIs, so that we at
least have some test payload on them (doctest will be generated as kunit
test).

Anyway, thanks for the patch!

Regards,
Boqun

> This v2 of the patch, addressing the issues raised by Andreas Hindborg.
> 
> On 250228 1417, Miguel Ojeda wrote:
> > 
> > I think should be caught by Clippy -- Oliver, please try building with
> > `CLIPPY=1` (we would like Clippy-clean builds as much as reasonably
> > possible),
> 
> Got it. This version should be okay for rustfmt, clippy and checkpatch :)
> 
> Best regards,
> 
> Oliver
> 
> ---
>  rust/kernel/types.rs | 153 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 55ddd50e8aaa..72a973d9e1c7 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::unique_to_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,153 @@ 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`].
> +/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
> +/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
> +/// - that the returned [`ARef`] expects for an object with a single reference
> +///   in existence.
> +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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
> +    /// Converts the [`UniqueRef`] into an [`ARef`].
> +    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;
> +    /// 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) };
> +    }
> +}
> +
> +/// 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 references 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 `Sync` because
> +// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> +// `T` to be `Send` because any thread that has an `UniqueRef<T>` may ultimately access `T` using a
> +// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: UniqueRefCounted + Sync + 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`); additionally,
> +// it needs `T` to be `Send` because any thread that has a `&UniqueRef<T>` may clone it and get an
> +// `UniqueRef<T>` on that thread, so the thread may ultimately access `T`
> +// using a mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: UniqueRefCounted + Sync + Send> 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 [`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> From<&T> for UniqueRef<T> {
> +    /// Converts the [`UniqueRef`] into an [`ARef`]
> +    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
> +    fn from(b: &T) -> Self {
> +        b.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(NonNull::from(b)) }
> +    }
> +}
> +
> +impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
> +    type Error = ARef<T>;
> +    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
> +    /// by calling [`UniqueRefCounted::try_shared_to_unique`].
> +    /// 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_shared_to_unique(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
> -- 
> 2.48.1
> 
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
@ 2025-02-28 16:06 Oliver Mangold
  2025-02-28 16:21 ` Boqun Feng
  2025-02-28 16:52 ` Miguel Ojeda
  0 siblings, 2 replies; 16+ messages in thread
From: Oliver Mangold @ 2025-02-28 16:06 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

> 
> Hmm... Something went wrong here -- this patch looks broken, e.g. the
> SoB is at the end, the title does not say v2, these replies seem to be
> part of the patch, etc.
>
Hi,

should I post it again with a fix, then?

> (Also, by the way, in case it helps, the message got marked as spam in
> my side for some reason).
> 

Cannot comment on that. The mysteries of a spam filter :)

On 250228 0754, Boqun Feng wrote:
> 
> One meta comment: could you add examples for the usage of UniqueRef?
> Ideally, we should have example code for all the new APIs, so that we at
> least have some test payload on them (doctest will be generated as kunit
> test).

For now I can point you to the implementation for block::mq::Request which
Andreas did on top of this. So there will shortly at least one actual user
of the API (which uses all the features, as I believe).

https://github.com/metaspace/linux/commit/797b90ae0f83b45495364d4c31651bca06471ef0

But if you still need a separate test, I can do one during the next couple
of days.

Best,

Oliver


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 16:06 Oliver Mangold
@ 2025-02-28 16:21 ` Boqun Feng
  2025-02-28 16:52 ` Miguel Ojeda
  1 sibling, 0 replies; 16+ messages in thread
From: Boqun Feng @ 2025-02-28 16:21 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Miguel Ojeda, Andreas Hindborg, Miguel Ojeda, Alex Gaynor,
	Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux

On Fri, Feb 28, 2025 at 04:06:01PM +0000, Oliver Mangold wrote:
> > 
> > Hmm... Something went wrong here -- this patch looks broken, e.g. the
> > SoB is at the end, the title does not say v2, these replies seem to be
> > part of the patch, etc.
> >
> Hi,
> 
> should I post it again with a fix, then?
> 
> > (Also, by the way, in case it helps, the message got marked as spam in
> > my side for some reason).
> > 
> 
> Cannot comment on that. The mysteries of a spam filter :)
> 
> On 250228 0754, Boqun Feng wrote:
> > 
> > One meta comment: could you add examples for the usage of UniqueRef?
> > Ideally, we should have example code for all the new APIs, so that we at
> > least have some test payload on them (doctest will be generated as kunit
> > test).
> 
> For now I can point you to the implementation for block::mq::Request which
> Andreas did on top of this. So there will shortly at least one actual user
> of the API (which uses all the features, as I believe).
> 

I meant the "# Examples" section in the function documentation, you can
see UniqueArc for example:

	https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/sync/arc.rs#n592

(in case I wasn't clear)

> https://github.com/metaspace/linux/commit/797b90ae0f83b45495364d4c31651bca06471ef0
> 
> But if you still need a separate test, I can do one during the next couple

Yes, we do because we are trying to maintain an API here, and we need at
least some unit tests to verify the correctness of an API. Thanks!

Regards,
Boqun

> of days.
> 
> Best,
> 
> Oliver
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 16:06 Oliver Mangold
  2025-02-28 16:21 ` Boqun Feng
@ 2025-02-28 16:52 ` Miguel Ojeda
  1 sibling, 0 replies; 16+ messages in thread
From: Miguel Ojeda @ 2025-02-28 16:52 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

On Fri, Feb 28, 2025 at 5:06 PM Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> should I post it again with a fix, then?

Yes, sorry, we need properly formatted patches.

If you are editing manually the patch, I would recommend testing if
you can apply the patch locally before submitting (and check that the
commit message and so on is correct).

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 14:43 [PATCH] rust: adding UniqueRefCounted and UniqueRef types Oliver Mangold
  2025-02-28 15:21 ` Miguel Ojeda
  2025-02-28 15:54 ` Boqun Feng
@ 2025-02-28 18:01 ` Oliver Mangold
  2025-02-28 18:09   ` Boqun Feng
                     ` (2 more replies)
  2025-02-28 23:41 ` [PATCH] " Benoît du Garreau
  3 siblings, 3 replies; 16+ messages in thread
From: Oliver Mangold @ 2025-02-28 18:01 UTC (permalink / raw)
  To: Andreas Hindborg
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

For usage with block-mq, a variant of ARef
which is guaranteed to be unique would be useful.
As chances are it is useful in general, This implements it
as kernel::types::UniqueRef.
The difference between ARef and UniqueRef
is basically the same as between Arc and UniqueArc.

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
---
 rust/kernel/types.rs | 153 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 55ddd50e8aaa..72a973d9e1c7 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::unique_to_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,153 @@ 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`].
+/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
+/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
+/// - that the returned [`ARef`] expects for an object with a single reference
+///   in existence.
+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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
+    /// Converts the [`UniqueRef`] into an [`ARef`].
+    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;
+    /// 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) };
+    }
+}
+
+/// 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 references 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 `Sync` because
+// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
+// `T` to be `Send` because any thread that has an `UniqueRef<T>` may ultimately access `T` using a
+// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: UniqueRefCounted + Sync + 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`); additionally,
+// it needs `T` to be `Send` because any thread that has a `&UniqueRef<T>` may clone it and get an
+// `UniqueRef<T>` on that thread, so the thread may ultimately access `T`
+// using a mutable reference, for example, when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: UniqueRefCounted + Sync + Send> 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 [`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> From<&T> for UniqueRef<T> {
+    /// Converts the [`UniqueRef`] into an [`ARef`]
+    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
+    fn from(b: &T) -> Self {
+        b.inc_ref();
+        // SAFETY: We just incremented the refcount above.
+        unsafe { Self::from_raw(NonNull::from(b)) }
+    }
+}
+
+impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
+    type Error = ARef<T>;
+    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
+    /// by calling [`UniqueRefCounted::try_shared_to_unique`].
+    /// 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_shared_to_unique(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
-- 
2.48.1

Best regards,

Oliver


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 18:01 ` [PATCH v2] " Oliver Mangold
@ 2025-02-28 18:09   ` Boqun Feng
  2025-02-28 18:16     ` Boqun Feng
  2025-02-28 18:29   ` Andreas Hindborg
  2025-03-03 14:09   ` [PATCH v2] " Andreas Hindborg
  2 siblings, 1 reply; 16+ messages in thread
From: Boqun Feng @ 2025-02-28 18:09 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

On Fri, Feb 28, 2025 at 06:01:55PM +0000, Oliver Mangold wrote:
> For usage with block-mq, a variant of ARef
> which is guaranteed to be unique would be useful.
> As chances are it is useful in general, This implements it
> as kernel::types::UniqueRef.
> The difference between ARef and UniqueRef
> is basically the same as between Arc and UniqueArc.
> 

Please add the "# Examples" section as I requested:

	https://lore.kernel.org/rust-for-linux/Z8HcHVtPiG-X6ujP@Mac.home/

I also would like to know why do you think it's OK to ignore my previous
comment, thanks!

Regards,
Boqun

> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> ---
>  rust/kernel/types.rs | 153 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 55ddd50e8aaa..72a973d9e1c7 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::unique_to_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,153 @@ 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`].
> +/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
> +/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
> +/// - that the returned [`ARef`] expects for an object with a single reference
> +///   in existence.
> +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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
> +    /// Converts the [`UniqueRef`] into an [`ARef`].
> +    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;
> +    /// 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) };
> +    }
> +}
> +
> +/// 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 references 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 `Sync` because
> +// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> +// `T` to be `Send` because any thread that has an `UniqueRef<T>` may ultimately access `T` using a
> +// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: UniqueRefCounted + Sync + 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`); additionally,
> +// it needs `T` to be `Send` because any thread that has a `&UniqueRef<T>` may clone it and get an
> +// `UniqueRef<T>` on that thread, so the thread may ultimately access `T`
> +// using a mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: UniqueRefCounted + Sync + Send> 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 [`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> From<&T> for UniqueRef<T> {
> +    /// Converts the [`UniqueRef`] into an [`ARef`]
> +    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
> +    fn from(b: &T) -> Self {
> +        b.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(NonNull::from(b)) }
> +    }
> +}
> +
> +impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
> +    type Error = ARef<T>;
> +    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
> +    /// by calling [`UniqueRefCounted::try_shared_to_unique`].
> +    /// 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_shared_to_unique(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
> -- 
> 2.48.1
> 
> Best regards,
> 
> Oliver
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 18:09   ` Boqun Feng
@ 2025-02-28 18:16     ` Boqun Feng
  0 siblings, 0 replies; 16+ messages in thread
From: Boqun Feng @ 2025-02-28 18:16 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

On Fri, Feb 28, 2025 at 10:09:11AM -0800, Boqun Feng wrote:
> On Fri, Feb 28, 2025 at 06:01:55PM +0000, Oliver Mangold wrote:
> > For usage with block-mq, a variant of ARef
> > which is guaranteed to be unique would be useful.
> > As chances are it is useful in general, This implements it
> > as kernel::types::UniqueRef.
> > The difference between ARef and UniqueRef
> > is basically the same as between Arc and UniqueArc.
> > 
> 
> Please add the "# Examples" section as I requested:
> 
> 	https://lore.kernel.org/rust-for-linux/Z8HcHVtPiG-X6ujP@Mac.home/
> 

More particularly, I gave an example here:

	https://lore.kernel.org/rust-for-linux/Z8HikJloVjoZXBbc@Mac.home/	

maybe I wasn't clear, sorry about that. I meant to say you need to
include the examples with the introduction of the API, and that provides
us the sample code and the tests at the first day of the new API.

Regards,
Boqun

> I also would like to know why do you think it's OK to ignore my previous
> comment, thanks!
> 
> Regards,
> Boqun
> 
> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> > ---
> >  rust/kernel/types.rs | 153 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 153 insertions(+)
> > 
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index 55ddd50e8aaa..72a973d9e1c7 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::unique_to_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,153 @@ 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`].
> > +/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
> > +/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
> > +/// - that the returned [`ARef`] expects for an object with a single reference
> > +///   in existence.
> > +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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
> > +    /// Converts the [`UniqueRef`] into an [`ARef`].
> > +    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;
> > +    /// 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) };
> > +    }
> > +}
> > +
> > +/// 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 references 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 `Sync` because
> > +// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> > +// `T` to be `Send` because any thread that has an `UniqueRef<T>` may ultimately access `T` using a
> > +// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> > +unsafe impl<T: UniqueRefCounted + Sync + 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`); additionally,
> > +// it needs `T` to be `Send` because any thread that has a `&UniqueRef<T>` may clone it and get an
> > +// `UniqueRef<T>` on that thread, so the thread may ultimately access `T`
> > +// using a mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> > +unsafe impl<T: UniqueRefCounted + Sync + Send> 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 [`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> From<&T> for UniqueRef<T> {
> > +    /// Converts the [`UniqueRef`] into an [`ARef`]
> > +    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
> > +    fn from(b: &T) -> Self {
> > +        b.inc_ref();
> > +        // SAFETY: We just incremented the refcount above.
> > +        unsafe { Self::from_raw(NonNull::from(b)) }
> > +    }
> > +}
> > +
> > +impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
> > +    type Error = ARef<T>;
> > +    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
> > +    /// by calling [`UniqueRefCounted::try_shared_to_unique`].
> > +    /// 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_shared_to_unique(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
> > -- 
> > 2.48.1
> > 
> > Best regards,
> > 
> > Oliver
> > 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 18:01 ` [PATCH v2] " Oliver Mangold
  2025-02-28 18:09   ` Boqun Feng
@ 2025-02-28 18:29   ` Andreas Hindborg
  2025-03-03 13:29     ` [PATCH v3] " Oliver Mangold
  2025-03-03 14:09   ` [PATCH v2] " Andreas Hindborg
  2 siblings, 1 reply; 16+ messages in thread
From: Andreas Hindborg @ 2025-02-28 18:29 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

"Oliver Mangold" <oliver.mangold@pm.me> writes:

> For usage with block-mq, a variant of ARef
> which is guaranteed to be unique would be useful.
> As chances are it is useful in general, This implements it
> as kernel::types::UniqueRef.
> The difference between ARef and UniqueRef
> is basically the same as between Arc and UniqueArc.

I would suggest splitting out the use case from the commit message:

  rust: add `UniqueRef` for `ARef` references

  Add `UniqeRef` as a variabt 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.

  ---

  Upcomming changes to the rust block device driver API depend on this
  patch. Please see [1] for a use casel.

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


The comments after the --- will not got in the commit log, they are a
message for the review process only.


Best regards,
Andreas Hindborg



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 14:43 [PATCH] rust: adding UniqueRefCounted and UniqueRef types Oliver Mangold
                   ` (2 preceding siblings ...)
  2025-02-28 18:01 ` [PATCH v2] " Oliver Mangold
@ 2025-02-28 23:41 ` Benoît du Garreau
  2025-03-01  8:06   ` Oliver Mangold
  3 siblings, 1 reply; 16+ messages in thread
From: Benoît du Garreau @ 2025-02-28 23:41 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Benoît du Garreau, Andreas Hindborg, Miguel Ojeda,
	Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Alice Ryhl, Trevor Gross, linux-kernel,
	rust-for-linux

From: Benoît du Garreau <bdgdlm@outlook.com>

On Fri, 28 Feb 2025 14:43:03 +0000 Oliver Mangold <oliver.mangold@pm.me> wrote:

> Hi,
> 
> For usage with block-mq, we found that having a variant of ARef
> which is guaranteed to be unique being useful.
> As chances are it is useful in general, I implemented it
> as kernel::types::UniqueRef.
> The difference between ARef and UniqueRef
> is basically the same as between Arc and UniqueArc.
> 
> This v2 of the patch, addressing the issues raised by Andreas Hindborg.
> 
> On 250228 1417, Miguel Ojeda wrote:
> > 
> > I think should be caught by Clippy -- Oliver, please try building with
> > `CLIPPY=1` (we would like Clippy-clean builds as much as reasonably
> > possible),
> 
> Got it. This version should be okay for rustfmt, clippy and checkpatch :)
> 
> Best regards,
> 
> Oliver
> 
> ---
>  rust/kernel/types.rs | 153 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 55ddd50e8aaa..72a973d9e1c7 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::unique_to_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,153 @@ 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`].
> +/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
> +/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
> +/// - that the returned [`ARef`] expects for an object with a single reference
> +///   in existence.
> +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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
> +    /// Converts the [`UniqueRef`] into an [`ARef`].
> +    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;
> +    /// 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) };
> +    }
> +}

It would be great for this trait to only have a `is_unique` method, and that functions here
do the actual work. It would make it easier to implement and would avoid duplicating this
work.
Maybe this could even be a new method on `AlwaysRefCounted`? 

> +
> +/// 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 references to the underlying object exist while the [`UniqueRef`] is live.

I think you meant "no other refcount" or "only references borrowed from this".

> +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 `Sync` because
> +// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> +// `T` to be `Send` because any thread that has an `UniqueRef<T>` may ultimately access `T` using a
> +// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: UniqueRefCounted + Sync + Send> Send for UniqueRef<T> {}

`UniqueRef` is essentially a `Box`, so it should have the same `Send`/`Sync` implementations. Here
I don't see how sending a `UniqueRef<T>` is sharing a `&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`); additionally,
> +// it needs `T` to be `Send` because any thread that has a `&UniqueRef<T>` may clone it and get an
> +// `UniqueRef<T>` on that thread, so the thread may ultimately access `T`
> +// using a mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: UniqueRefCounted + Sync + Send> Sync for UniqueRef<T> {}

Same here: you can only get a `&T` from a `UniqueRef<T>`, definitely not clone it.

> +
> +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 [`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> From<&T> for UniqueRef<T> {
> +    /// Converts the [`UniqueRef`] into an [`ARef`]
> +    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
> +    fn from(b: &T) -> Self {
> +        b.inc_ref();
> +        // SAFETY: We just incremented the refcount above.
> +        unsafe { Self::from_raw(NonNull::from(b)) }
> +    }
> +}

This is wrong: the reference borrows from a refcount (as per `AlwaysRefCounted`), and this
method increments it once more. It cannot be unique when the function returns.
Actually the only way such conversion could be written is by cloning `T`, which is probably
not what we want.

> +
> +impl<T: UniqueRefCounted> TryFrom<ARef<T>> for UniqueRef<T> {
> +    type Error = ARef<T>;
> +    /// Tries to convert the [`ARef`] to an [`UniqueRef`]
> +    /// by calling [`UniqueRefCounted::try_shared_to_unique`].
> +    /// 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_shared_to_unique(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
> -- 
> 2.48.1
> 
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>

Benoît du Garreau

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 23:41 ` [PATCH] " Benoît du Garreau
@ 2025-03-01  8:06   ` Oliver Mangold
  0 siblings, 0 replies; 16+ messages in thread
From: Oliver Mangold @ 2025-03-01  8:06 UTC (permalink / raw)
  To: Benoît du Garreau
  Cc: Benoît du Garreau, Andreas Hindborg, Miguel Ojeda,
	Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Alice Ryhl, Trevor Gross, linux-kernel,
	rust-for-linux

On 250301 0041, Benoît du Garreau wrote:
> From: Benoît du Garreau <bdgdlm@outlook.com>
> 
> It would be great for this trait to only have a `is_unique` method, and that functions here
> do the actual work. It would make it easier to implement and would avoid duplicating this
> work.
Hi,

I see where you are coming from, but there are good reasons for it to be like this.

mq::Request (for which this work was initially done) has the surprising property,
that it is possible to create a reference to it 'out of band'
(i.e. without deriving it from an existing one) through mq::TagSet::tag_to_rq().

As this means try_shared_to_unique() to an UniqueRef can race
with tag_to_rq(), a non-standard reference counting scheme needs to be used
which can distinguish between 'a single ARef exists' and 'an UniqueRef exists',
and requires the is_unique() check and the necessary modification of the
reference count in try_shared_to_unique() have to be done
in one combined atomic operation.

Of course the whole thing could be refactored that both ways work, this one
and one like you suggest with just a to_unique(). I will think about it
for a bit, but I'm not sure it is worth the effort.

What probably makes sense is to at least have a default implementation
for unique_to_shared() which just does a
ARef::from_raw(UniqueRef::into_raw()) leaving only try_shared_to_unique()
as mandatory to implemented.

Thoughts?
> Maybe this could even be a new method on `AlwaysRefCounted`?
>
I didn't do that intentionally, because I think for many AlwaysRefCounted
implementors it would be a pain, as they simply use some get() and put()
methods to inc/dec the refcount which are provided by the C API,
while the actual refcount stays hidden from them.
> 
> I think you meant "no other refcount" or "only references borrowed from this".
>
Yes, you are right. Thanks. I will fix that.
> 
> `UniqueRef` is essentially a `Box`, so it should have the same `Send`/`Sync` implementations. Here
> I don't see how sending a `UniqueRef<T>` is sharing a `&T`.
> 
> Same here: you can only get a `&T` from a `UniqueRef<T>`, definitely not clone it.
>
I just copy/pasted that from ARef. You might be right, I have to think about that when
my minds is less foggy than right now :)
> > +
> > +
> > +impl<T: UniqueRefCounted> From<&T> for UniqueRef<T> {
> > +    /// Converts the [`UniqueRef`] into an [`ARef`]
> > +    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
> > +    fn from(b: &T) -> Self {
> > +        b.inc_ref();
> > +        // SAFETY: We just incremented the refcount above.
> > +        unsafe { Self::from_raw(NonNull::from(b)) }
> > +    }
> > +}
> 
> This is wrong: the reference borrows from a refcount (as per `AlwaysRefCounted`), and this
> method increments it once more. It cannot be unique when the function returns.
> Actually the only way such conversion could be written is by cloning `T`, which is probably
> not what we want.
> 
Good catch. That was also an relict of copy-paste which I missed. The method should
just be deleted. I will do that.

Best,

Oliver


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 18:29   ` Andreas Hindborg
@ 2025-03-03 13:29     ` Oliver Mangold
  2025-03-03 14:22       ` Andreas Hindborg
  0 siblings, 1 reply; 16+ messages in thread
From: Oliver Mangold @ 2025-03-03 13:29 UTC (permalink / raw)
  To: Andreas Hindborg
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

From 5bdbcd54855fed6ad9ae6bcc53dba3aab2c6c6b1 Mon Sep 17 00:00:00 2001
From: Oliver Mangold <oliver.mangold@pm.me>
Date: Fri, 21 Feb 2025 08:36:46 +0100
Subject: [PATCH] rust: adding UniqueRefCounted and UniqueRef types

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>
---
 rust/kernel/types.rs | 315 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 315 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 55ddd50e8aaa..7ea0a266caa5 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::unique_to_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.
+/// - [`unique_to_shared()`](UniqueRefCounted::unique_to_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 [`unique_to_shared()`](UniqueRefCounted::unique_to_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_shared_to_unique(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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;
+    /// Converts the [`UniqueRef`] into an [`ARef`].
+    fn unique_to_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_shared_to_unique(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_shared_to_unique()`](UniqueRefCounted::try_shared_to_unique).
+    /// 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_shared_to_unique(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
-- 

This should address all issues that have been raised with v2:

- Added a default implementation for unique_to_shared() which does a simple
  rewrap of the underlying object.
- Added a SimpleUniqueRefCounted trait which requires only to implement
  is_unique() as Benoît asked for. Maybe the feature is not worth
  the extra code, though. For me keeping it or removing would be both fine.
- Removed the unsound conversion from &T to UniqueRef, as spotted by Benoît.
- Relaxed the requirements for Send and Sync, to be identical to the ones
  for Box. See comment below.
- Added Examples for both UniqueRefCounted and SimpleUniqueRefCounted
  as asked for by Boqun Feng.
  For me they compile and run without errors as KUnits.
- Changed the commit message like suggested by Andreas.

@Benoît: I think you are right about Send and Sync.
What gave me a bit of a headache is if Send really does not require
the underlying object to be Sync, as the refcount itself -
which is part of the object - might be touched concurrently in a case
like with tag_to_req(), but I think one would not implement
something like that without having a synchronized refcount.

Best regards,

Oliver


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] rust: adding UniqueRefCounted and UniqueRef types
  2025-02-28 18:01 ` [PATCH v2] " Oliver Mangold
  2025-02-28 18:09   ` Boqun Feng
  2025-02-28 18:29   ` Andreas Hindborg
@ 2025-03-03 14:09   ` Andreas Hindborg
  2 siblings, 0 replies; 16+ messages in thread
From: Andreas Hindborg @ 2025-03-03 14:09 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

"Oliver Mangold" <oliver.mangold@pm.me> writes:

[...]

> +/// 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`].
> +/// - [`UniqueRefCounted::dec_ref`] correctly frees the underlying object.
> +/// - [`UniqueRefCounted::unique_to_shared`] set the reference count to the value
> +/// - that the returned [`ARef`] expects for an object with a single reference
> +///   in existence.
> +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_shared_to_unique(this: ARef<Self>) -> Result<UniqueRef<Self>, ARef<Self>>;

This could just be `try_into_unique`, since the type of `this` gives the
rest of the context.

> +    /// Converts the [`UniqueRef`] into an [`ARef`].
> +    fn unique_to_shared(this: UniqueRef<Self>) -> ARef<Self>;

Similarly, this could be `into_shared`.


Best regards,
Andreas Hindborg



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] rust: adding UniqueRefCounted and UniqueRef types
  2025-03-03 13:29     ` [PATCH v3] " Oliver Mangold
@ 2025-03-03 14:22       ` Andreas Hindborg
  2025-03-03 16:33         ` Oliver Mangold
  0 siblings, 1 reply; 16+ messages in thread
From: Andreas Hindborg @ 2025-03-03 14:22 UTC (permalink / raw)
  To: Oliver Mangold
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

"Oliver Mangold" <oliver.mangold@pm.me> writes:

> From: Oliver Mangold <oliver.mangold@pm.me>
> Date: Fri, 21 Feb 2025 08:36:46 +0100
> Subject: [PATCH] rust: adding UniqueRefCounted and UniqueRef types
>
> 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>


This part:


    This should address all issues that have been raised with v2:

    - Added a default implementation for unique_to_shared() which does a simple
      rewrap of the underlying object.
    - Added a SimpleUniqueRefCounted trait which requires only to implement
      is_unique() as Benoît asked for. Maybe the feature is not worth
      the extra code, though. For me keeping it or removing would be both fine.
    - Removed the unsound conversion from &T to UniqueRef, as spotted by Benoît.
    - Relaxed the requirements for Send and Sync, to be identical to the ones
      for Box. See comment below.
    - Added Examples for both UniqueRefCounted and SimpleUniqueRefCounted
      as asked for by Boqun Feng.
      For me they compile and run without errors as KUnits.
    - Changed the commit message like suggested by Andreas.

    @Benoît: I think you are right about Send and Sync.
    What gave me a bit of a headache is if Send really does not require
    the underlying object to be Sync, as the refcount itself -
    which is part of the object - might be touched concurrently in a case
    like with tag_to_req(), but I think one would not implement
    something like that without having a synchronized refcount.

    Best regards,

    Oliver


Goes immedieatly after the cut like so:


    From: Oliver Mangold <oliver.mangold@pm.me>
    Date: Fri, 21 Feb 2025 08:36:46 +0100
    Subject: [PATCH] rust: adding UniqueRefCounted and UniqueRef types

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


    This should address all issues that have been raised with v2:

    - Added a default implementation for unique_to_shared() which does a simple
      rewrap of the underlying object.
    - Added a SimpleUniqueRefCounted trait which requires only to implement
      is_unique() as Benoît asked for. Maybe the feature is not worth
      the extra code, though. For me keeping it or removing would be both fine.
    - Removed the unsound conversion from &T to UniqueRef, as spotted by Benoît.
    - Relaxed the requirements for Send and Sync, to be identical to the ones
      for Box. See comment below.
    - Added Examples for both UniqueRefCounted and SimpleUniqueRefCounted
      as asked for by Boqun Feng.
      For me they compile and run without errors as KUnits.
    - Changed the commit message like suggested by Andreas.

    @Benoît: I think you are right about Send and Sync.
    What gave me a bit of a headache is if Send really does not require
    the underlying object to be Sync, as the refcount itself -
    which is part of the object - might be touched concurrently in a case
    like with tag_to_req(), but I think one would not implement
    something like that without having a synchronized refcount.

    Best regards,

    Oliver

    ---

    rust/kernel/types.rs | 315 +++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 315 insertions(+)


Again, I'll recommend the use of b4 [1].

Best regards,
Andreas Hindborg


[1] https://b4.docs.kernel.org/en/latest/contributor/prep.html


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] rust: adding UniqueRefCounted and UniqueRef types
  2025-03-03 14:22       ` Andreas Hindborg
@ 2025-03-03 16:33         ` Oliver Mangold
  0 siblings, 0 replies; 16+ messages in thread
From: Oliver Mangold @ 2025-03-03 16:33 UTC (permalink / raw)
  To: Andreas Hindborg
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

On 250303 1522, Andreas Hindborg wrote:
> 
> This part:
> 
> Goes immedieatly after the cut like so:
>
Hi Andreas,

sorry, again. I misunderstood what 'after the marker' means.
But as I now tried all orderings, I should get it right from now on :)

> Again, I'll recommend the use of b4 [1].
>

Might make sense. I will look into it,

Oliver


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2025-03-03 16:34 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-28 14:43 [PATCH] rust: adding UniqueRefCounted and UniqueRef types Oliver Mangold
2025-02-28 15:21 ` Miguel Ojeda
2025-02-28 15:54 ` Boqun Feng
2025-02-28 18:01 ` [PATCH v2] " Oliver Mangold
2025-02-28 18:09   ` Boqun Feng
2025-02-28 18:16     ` Boqun Feng
2025-02-28 18:29   ` Andreas Hindborg
2025-03-03 13:29     ` [PATCH v3] " Oliver Mangold
2025-03-03 14:22       ` Andreas Hindborg
2025-03-03 16:33         ` Oliver Mangold
2025-03-03 14:09   ` [PATCH v2] " Andreas Hindborg
2025-02-28 23:41 ` [PATCH] " Benoît du Garreau
2025-03-01  8:06   ` Oliver Mangold
  -- strict thread matches above, loose matches on Subject: below --
2025-02-28 16:06 Oliver Mangold
2025-02-28 16:21 ` Boqun Feng
2025-02-28 16:52 ` Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).