[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa6d6c0a-17eb-4280-baa3-df5f97e58497@proton.me>
Date: Sat, 06 Jul 2024 10:59:09 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...hat.com>, 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
Cc: 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
Subject: Re: [PATCH 09/20] rust: types: implement `Unique<T>`
On 04.07.24 19:06, Danilo Krummrich wrote:
> Implement the `Unique` type as a prerequisite for `KBox` and `Kvec`
> 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.
It's not really exposed, as the feature (ptr_internals) is an internal
unstable feature and thus probably perma-unstable.
(but your argument still holds, this just means that we *really* should
not be using it)
> This implementation of `Unique` is almost identical, but mostly stripped
> down to the functionality we need for `KBox` and `KVec`. Additionally,
> all unstable features are removed and / or replaced by stable ones.
>
> Signed-off-by: Danilo Krummrich <dakr@...hat.com>
> ---
> rust/kernel/types.rs | 176 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 176 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 2e7c9008621f..281327ea2932 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -409,3 +409,179 @@ pub enum Either<L, R> {
> /// Constructs an instance of [`Either`] containing a value of type `R`.
> Right(R),
> }
> +
> +/// 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.
Since you copied this directly from the stdlib, should this be citing
it?
> +#[repr(transparent)]
> +pub struct Unique<T: ?Sized> {
> + pointer: NonNull<T>,
Gary and I already mentioned this in the meeting, but I will put it here
for the record:
The `Unique` from std is special, in the sense that the Rust compiler
will emit the `noalias` LLVM attribute.
This gives std's `Box` type a possible performance advantage (IIRC Gary
had a compiler explorer example that showed different instruction
count).
I thought that we could mimic this behavior if we would change the type
of `pointer` to `ManuallyDrop<Box<T>>`. But that only works if the
pointer is always valid, since otherwise we can't call `Box::from_raw`.
So currently I don't see a workaround that would give us the noalias
attribute back.
It would be a good idea to get some benchmarks on how this affects
performance, Andreas or Alice do you think you can give a bit of compute
time, once this patchset is more mature?
> + // 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,
> + }
> + }
> +}
> +
> +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 {
> + 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> {
I think we should give `Unique` the invariant that it is unique, making
this function `unsafe`, since the caller must ensure that only this
unique has access to the pointee.
---
Cheers,
Benno
> + if let Some(pointer) = NonNull::new(ptr) {
> + Some(Unique {
> + pointer,
> + _marker: PhantomData,
> + })
> + } else {
> + None
> + }
> + }
Powered by blists - more mailing lists