Message-ID: <ZrKt7K68W1Jh6nhr@pollux>
Date: Wed, 7 Aug 2024 01:12:44 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	acurrid@...dia.com, cjia@...dia.com, jhubbard@...dia.com,
	airlied@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH v4 08/28] rust: types: implement `Unique<T>`

On Tue, Aug 06, 2024 at 05:22:21PM +0000, Benno Lossin wrote:
> On 05.08.24 17:19, Danilo Krummrich wrote:
> > Implement the `Unique` type as a prerequisite for `Box` and `Vec`
> > introduced in subsequent patches.
> > 
> > `Unique` serves as wrapper around a `NonNull`, but indicates that the
> > possessor of this wrapper owns the referent.
> > 
> > This type already exists in Rust's core library, but, unfortunately, is
> > exposed as unstable API and hence shouldn't be used in the kernel.
> > 
> > This implementation of `Unique` is almost identical, but mostly stripped
> > down to the functionality we need for `Box` and `Vec`. Additionally, all
> > unstable features are removed and / or replaced by stable ones.
> > 
> > Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
> > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > ---
> >  rust/kernel/types.rs | 183 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 183 insertions(+)
> > 
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index bd189d646adb..7cf89067b5fc 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -473,3 +473,186 @@ unsafe impl AsBytes for str {}
> >  // does not have any uninitialized portions either.
> >  unsafe impl<T: AsBytes> AsBytes for [T] {}
> >  unsafe impl<T: AsBytes, const N: usize> AsBytes for [T; N] {}
> > +
> > +/// A wrapper around a raw non-null `*mut T` that indicates that the possessor
> > +/// of this wrapper owns the referent. Useful for building abstractions like
> > +/// `Box<T>`, `Vec<T>`, `String`, and `HashMap<K, V>`.
> > +///
> > +/// Unlike `*mut T`, `Unique<T>` behaves "as if" it were an instance of `T`.
> > +/// It implements `Send`/`Sync` if `T` is `Send`/`Sync`. It also implies
> > +/// the kind of strong aliasing guarantees an instance of `T` can expect:
> > +/// the referent of the pointer should not be modified without a unique path to
> > +/// its owning Unique.
> > +///
> > +/// If you're uncertain of whether it's correct to use `Unique` for your purposes,
> > +/// consider using `NonNull`, which has weaker semantics.
> > +///
> > +/// Unlike `*mut T`, the pointer must always be non-null, even if the pointer
> > +/// is never dereferenced. This is so that enums may use this forbidden value
> > +/// as a discriminant -- `Option<Unique<T>>` has the same size as `Unique<T>`.
> > +/// However the pointer may still dangle if it isn't dereferenced.
> > +///
> > +/// Unlike `*mut T`, `Unique<T>` is covariant over `T`. This should always be correct
> > +/// for any type which upholds Unique's aliasing requirements.
> > +#[repr(transparent)]
> > +pub struct Unique<T: ?Sized> {
> > +    pointer: NonNull<T>,
> > +    // NOTE: this marker has no consequences for variance, but is necessary
> > +    // for dropck to understand that we logically own a `T`.
> > +    //
> > +    // For details, see:
> > +    // https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#phantom-data
> > +    _marker: PhantomData<T>,
> > +}
> > +
> > +/// `Unique` pointers are `Send` if `T` is `Send` because the data they
> > +/// reference is unaliased. Note that this aliasing invariant is
> > +/// unenforced by the type system; the abstraction using the
> > +/// `Unique` must enforce it.
> > +unsafe impl<T: Send + ?Sized> Send for Unique<T> {}
> > +
> > +/// `Unique` pointers are `Sync` if `T` is `Sync` because the data they
> > +/// reference is unaliased. Note that this aliasing invariant is
> > +/// unenforced by the type system; the abstraction using the
> > +/// `Unique` must enforce it.
> > +unsafe impl<T: Sync + ?Sized> Sync for Unique<T> {}
> > +
> > +impl<T: Sized> Unique<T> {
> > +    /// Creates a new `Unique` that is dangling, but well-aligned.
> > +    ///
> > +    /// This is useful for initializing types which lazily allocate, like
> > +    /// `Vec::new` does.
> > +    ///
> > +    /// Note that the pointer value may potentially represent a valid pointer to
> > +    /// a `T`, which means this must not be used as a "not yet initialized"
> > +    /// sentinel value. Types that lazily allocate must track initialization by
> > +    /// some other means.
> > +    #[must_use]
> > +    #[inline]
> > +    pub const fn dangling() -> Self {
> > +        Unique {
> > +            pointer: NonNull::dangling(),
> > +            _marker: PhantomData,
> > +        }
> > +    }
> 
> I think I already asked this, but the code until this point is copied
> from the rust stdlib and nowhere cited, does that work with the
> licensing?
> 
> I also think that the code above could use some improvements:
> - add an `# Invariants` section with appropriate invariants (what are
>   they supposed to be?)
> - Do we really want this type to be public and exported from the kernel
>   crate? I think it would be better if it were crate-private.
> - What do we gain from having this type? As I learned recently, the
>   `Unique` type from `core` doesn't actually put the `noalias` onto
>   `Box` and `Vec`. The functions are mostly delegations to `NonNull`, so
>   if the only advantages are that `Send` and `Sync` are already
>   implemented, then I think we should drop this.

I originally introduced it for the reasons described in [1], but mainly to make
clear that the owner of this thing also owns the memory behind the pointer and
the `Send` and `Sync` stuff you already mentioned.

If no one else has objections we can also just drop it. Personally, I'm fine
either way.

[1] https://docs.rs/rust-libcore/latest/core/ptr/struct.Unique.html

> 
> > +}
> > +
> > +impl<T: ?Sized> Unique<T> {
> > +    /// Creates a new `Unique`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `ptr` must be non-null.
> > +    #[inline]
> > +    pub const unsafe fn new_unchecked(ptr: *mut T) -> Self {
> > +        // SAFETY: the caller must guarantee that `ptr` is non-null.
> > +        unsafe {
> 
> The only unsafe operation in the body is `new_unchecked` only that one
> should be wrapped in `unsafe {}`.
> 
> > +            Unique {
> > +                pointer: NonNull::new_unchecked(ptr),
> > +                _marker: PhantomData,
> > +            }
> > +        }
> > +    }
> > +
> > +    /// Creates a new `Unique` if `ptr` is non-null.
> > +    #[allow(clippy::manual_map)]
> > +    #[inline]
> > +    pub fn new(ptr: *mut T) -> Option<Self> {
> > +        if let Some(pointer) = NonNull::new(ptr) {
> > +            Some(Unique {
> > +                pointer,
> > +                _marker: PhantomData,
> > +            })
> > +        } else {
> > +            None
> > +        }
> 
> Why is this so verbose? You even needed to disable the clippy lint!
> Can't this just be?:
> 
>     Some(Unique {
>         pointer: NonNull::new(ptr)?,
>         _marker: PhantomData,
>     })
> 
> or maybe even
> 
>     NonNull::new(ptr).map(Unique::from)
> 
> 
> > +    }
> > +
> > +    /// Acquires the underlying `*mut` pointer.
> > +    #[must_use = "`self` will be dropped if the result is not used"]
> 
> This seems like an odd thing, there is no `Drop` impl that drops the
> pointee...
> 
> ---
> Cheers,
> Benno
> 

Powered by blists - more mailing lists