Re: [PATCH v11 1/4] rust: types: Add Ownable/Owned types

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>>>> On 250819 0027, Benno Lossin wrote:
>>>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>>>>> > On 250818 1446, Andreas Hindborg wrote:
>>>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>>>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>>>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>>>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>>>>> >> > +        // return a mutable reference to it.
>>>>> >> > +        unsafe { self.ptr.as_mut() }
>>>>> >> > +    }
>>>>> >> > +}
>>>>> >>
>>>>> >> I think someone mentioned this before, but handing out mutable
>>>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>>>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>>>>> >>
>>>>> >
>>>>> > That was the reason, why `OwnableMut` was introduced in the first place.
>>>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>>>>>
>>>>> Yeah the safety requirements ensure that you can't implement it on
>>>>> `!Unpin` types.
>>>>>
>>>>> But I'm not sure it's useful then? As you said there aren't many types
>>>>> that will implement the type then, so how about we change the meaning
>>>>> and make it give out a pinned mutable reference instead?
>>>>
>>>> Making `deref_mut()` give out a pinned type won't work. The return types of
>>>> deref() are required to match.
>>>
>>> I meant the changes that Andreas suggested.
>>
>> Not sure what you are asking, but I need to assert exclusive access to
>> an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
>> `Pin<&mut Page>`. I think the latter is more agnostic.
>
> The former isn't really correct? It's like having a `&mut Box<Page>`
> which is weird. I was saying we can have a `DerefMut` impl gated on `T:
> Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.
>
>>>>> > Good question, I have been thinking about it, too. But it might
>>>>> > be, that it isn't needed at all. As I understand, usually Rust wrappers
>>>>> > are around non-movable C structs. Do we actually have a useful application
>>>>> > for OwnableMut?
>>>>>
>>>>> Also, do we even need two different traits? Which types would only
>>>>> implement `Ownable` but not `OwnableMut`?
>>>>
>>>> I'm not 100% sure, but on a quick glance it looks indeed be safe to
>>>> substitute `OwnableMut` by `Unpin`.
>>>
>>> We just have to change the safety requirements of `OwnableMut`.
>>
>> `OwnableMut` already requires `Unpin`, it just does not say so directly:
>>
>>
>> /// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
>> ///   (i.e. most kernel types).
>>
>> We could remove this and then just add a trait bound on `Unpin`.
>
> Oh I happened to not have read it that thoroughly then... I don't think
> it makes sense to have `OwnableMut` then. So I agree with your suggested
> change :)
>
>>>> If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
>>>> it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.
>>>
>>> Well the `DerefMut` impl still is convenient in the `Unpin` case.
>>
>> `OwnableMut` is probably not that useful, since all the types we want to
>> implement `Ownable` for is `!Unpin`. We could remove it, but I felt it
>> was neat to add the `DerefMut` impl for `Unpin` types.
>
> But we don't need `OwnableMut` in that case?

Right, we can directly limit the `DerefMut` impl.

>
> Let's just do the following if it makes sense:
> * remove `OwnableMut`
> * allow obtaining a `Pin<&mut T>` from `Owned<T>` via a `&mut self`
>   method (we need a new safety requirement for this on `Ownable`)
> * have the `DerefMut` impl require `T: Unpin`

Sounds good to me 👍


Best regards,
Andreas Hindborg