Re: [PATCH v2 1/2] rust: move ARef and AlwaysRefCounted to sync::aref

[Shankari Anand <shankari.ak0208@gmail.com>]

Hello,
Thanks for your feedback!

> I'd say this module is about supporting objects with builtin reference
> counting.
I described the module as an "Atomic reference-counted pointer
abstraction" to highlight its similarity to Arc<T>, but for manually
refcounted types. Your version about “supporting objects with built-in
reference counting” is definitely clearer, happy to adopt that
phrasing if preferred.

> I would lead with comparing `ARef<T>` to `Arc<T>` and only later mention
> `AlwaysRefCounted`.
I mentioned AlwaysRefCounted early on just to give readers a quick
idea of what the module is really about, but I understand your point
about introducing it a bit later and can rearrange that.

>
> You also rephrased these docs, can you do that in a separate patch?

I rephrased a few lines in the intro since we’re starting a new module
and I thought it might help with readability. But if keeping the
original is better, I can revert those changes, no problem at all.


> Newline?
About the extra newline - I didn’t add it manually, but it might’ve
come from creating the new file. I’ll check the patch, and if it was
actually added, I’ll remove it.

> ---
> Cheers,
> Benno
---
Thanks and regards,
Shankari


On Thu, Jun 26, 2025 at 3:59 AM Benno Lossin <lossin@kernel.org> wrote:
>
> On Wed Jun 25, 2025 at 1:11 PM CEST, Shankari Anand wrote:
> > diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> > new file mode 100644
> > index 000000000000..93a23b493e21
> > --- /dev/null
> > +++ b/rust/kernel/sync/aref.rs
> > @@ -0,0 +1,170 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Atomic reference-counted pointer abstraction.
>
> I'd say this module is about supporting objects with builtin reference
> counting.
>
> > +//!
> > +//! This module provides [`ARef<T>`], an owned reference to a value that implements
> > +//! [`AlwaysRefCounted`] — an unsafe trait for types that manage their own reference count.
>
> I would lead with comparing `ARef<T>` to `Arc<T>` and only later mention
> `AlwaysRefCounted`.
>
> > +//!
> > +//! It is based on the Linux kernel's manual reference counting model and is typically used
> > +//! with C types that implement reference counting (e.g., via `refcount_t` or `kref`).
> > +//!
> > +//! For Rust-managed objects, prefer using [`Arc`](crate::sync::Arc) instead.
> > +
> > +use core::{
> > +    marker::PhantomData,
> > +    mem::ManuallyDrop,
> > +    ops::Deref,
> > +    ptr::NonNull,
> > +};
> > +
> > +/// Trait for types that are _always_ reference-counted.
> > +///
> > +/// This trait allows types to define custom reference increment and decrement logic.
> > +/// It enables safe conversion from a shared reference `&T` to an owned [`ARef<T>`].
> > +///
> > +/// This is usually implemented by wrappers around C types with manual refcounting.
> > +///
> > +/// For purely Rust-managed memory, consider using [`Arc`](crate::sync::Arc) instead.
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +///
> > +/// - Calling [`AlwaysRefCounted::inc_ref`] keeps the object alive in memory until a matching [`AlwaysRefCounted::dec_ref`] is called.
> > +/// - The object is always managed by a reference count; it must never be stack-allocated or
> > +///   otherwise untracked.
> > +/// - When the count reaches zero in [`AlwaysRefCounted::dec_ref`], the object is properly freed and no further
> > +///   access occurs.
> > +///
> > +/// Failure to follow these rules may lead to use-after-free or memory corruption.
>
> You also rephrased these docs, can you do that in a separate patch?
>
> > +
>
> Newline?
>
> ---
> Cheers,
> Benno
>
> > +pub unsafe trait AlwaysRefCounted {
> > +    /// Increments the reference count on the object.
> > +    fn inc_ref(&self);
> > +
> > +    /// Decrements the reference count on the object.
> > +    ///
> > +    /// Frees the object when the count reaches zero.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that there was a previous matching increment to the reference count,
> > +    /// and that the object is no longer used after its reference count is decremented (as it may
> > +    /// result in the object being freed), unless the caller owns another increment on the refcount
> > +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> > +    /// [`AlwaysRefCounted::dec_ref`] once).
> > +    unsafe fn dec_ref(obj: NonNull<Self>);
> > +}