rust/kernel/block/mq/request.rs | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------ 1 file changed, 61 insertions(+), 36 deletions(-)
For usage with block-mq, we found that having a variant of ARef which is guaranteed to be unique is useful. As chances are it is useful in general, I implemented it as kernel::types::URef. The difference between ARef and URef is basically the same as between Arc and UniqueArc.
Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
---
rust/kernel/block/mq/request.rs | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------
1 file changed, 61 insertions(+), 36 deletions(-)
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 55ddd50e8aaa..80dc9edef1b9 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -543,6 +543,12 @@ fn from(b: &T) -> Self {
}
}
+impl<T: UniqueRefCounted> From<URef<T>> for ARef<T> {
+ fn from(b: URef<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,119 @@ fn drop(&mut self) {
}
}
+/// Types that are `AlwaysRefCounted` and can be safely converted to an `URef`
+///
+pub unsafe trait UniqueRefCounted: AlwaysRefCounted + Sized {
+ /// Checks if the the [ARef] is unique and convert it
+ /// to an [URef] 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<URef<Self>,ARef<Self>>;
+ /// Converts the [URef] into an [ARef].
+ fn unique_to_shared(this: URef<Self>) -> ARef<Self>;
+}
+
+/// An unique owned reference to an always-reference-counted 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 [`ARef`] instance. In
+/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
+pub struct URef<T: UniqueRefCounted> {
+ ptr: NonNull<T>,
+ _p: PhantomData<T>,
+}
+
+// SAFETY: It is safe to send `URef<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 `URef<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 URef<T> {}
+
+// SAFETY: It is safe to send `&URef<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 `&URef<T>` may clone it and get an
+// `URef<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 URef<T> {}
+
+impl<T: UniqueRefCounted> URef<T> {
+ /// Creates a new instance of [`URef`].
+ ///
+ /// 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 AlwaysRefCounted::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 [`URef`].
+ 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 `URef`, 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 `URef`.
+ pub fn into_raw(me: Self) -> NonNull<T> {
+ ManuallyDrop::new(me).ptr
+ }
+}
+
+impl<T: UniqueRefCounted> Deref for URef<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 URef<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 URef<T> {
+ /// Converts the [URef] 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 URef<T> {
+ type Error = ARef<T>;
+ /// Tries to convert the [ARef] to an [URef] 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<URef<T>,Self::Error> {
+ UniqueRefCounted::try_shared_to_unique(b)
+ }
+}
+
+impl<T: UniqueRefCounted> Drop for URef<T> {
+ fn drop(&mut self) {
+ // SAFETY: The type invariants guarantee that the `URef` owns the reference we're about to
+ // decrement.
+ unsafe { T::dec_ref(self.ptr) };
+ }
+}
+
/// A sum type that always holds either a value of type `L` or `R`.
///
/// # Examples
---Hi Oliver,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rust/rust-next]
[also build test WARNING on linus/master v6.14-rc5 next-20250303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oliver-Mangold/Rust-Implement-a-unique-reference-type-URef-supplementing-ARef/20250221-160625
base: https://github.com/Rust-for-Linux/linux rust-next
patch link: https://lore.kernel.org/r/SpXhwnZO__ST8sHQ3HQ3ygThOcnmn0x2JlJ_nwJglL87vw5XfQA8sDH6HdkrGgFVycLhPBlCc6-UtEImTvY26A%3D%3D%40protonmail.internalid
patch subject: [PATCH] Rust: Implement a unique reference type URef supplementing ARef
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250304/202503041325.UnOn2m7a-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250304/202503041325.UnOn2m7a-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503041325.UnOn2m7a-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> warning: docs for unsafe trait missing `# Safety` section
--> rust/kernel/types.rs:546:1
|
546 | pub unsafe trait UniqueRefCounted: AlwaysRefCounted + Sized {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
= note: `-W clippy::missing-safety-doc` implied by `-W clippy::all`
= help: to override `-W clippy::all` add `#[allow(clippy::missing_safety_doc)]`
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Oliver,
"Oliver Mangold" <oliver.mangold@pm.me> writes:
> For usage with block-mq, we found that having a variant of ARef which is guaranteed to be unique is useful. As chances are it is useful in general, I implemented it as kernel::types::URef. The difference between ARef and URef is basically the same as between Arc and UniqueArc.
Wrap at 75 characters please :)
>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> ---
> rust/kernel/block/mq/request.rs | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------
> 1 file changed, 61 insertions(+), 36 deletions(-)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 55ddd50e8aaa..80dc9edef1b9 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -543,6 +543,12 @@ fn from(b: &T) -> Self {
> }
> }
>
> +impl<T: UniqueRefCounted> From<URef<T>> for ARef<T> {
> + fn from(b: URef<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,119 @@ fn drop(&mut self) {
> }
> }
>
> +/// Types that are `AlwaysRefCounted` and can be safely converted to an `URef`
> +///
When the trait is unsafe, we need to specify the conditions under which
it is safe to implement:
# Safety
- Requirement 1
- Requirement 2
- etc
See `AlwaysRefCounted` for reference on formatting.
> +pub unsafe trait UniqueRefCounted: AlwaysRefCounted + Sized {
> + /// Checks if the the [ARef] is unique and convert it
> + /// to an [URef] it that is that case.
Please use back ticks for types: [`ARef`], [`URref`]
> + /// Otherwise it returns again an [ARef] to the same
> + /// underlying object.
> + fn try_shared_to_unique(this: ARef<Self>) -> Result<URef<Self>,ARef<Self>>;
> + /// Converts the [URef] into an [ARef].
> + fn unique_to_shared(this: URef<Self>) -> ARef<Self>;
> +}
> +
> +/// An unique owned reference to an always-reference-counted 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 [`ARef`] instance. In
> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> +pub struct URef<T: UniqueRefCounted> {
> + ptr: NonNull<T>,
> + _p: PhantomData<T>,
> +}
> +
> +// SAFETY: It is safe to send `URef<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 `URef<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 URef<T> {}
> +
> +// SAFETY: It is safe to send `&URef<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 `&URef<T>` may clone it and get an
> +// `URef<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 URef<T> {}
> +
> +impl<T: UniqueRefCounted> URef<T> {
I would prefer `UniqueRef`. I know `ARef` has a different naming scheme,
but I think `UniqueRef` is sufficiently short and significantly more
descriptive than `URef`.
> + /// Creates a new instance of [`URef`].
> + ///
> + /// 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 AlwaysRefCounted::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 [`URef`].
> + 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 `URef`, 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 `URef`.
> + pub fn into_raw(me: Self) -> NonNull<T> {
> + ManuallyDrop::new(me).ptr
> + }
> +}
> +
> +impl<T: UniqueRefCounted> Deref for URef<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 URef<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 URef<T> {
> + /// Converts the [URef] 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 URef<T> {
> + type Error = ARef<T>;
> + /// Tries to convert the [ARef] to an [URef] 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<URef<T>,Self::Error> {
> + UniqueRefCounted::try_shared_to_unique(b)
> + }
> +}
> +
> +impl<T: UniqueRefCounted> Drop for URef<T> {
> + fn drop(&mut self) {
> + // SAFETY: The type invariants guarantee that the `URef` owns the reference we're about to
> + // decrement.
> + unsafe { T::dec_ref(self.ptr) };
> + }
> +}
> +
> /// A sum type that always holds either a value of type `L` or `R`.
> ///
> /// # Examples
> ---
For your next version, can you run `make rustfmt`?:
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 58556d417cae..49903fd0446e 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -542,13 +542,12 @@ fn drop(&mut self) {
}
/// Types that are `AlwaysRefCounted` and can be safely converted to an `URef`
-///
pub unsafe trait UniqueRefCounted: AlwaysRefCounted + Sized {
/// Checks if the the [ARef] is unique and convert it
/// to an [URef] 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<URef<Self>,ARef<Self>>;
+ fn try_shared_to_unique(this: ARef<Self>) -> Result<URef<Self>, ARef<Self>>;
/// Converts the [URef] into an [ARef].
fn unique_to_shared(this: URef<Self>) -> ARef<Self>;
}
@@ -620,7 +619,6 @@ fn deref(&self) -> &Self::Target {
}
impl<T: UniqueRefCounted> DerefMut for URef<T> {
-
fn deref_mut(&mut self) -> &mut Self::Target {
// SAFETY: The type invariants guarantee that the object is valid.
unsafe { self.ptr.as_mut() }
@@ -638,10 +636,10 @@ fn from(b: &T) -> Self {
impl<T: UniqueRefCounted> TryFrom<ARef<T>> for URef<T> {
type Error = ARef<T>;
- /// Tries to convert the [ARef] to an [URef] 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<URef<T>,Self::Error> {
+ /// Tries to convert the [ARef] to an [URef] 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<URef<T>, Self::Error> {
UniqueRefCounted::try_shared_to_unique(b)
}
}
Also it would be great if you include your "rust: for fix dec_ref for
URef<Request>" folded in.
Best regards,
Andreas Hindborg
On Fri, Feb 28, 2025 at 12:11 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> When the trait is unsafe, we need to specify the conditions under which
> it is safe to implement:
>
> # Safety
>
> - Requirement 1
> - Requirement 2
> - etc
>
> See `AlwaysRefCounted` for reference on formatting.
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), you should be getting something like:
error: docs for unsafe trait missing `# Safety` section
--> rust/........rs:23:1
|
23 | pub unsafe trait A {}
| ^^^^^^^^^^^^^^^^^^
|
= help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
= note: `-D clippy::missing-safety-doc` implied by `-D warnings`
= help: to override `-D warnings` add
`#[allow(clippy::missing_safety_doc)]`
Thanks!
Cheers,
Miguel
On 250228 1210, Andreas Hindborg wrote:
Hi Andreas,
> Wrap at 75 characters please :)
> See `AlwaysRefCounted` for reference on formatting.
> Please use back ticks for types: [`ARef`], [`URref`]
> For your next version, can you run `make rustfmt`?:
Sorry for all that. Apparently I need to get used a bit more to how things
are supposed to be done here. Please be patient with me for a bit :)
> When the trait is unsafe, we need to specify the conditions under which
> it is safe to implement:
Of course. Sorry, missed that.
> I would prefer `UniqueRef`. I know `ARef` has a different naming scheme,
> but I think `UniqueRef` is sufficiently short and significantly more
> descriptive than `URef`.
Ok, will do. Honestly I also prefer UniqueRef.
> Also it would be great if you include your "rust: for fix dec_ref for
> URef<Request>" folded in.
Are your sure? Wouldn't the patches have to be ordered like this?
this patch (mine)
rust: block: change `queue_rq` request type to unique (yours)
rust: block: simplify reference counting scheme (yours)
rust: for fix dec_ref for URef<Request> (mine)
IOW, the latter requests depends on the 2 patches of yours, while these depend on this patch. You know what I mean?
Best regards,
Oliver
"Oliver Mangold" <oliver.mangold@pm.me> writes: > On 250228 1210, Andreas Hindborg wrote: > > Hi Andreas, > >> Wrap at 75 characters please :) > >> See `AlwaysRefCounted` for reference on formatting. > >> Please use back ticks for types: [`ARef`], [`URref`] > >> For your next version, can you run `make rustfmt`?: > > Sorry for all that. Apparently I need to get used a bit more to how things > are supposed to be done here. Please be patient with me for a bit :) > >> When the trait is unsafe, we need to specify the conditions under which >> it is safe to implement: > > Of course. Sorry, missed that. > >> I would prefer `UniqueRef`. I know `ARef` has a different naming scheme, >> but I think `UniqueRef` is sufficiently short and significantly more >> descriptive than `URef`. > > Ok, will do. Honestly I also prefer UniqueRef. > >> Also it would be great if you include your "rust: for fix dec_ref for >> URef<Request>" folded in. > > Are your sure? Wouldn't the patches have to be ordered like this? > > this patch (mine) > rust: block: change `queue_rq` request type to unique (yours) > rust: block: simplify reference counting scheme (yours) > rust: for fix dec_ref for URef<Request> (mine) > > IOW, the latter requests depends on the 2 patches of yours, while these depend on this patch. You know what I mean? Sorry, yes I meant "rust: allow to override dec_ref() for URef's". Either include it in the series or fold it in, whatever makes most sense. The request update can go later. Best regards, Andreas Hindborg
Andreas Hindborg <a.hindborg@kernel.org> writes: > Hi Oliver, > > "Oliver Mangold" <oliver.mangold@pm.me> writes: > >> For usage with block-mq, we found that having a variant of ARef which is guaranteed to be unique is useful. As chances are it is useful in general, I implemented it as kernel::types::URef. The difference between ARef and URef is basically the same as between Arc and UniqueArc. > > Wrap at 75 characters please :) > Also, be sure to Cc relevant maintainers and reviewers (I did so in this reply). Use scripts/checkpatch.pl and refer to [1]. You can also use `b4` [2] to get a lot of automation. Best regards, Andreas Hindborg [1] https://docs.kernel.org/process/submitting-patches.html [2] https://b4.docs.kernel.org/en/latest/contributor/prep.html
On Fri, Feb 21, 2025 at 08:04:34AM +0000, Oliver Mangold wrote: > For usage with block-mq, we found that having a variant of ARef which is guaranteed to be unique is useful. As chances are it is useful in general, I implemented it as kernel::types::URef. The difference between ARef and URef is basically the same as between Arc and UniqueArc. Nit, please wrap your changelog text at 72 columns or so, checkpatch should have warned you about this, right? thanks, greg k-h
On 250221 0912, Greg KH wrote: > Nit, please wrap your changelog text at 72 columns or so, checkpatch > should have warned you about this, right? Sorry, I did checkpatch before adding the description. Learned something new, I guess :)
"Oliver Mangold" <oliver.mangold@pm.me> writes: > On 250221 0912, Greg KH wrote: >> Nit, please wrap your changelog text at 72 columns or so, checkpatch >> should have warned you about this, right? > > Sorry, I did checkpatch before adding the description. Learned something new, I guess :) Thanks for sending this! I can absolutely recommend b4 for sending your work [1]. It will warn you if you try to send but did not run checkpatch after your latest change. Best regards, Andreas Hindborg [1] https://b4.docs.kernel.org/en/latest/
© 2016 - 2025 Red Hat, Inc.