Re: [PATCH v20 7/8] rust: Add `OwnableRefCounted`

On Tue Aug 25, 2026 at 2:16 PM BST, Danilo Krummrich wrote:
> On Mon Aug 24, 2026 at 1:17 PM CEST, Andreas Hindborg wrote:
>> +/// struct Foo {
>> +///     refcount: Cell<usize>,
>> +/// }
>> +///
>> +/// impl Foo {
>> +///     fn new() -> Result<Owned<Self>> {
>> +///         // We are just using a `KBox` here to handle the actual allocation, as our `Foo` is
>> +///         // not actually a C-allocated object.
>> +///         // INVARIANT: We initialize `refcount` to 1, satisfying the invariants.
>> +///         let result = KBox::new(
>> +///             Foo {
>> +///                 refcount: Cell::new(1),
>> +///             },
>> +///             flags::GFP_KERNEL,
>> +///         )?;
>> +///         let result = KBox::into_non_null(result);
>> +///         // SAFETY:
>> +///         //  - We just allocated the `Self`, thus it is valid and we own it.
>> +///         //  - We can transfer this ownership to the `from_raw` method.
>> +///         Ok(unsafe { Owned::from_raw(result) })
>> +///     }
>> +/// }
>> +///
>> +/// // SAFETY: We increment and decrement each time the respective function is called and only free
>> +/// // the `Foo` when the refcount reaches zero.
>> +/// unsafe impl RefCounted for Foo {
>> +///     fn inc_ref(&self) {
>> +///         self.refcount.replace(self.refcount.get() + 1);
>> +///     }
>> +///
>> +///     unsafe fn dec_ref(this: NonNull<Self>) {
>> +///         // SAFETY: By requirement on calling this function, the refcount is non-zero,
>> +///         // implying the underlying object is valid.
>> +///         let refcount = unsafe { &this.as_ref().refcount };
>> +///         let new_refcount = refcount.get() - 1;
>> +///         if new_refcount == 0 {
>> +///             // The `Foo` will be dropped when `KBox` goes out of scope.
>> +///             // SAFETY: The [`KBox<Foo>`] is still alive as the old refcount is 1. We can pass
>> +///             // ownership to the [`KBox`] as by requirement on calling this function,
>> +///             // the `Self` will no longer be used by the caller.
>> +///             unsafe { KBox::from_raw(this.as_ptr()) };
>> +///         } else {
>> +///             refcount.replace(new_refcount);
>> +///         }
>> +///     }
>> +/// }
>
> This is valid as Foo is !Sync, but I think it does look racy on casual reading
> and possibly even encourages people to do the wrong thing, i.e. to peek a
> reference count and subsequently act on the read value.
>
> Besides that, if the value can't be shared across tasks it's not overly useful
> in the kernel to reference count it in the first place. Do you have a better
> example for this?  Maybe a broken down version of the one that motivates the
> patch?

I think the series is very old and might even predate the `sync::Refcount`. But
given that we have that type now, the example should do the correct thing and
use that instead.

Best,
Gary