Re: [PATCH v3 1/4] rust: implement `kernel::sync::Refcount`

Tamir Duberstein
2025-02-19T22:12:57Z

On Wed, Feb 19, 2025 at 3:17 PM Gary Guo <gary@garyguo.net> wrote:
>
> This is a wrapping layer of `include/linux/refcount.h`. Currently the
> kernel refcount has already been used in `Arc`, however it calls into
> FFI directly.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
> rust/helpers/refcount.c | 10 +++++
> rust/kernel/sync.rs | 2 +
> rust/kernel/sync/refcount.rs | 86 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 98 insertions(+)
> create mode 100644 rust/kernel/sync/refcount.rs
>
> diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> index d6adbd2e45a1..d175898ad7b8 100644
> --- a/rust/helpers/refcount.c
> +++ b/rust/helpers/refcount.c
> @@ -7,11 +7,21 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
> return (refcount_t)REFCOUNT_INIT(n);
> }
>
> +void rust_helper_refcount_set(refcount_t *r, int n)
> +{
> + refcount_set(r, n);
> +}
> +
> void rust_helper_refcount_inc(refcount_t *r)
> {
> refcount_inc(r);
> }
>
> +void rust_helper_refcount_dec(refcount_t *r)
> +{
> + refcount_dec(r);
> +}
> +
> bool rust_helper_refcount_dec_and_test(refcount_t *r)
> {
> return refcount_dec_and_test(r);
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 3498fb344dc9..b196cd0b358e 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -13,6 +13,7 @@
> mod locked_by;
> pub mod poll;
> pub mod rcu;
> +mod refcount;
>
> pub use arc::{Arc, ArcBorrow, UniqueArc};
> pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
> @@ -20,6 +21,7 @@
> pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
> pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
> pub use locked_by::LockedBy;
> +pub use refcount::Refcount;
>
> /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> #[repr(transparent)]
> diff --git a/rust/kernel/sync/refcount.rs b/rust/kernel/sync/refcount.rs
> new file mode 100644
> index 000000000000..a6a683f5d7b8
> --- /dev/null
> +++ b/rust/kernel/sync/refcount.rs
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Atomic reference counting.
> +//!
> +//! C header: [`include/linux/refcount.h`](srctree/include/linux/refcount.h)
> +
> +use crate::types::Opaque;
> +
> +/// Atomic reference counter.
> +///
> +/// This type is conceptually an atomic integer, but provides saturation semantics compared to
> +/// normal atomic integers. Values in the negative range when viewed as a signed integer are
> +/// saturation (bad) values. For details about the saturation semantics, please refer to top of
> +/// [`include/linux/refcount.h`](srctree/include/refcount.h).
> +///
> +/// Wraps the kernel's C `refcount_t`.
> +#[repr(transparent)]
> +pub struct Refcount(Opaque<bindings::refcount_t>);
> +
> +impl Refcount {
> + /// Construct a new [`Refcount`] from an initial value.
> + #[inline]
> + pub fn new(value: i32) -> Self {
> + // SAFETY: There are no safety requirements for this FFI call.
> + Self(Opaque::new(unsafe { bindings::REFCOUNT_INIT(value) }))
> + }
> +
> + #[inline]
> + fn as_ptr(&self) -> *mut bindings::refcount_t {
> + self.0.get()
> + }
> +
> + /// Set a refcount's value.
> + #[inline]
> + pub fn set(&self, value: i32) {
> + // SAFETY: `self.as_ptr()` is valid.
> + unsafe { bindings::refcount_set(self.as_ptr(), value) }
> + }
> +
> + /// Increment a refcount.
> + ///
> + /// It will saturate if overflows and `WARN`. It will also `WARN` if the refcount is 0, as this
> + /// represents a possible use-after-free condition.
> + ///
> + /// Provides no memory ordering, it is assumed that caller already has a reference on the
> + /// object.
> + #[inline]
> + pub fn inc(&self) {
> + // SAFETY: self is valid.
> + unsafe { bindings::refcount_inc(self.as_ptr()) }
> + }
> +
> + /// Decrement a refcount.
> + ///
> + /// It will `WARN` on underflow and fail to decrement when saturated.
> + ///
> + /// Provides release memory ordering, such that prior loads and stores are done
> + /// before.
> + #[inline]
> + pub fn dec(&self) {
> + // SAFETY: `self.as_ptr()` is valid.
> + unsafe { bindings::refcount_dec(self.as_ptr()) }
> + }
> +
> + /// Decrement a refcount and test if it is 0.
> + ///
> + /// It will `WARN` on underflow and fail to decrement when saturated.
> + ///
> + /// Provides release memory ordering, such that prior loads and stores are done
> + /// before, and provides an acquire ordering on success such that memory deallocation
> + /// must come after.
> + ///
> + /// Returns true if the resulting refcount is 0, false otherwise.
> + #[inline]
> + #[must_use = "use `dec` instead you do not need to test if it is 0"]

The word "if" seems to be missing?

The C function comment includes this bit:

* Use of this function is not recommended for the normal reference counting
* use case in which references are taken and released one at a time. In these
* cases, refcount_dec(), or one of its variants, should instead be used to
* decrement a reference count.

Do we need to include this warning?

> + pub fn dec_and_test(&self) -> bool {
> + // SAFETY: `self.as_ptr()` is valid.
> + unsafe { bindings::refcount_dec_and_test(self.as_ptr()) }
> + }
> +}
> +
> +// SAFETY: `refcount_t` is thread-safe.
> +unsafe impl Send for Refcount {}
> +
> +// SAFETY: `refcount_t` is thread-safe.
> +unsafe impl Sync for Refcount {}
> --
> 2.47.2
>