Message-ID: <b7492912-3e58-43c4-aa0c-e6bcd4925e24@gmail.com>
Date: Thu, 23 Jan 2025 11:21:23 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...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>, Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 Daniel Almeida <daniel.almeida@...labora.com>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 1/3] rust: add Aliased type



On 20.01.25 6:24 PM, Boqun Feng wrote:
> Hi Christian,
> 
> [Cc Daniel and Danilo]
> 
> Thanks for the patch!
> 
> On Sun, Jan 19, 2025 at 11:11:13PM +0100, Christian Schrefl wrote:
>> This type is useful for cases where a value might be shared with C code
>> but not interpreted by it.
>> In partiquarly this is added to for data that is shared between a Driver
>> and a MiscDevice implementation.
>>
>> Similar to Opaque but guarantees that the value is initialized and the
>> inner value is dropped when Aliased is dropped.
>>
>> This was origianally proposed for the IRQ abstractions [0], but also
>> useful for other cases where Data may be aliased, but is always valid
>> and automatic drop is desired.
>>
>> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
>> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@...il.com>
>> ---
>>  rust/kernel/types.rs | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 3aea6af9a0bca70ee42b4bad2fe31a99750cbf11..5640128c9a9055476a0040033946ba6caa6e7076 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -528,3 +528,43 @@ pub enum Either<L, R> {
>>  /// [`NotThreadSafe`]: type@...ThreadSafe
>>  #[allow(non_upper_case_globals)]
>>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
>> +
>> +/// Stores a value that may be aliased.
>> +///
>> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will
>> +/// Call the Drop implementation of T when dropped.
>> +#[repr(transparent)]
>> +pub struct Aliased<T> {
> 
> As I already mentioned [1], the name `Aliased` is more reflecting the
> fact that this wrapper will avoid generating the "noalias" attribute(?)
> on the reference/pointer to the type rather than an intuitive idea about
> "why or when do I need this". Moreover, I think the argument about the
> naming of the counterpart in unstable Rust (UnsafePinned) makes sense to
> me [2]: this type alone won't prevent `&mut Aliased` getting `swap`, and
> it has to be used with `Pin`.
> 
> Therefore, I think we should use a different name, perhaps
> `(Always)Shared`, or just use `UnsafePinned`, or, as always, looking
> fowards to a better name from anybody ;-)
> 
> [1]: https://lore.kernel.org/rust-for-linux/Z407egxOy7oNLpq8@boqun-archlinux/
> [2]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#naming

I don't particularly care about the name, I mostly used aliased, because that's
the name that Alice originally used.

`(Always)Shared` seems confusing to me.

I guess we can use `UnsafePinned`, but most people won't know what that means,
also I'm not sure if it's a good Idea to use the same name as a (future) 
language type.

If there is a consensus on a good name I'll use that for the next version.

> 
> Regards,
> Boqun
> 
>> +    value: UnsafeCell<T>,
>> +    _pin: PhantomPinned,
>> +}
>> +
>> +impl<T> Aliased<T> {
>> +    /// Creates a new `Aliased` value.
>> +    pub const fn new(value: T) -> Self {
>> +        Self {
>> +            value: UnsafeCell::new(value),
>> +            _pin: PhantomPinned,
>> +        }
>> +    }
>> +    /// Create an `Aliased` pin-initializer from the given pin-initializer.
>> +    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        // SAFETY:
>> +        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
>> +        // since value is initialized and _pin is a Zero sized type.
>> +        // The pin invariants of value are upheld, since no moving occurs.
>> +        unsafe { init::pin_init_from_closure(move |slot| value.__pinned_init(Self::raw_get(slot))) }
>> +    }
>> +    /// Returns a raw pointer to the opaque data.
>> +    pub const fn get(&self) -> *mut T {
>> +        UnsafeCell::get(&self.value).cast::<T>()
>> +    }
>> +
>> +    /// Gets the value behind `this`.
>> +    ///
>> +    /// This function is useful to get access to the value without creating intermediate
>> +    /// references.
>> +    pub const fn raw_get(this: *const Self) -> *mut T {
>> +        UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
>> +    }
>> +}
>>
>> -- 
>> 2.48.1
>>

Powered by blists - more mailing lists