Re: [PATCH v8 4/4] rust: adding OwnableRefCounted and SimpleOwnableRefCounted

[Oliver Mangold <oliver.mangold@pm.me>]

On 250321 0937, Boqun Feng wrote:
> >
> > +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
> > +/// [`ARef`].
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +///
> > +/// - Both the safety requirements for [`Ownable`] and [`RefCounted`] are fulfilled.
> > +/// - The uniqueness invariant of [`Owned`] is upheld until dropped.
> 
> Could you explain what this safety requirement means? Isn't this part of
> the safe requirement of `impl Ownable`?

To be honest, I don't remember. Right now I also can't see a good reason
for it. Might be I was confused by the documentation of the safety
requirements for Ownable.

Thinking about it, there seems to be something wrong with these:

> /// # Safety
> ///
> /// Implementers must ensure that any objects borrowed directly as `&T` stay alive for the duration
> /// of the lifetime, and that any objects owned by Rust as [`Owned<T>`] stay alive while that owned
> /// reference exists, until the [`Ownable::release()`] trait method is called.
> pub unsafe trait Ownable {
>     /// Releases the object (frees it or returns it to foreign ownership).
>     ///
>     /// # Safety
>     ///
>     /// Callers must ensure that the object is no longer referenced after this call.
>     unsafe fn release(this: NonNull<Self>);
> }
> 
> /// A subtrait of Ownable that asserts that an [`Owned<T>`] Rust reference is not only unique
> /// within Rust and keeps the `T` alive, but also guarantees that the C code follows the
> /// usual mutable reference requirements. That is, the kernel will never mutate the
> /// `T` (excluding internal mutability that follows the usual rules) while Rust owns it.
> ///
> /// When this type is implemented for an [`Ownable`] type, it allows [`Owned<T>`] to be
> /// dereferenced into a &mut T.
> ///
> /// # Safety
> ///
> /// Implementers must ensure that the kernel never mutates the underlying type while
> /// Rust owns it.
> pub unsafe trait OwnableMut: Ownable {}

As an Owned hands out an `&` to an Ownable, the requirement about the kernel
not mutating it should apply to an Ownable just the same as to an OwnableMut.

The point of OwnableMut should by to declare that it is safe to hand out an
`&mut` which implies that it is not pinned.

I didn't want to mess too much with Asahi Lina's patch, so I left it as is.
Maybe she wanted to assign a different meaning of these traits.

Thoughts?

> > +///
> > +/// struct Foo {
> > +///     refcount: Cell<usize>,
> 
> It's fine to use a Cell for now, but eventually we want to replace this
> with either Gary's Refcount [1] or LKMM atomics.
> 
> [1]: https://lore.kernel.org/rust-for-linux/20250219201602.1898383-1-gary@garyguo.net/
> 
> (just keeping a note here)

Ok, then I will leave it as is. I was wondering if I should make it atomic,
but I didn't because of the recent Rust atomics vs. kernel atomics
discussion, which I understood as there being some unresolved questions.

> > +pub unsafe trait OwnableRefCounted: RefCounted + Ownable + Sized {
> > +    /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
> > +    /// Otherwise it returns again an [`ARef`] to the same underlying object.
> > +    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
> > +    /// Converts the [`Owned`] into an [`ARef`].
> > +
> 
> ^ this blanket line seems to be at the wrong place.

Right. Thanks.

Best regards,

Oliver