[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <D9L1TI5NVKJU.361JFPWMLDWN4@kernel.org>
Date: Thu, 01 May 2025 20:51:08 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Christian Schrefl" <chrisi.schrefl@...il.com>, "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 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.
>>> +// 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.
>>> + /// 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.
---
Cheers,
Benno
Powered by blists - more mailing lists