Message-ID: <7bc9f839-a69a-4819-ba6d-36eadd8776b3@gmail.com>
Date: Thu, 1 May 2025 21:11:20 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Benno Lossin <lossin@...nel.org>, Sky <sky@...9.dev>,
 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>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
 Gerald Wisböck <gerald.wisboeck@...ther.ink>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 1/3] rust: add UnsafePinned type

On 01.05.25 8:51 PM, Benno Lossin wrote:
> On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
>> On 30.04.25 11:45 AM, Benno Lossin wrote:
>>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>>> +/// This implementation works because of the "`!Unpin` hack" in rustc, which allows (some kinds of)
>>>> +/// mutual aliasing of `!Unpin` types. This hack might be removed at some point, after which only
>>>> +/// the `core::pin::UnsafePinned` type will allow this behavior. In order to simplify the migration
>>>> +/// to future rust versions only this polyfill of this type should be used when this behavior is
>>>> +/// required.
>>>> +///
>>>> +/// In order to disable niche optimizations this implementation uses [`UnsafeCell`] internally,
>>>> +/// the upstream version however will not. So the fact that [`UnsafePinned`] contains an
>>>> +/// [`UnsafeCell`] must not be relied on (Other than the niche blocking).
>>>
>>> I would make this last paragraph a normal comment, I don't think we
>>> should expose it in the docs.
>>
>> I added this as docs since I wanted it to be a bit more visible,
>> but I can replace the comment text (about `UnsafeCell`) with this paragraph
>> and drop it from the docs if you want.
> 
> I think we shouldn't talk about these implementation details in the
> docs.

Alright, what do you think of:

// As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`
// - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`
//   Required to use the `!Unpin hack`.
// - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
//   the upstream version however currently does not. This will most likely change in the future
//   but for now we don't expose this in the documentation, since adding the guarantee is simpler
//   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
//   must not be relied on (Other than the niche blocking).

> 
>>>> +// As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`
>>>> +// - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`
>>>> +//      Required to use the `!Unpin hack`.
>>>> +// - `UnsafeCell<T>` instead of T to disallow niche optimizations,
>>>> +//     which is handled in the compiler in upstream Rust
>>>> +#[repr(transparent)]
>>>> +pub struct UnsafePinned<T: ?Sized> {
>>>> +    _ph: PhantomPinned,
>>>> +    value: UnsafeCell<T>,
>>>> +}
>>>> +
>>>> +impl<T> UnsafePinned<T> {
>>>> +    /// Constructs a new instance of [`UnsafePinned`] which will wrap the specified value.
>>>> +    ///
>>>> +    /// All access to the inner value through `&UnsafePinned<T>` or `&mut UnsafePinned<T>` or
>>>> +    /// `Pin<&mut UnsafePinned<T>>` requires `unsafe` code.
>>>> +    #[inline(always)]
>>>> +    #[must_use]
>>>> +    pub const fn new(value: T) -> Self {
>>>> +        UnsafePinned {
>>>> +            value: UnsafeCell::new(value),
>>>> +            _ph: PhantomPinned,
>>>> +        }
>>>> +    }
>>>> +}
>>>> +impl<T: ?Sized> UnsafePinned<T> {
>>>> +    /// Get read-only access to the contents of a shared `UnsafePinned`.
>>>> +    ///
>>>> +    /// Note that `&UnsafePinned<T>` is read-only if `&T` is read-only. This means that if there is
>>>> +    /// mutation of the `T`, future reads from the `*const T` returned here are UB! Use
>>>> +    /// [`UnsafeCell`] if you also need interior mutability.
>>>
>>> I agree with copy-pasting the docs from upstream, even though our
>>> implementation already wraps the value in `UnsafeCell`, but I think we
>>> should include a comment at the top of this doc that mentions this
>>> difference. So something along the lines "In order to make replacing
>>> this type with the upstream one, we want to have as little API
>>> divergence as possible. Thus we don't mention the implementation detail
>>> of `UnsafeCell` and people have to use `UnsafePinned<UnsafeCell<T>>`
>>> instead of just `UnsafePinned<T>`." feel free to modify.
>>>
>>
>> I already wrote about this in comments (and documentation in this version)
>> on the `UnsafePinned` type definition.
>>
>> I'm not sure where exactly we want to have this, but I think having it
>> at the top of the file and on the type definition is a bit redundant.
> 
> Sure.

I'll add the following sentence to the end of the module documentation:

For details on the difference to the upstream implementation see the
comment on the [`UnsafePinned`] struct definition.

> 
>>>> +    /// Gets a mutable pointer to the wrapped value.
>>>> +    ///
>>>> +    /// The difference from `get_mut_pinned` and `get_mut_unchecked` is that this function
>>>> +    /// accepts a raw pointer, which is useful to avoid the creation of temporary references.
>>>
>>> You did not include the `get_mut_{pinned,unchecked}` methods, so
>>> mentioning them here in the docs might confuse people. Do we want to
>>> have those methods?
>>
>> I only included the functions that we needed for `Opaque` and my
>> `miscdevice' patches. I think these functions should only be added
>> once they have a user. That's why I wrote the next sentence in the
>> documents.
>>
>> Should I handle this differently?
>>
>> It should be a really simple patch to add these functions and I can
>> do that if someone needs them or I can just include them in this
>> patch set.
> 
> Then I'd remove the sentence referencing the functions you don't add.

Alright

Cheers
Christian

Powered by blists - more mailing lists