Message-ID: <aS1slBD1t-Y_K-aC@mango>
Date: Mon, 01 Dec 2025 10:23:20 +0000
From: Oliver Mangold <oliver.mangold@...me>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Benno Lossin <lossin@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Dave Ertman <david.m.ertman@...el.com>, Ira Weiny <ira.weiny@...el.com>, Leon Romanovsky <leon@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, Viresh Kumar
	<vireshk@...nel.org>, Nishanth Menon <nm@...com>, Stephen Boyd <sboyd@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Wilczyński <kwilczynski@...nel.org>, Paul Moore <paul@...l-moore.com>, Serge Hallyn <sergeh@...nel.org>, Asahi Lina <lina+kernel@...hilina.net>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, linux-block@...r.kernel.org, dri-devel@...ts.freedesktop.org, linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, linux-pm@...r.kernel.org, linux-pci@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`

On 251128 1506, Daniel Almeida wrote:

> > /// Type allocated and destroyed on the C side, but owned by Rust.
> > ///
> > -/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
> > -/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
> > -/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
> > -/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
> > +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type.
> > +///  - This is useful when it is desirable to tie the lifetime of an object reference to an owned
> > +///    object, rather than pass around a bare reference.
> > +///  - [`Ownable`] types can define custom drop logic that is executed when the owned reference
> > +///    of type [`Owned<_>`] pointing to the object is dropped.
> > ///
> > /// Note: The underlying object is not required to provide internal reference counting, because it
> > /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> > -/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> > +/// [`RefCounted`] should be implemented. [`OwnableRefCounted`] should be implemented if conversion
> > +/// between unique and shared (reference counted) ownership is needed.
> > ///
> > /// # Safety
> > ///
> > @@ -143,9 +146,7 @@ impl<T: Ownable> Owned<T> {
> >     ///   mutable reference requirements. That is, the kernel will not mutate or free the underlying
> >     ///   object and is okay with it being modified by Rust code.
> >     pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > -        Self {
> > -            ptr,
> > -        }
> > +        Self { ptr }
> >     }
> 
> Unrelated change?

Ah, yes, rustfmt must I done that, and I missed it. Will fix.

> > +///
> > +/// impl OwnableRefCounted for Foo {
> > +///     fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> > +///         if this.refcount.get() == 1 {
> > +///             // SAFETY: The `Foo` is still alive and has no other Rust references as the refcount
> > +///             // is 1.
> > +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> > +///         } else {
> > +///             Err(this)
> > +///         }
> > +///     }
> > +/// }
> > +///
> 
> We wouldn’t need this implementation if we added a “refcount()”
> member to this trait. This lets you abstract away this logic for all
> implementors, which has the massive upside of making sure we hardcode (and thus
> enforce) the refcount == 1 check.

This wouldn't work for the block `Request` use case. There a reference can
be acquired "out of thin air" using a `TagSet`. Thus "check for unique
refcount" + "create an owned reference" needs to be one atomic operation.

Also I think it might be generally problematic to require a refcount()
function. The API of the underlying kernel object we want to wrap might not
offer that, so we would need to access internal data.


> > +/// // SAFETY: This implementation of `release()` is safe for any valid `Self`.
> > +/// unsafe impl Ownable for Foo {
> > +///     unsafe fn release(this: NonNull<Self>) {
> > +///         // SAFETY: Using `dec_ref()` from [`RefCounted`] to release is okay, as the refcount is
> > +///         // always 1 for an [`Owned<Foo>`].
> > +///         unsafe{ Foo::dec_ref(this) };
> > +///     }
> > +/// }
> > +///
> > +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");
> 
> All these “expects()” and custom error strings would go away if you
> place this behind a fictional function that returns Result.

Not sure what you mean by fictional function. Do you mean a non-existent
function? We want to compile this code as a unit test.

The rest of your suggested changes make sense, I guess. I will implement
them.

Thanks,

Oliver

Powered by blists - more mailing lists