Message-ID: <aFjj8AV668pl9jLN@Mac.home>
Date: Sun, 22 Jun 2025 22:19:44 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	lkmm@...ts.linux.dev, linux-arch@...r.kernel.org,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, Will Deacon <will@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Mark Rutland <mark.rutland@....com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Lyude Paul <lyude@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Mitchell Levy <levymitchell0@...il.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v5 04/10] rust: sync: atomic: Add generic atomics

On Sat, Jun 21, 2025 at 12:32:12PM +0100, Gary Guo wrote:
[...]
> > +#[repr(transparent)]
> > +pub struct Atomic<T: AllowAtomic>(Opaque<T>);
> 
> This should store `Opaque<T::Repr>` instead.
> 

"should" is a strong word ;-) If we still use `into_repr`/`from_repr`
it's a bit impossible, because Atomic::new() wants to be a const
function, so it requires const_trait_impl I believe.

If we require transmutability as a safety requirement for `AllowAtomic`,
then either `T` or `T::Repr` is fine.

> The implementation below essentially assumes that this is
> `Opaque<T::Repr>`:
> * atomic ops cast this to `*mut T::Repr`
> * load/store operates on `T::Repr` then converts to `T` with
>   `T::from_repr`/`T::into_repr`.
> 

Note that we only require one direction of strong transmutability, that
is: for every `T`, it must be able to safe transmute to a `T::Repr`, for
`T::Repr` -> `T` transmutation, only if it's a result of a `transmute<T,
T::Repr>()`. This is mostly due to potential support for unit-only enum.
E.g. using an atomic variable to represent a finite state.

> Note tha the transparent new types restriction on `AllowAtomic` is not
> sufficient for this, as I can define
> 

Nice catch! I do agree we should disallow `MyWeirdI32`, and I also agree
that we should put transmutability as safety requirement for
`AllowAtomic`. However, I would suggest we still keep
`into_repr`/`from_repr`, and require the implementation to make them
provide the same results as transmute(), as a correctness precondition
(instead of a safety precondition), in other words, you can still write
a `MyWeirdI32`, and it won't cause safety issues, but it'll be
incorrect.

The reason why I think we should keep `into_repr`/`from_repr` but add
a correctness precondition is that they are easily to implement as safe
code for basic types, so it'll be better than a transmute() call. Also
considering `Atomic<*mut T>`, would transmuting between integers and
pointers act the same as expose_provenance() and
from_exposed_provenance()?


So something like this for `AllowAtomic`, implementation-wise, no need
to change.

    /// Atomics that support basic atomic operations.
    ///
    /// Implementers must guarantee that `into_repr()` and `from_repr()` provide the same results as
    /// transmute between [`Self`] and [`Self::Repr`].
    ///
    /// TODO: Currently the [`AllowAtomic`] types are restricted within basic integer types (and their
    /// transparent new types). In the future, we could extend the scope to more data types when there
    /// is a clear and meaningful usage, but for now, [`AllowAtomic`] should only be implemented inside
    /// atomic mod for the restricted types mentioned above.
    ///
    /// # Safety
    ///
    /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
    /// - Any value of [`Self`] must be safe to [`transmute()`] to a [`Self::Repr`], this also means
    ///   that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`].
    /// - If a value of [`Self::Repr`] is a result a [`transmute()`] from a [`Self`], it must be safe
    ///   to [`transmute()`] the value back to a [`Self`].
    ///   that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`].
    /// - The implementer must guarantee it's safe to transfer ownership from one execution context to
    ///   another, this means it has to be a [`Send`], but because `*mut T` is not [`Send`] and that's
    ///   the basic type needs to support atomic operations, so this safety requirement is added to
    ///   [`AllowAtomic`] trait. This safety requirement is automatically satisfied if the type is a
    ///   [`Send`].
    ///
    /// [`transmute()`]: core::mem::transmute
    pub unsafe trait AllowAtomic: Sized + Copy {

Thoughts?

Regards,
Boqun

> #[repr(transparent)]
> struct MyWeirdI32(pub i32);
> 
> impl AllowAtomic for MyWeirdI32 {
>     type Repr = i32;
> 
>     fn into_repr(self) -> Self::Repr {
>         !self
>     }
> 
>     fn from_repr(repr: Self::Repr) -> Self {
>         !self
>     }
> }
> 
> Then `Atomic<MyWeirdI32>::new(MyWeirdI32(0)).load(Relaxed)` will give me
> `MyWeirdI32(-1)`.
> 
> Alternatively, we should remove `into_repr`/`from_repr` and always cast
> instead. In such case, `AllowAtomic` needs to have the transmutability
> as a safety precondition.
> 
[...]

Powered by blists - more mailing lists