Message-Id: <DB92I10114UN.33MAFJVWIX4AB@kernel.org>
Date: Fri, 11 Jul 2025 10:03:07 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <lkmm@...ts.linux.dev>,
 <linux-arch@...r.kernel.org>
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>, "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>,
 "Alan Stern" <stern@...land.harvard.edu>
Subject: Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> new file mode 100644
> index 000000000000..e044fe21b128
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/generic.rs
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic atomic primitives.
> +
> +use super::ops::*;
> +use super::ordering::*;
> +use crate::build_error;
> +use core::cell::UnsafeCell;
> +
> +/// A generic atomic variable.

How about we copy upstream rust on this:

    A memory location which can be safely modified from multiple threads.

> +///
> +/// `T` must impl [`AllowAtomic`], that is, an [`AtomicImpl`] has to be chosen.

s/impl/implement/

I don't really think this sentence is that valuable... I think you could
mention several things before this:

* compatibility with LKMM (or that it is implemented through LKMM
  atomics)
* "what is an atomic"
* how big (relative to `T`) is this type? what about alignment?

> +///
> +/// # Examples
> +///
> +/// A customized type stored in [`Atomic`]:
> +///
> +/// ```rust
> +/// use kernel::sync::atomic::{generic::AllowAtomic, Atomic, Relaxed};
> +///
> +/// #[derive(Clone, Copy, PartialEq, Eq)]
> +/// #[repr(i32)]
> +/// enum State {
> +///     Uninit = 0,
> +///     Working = 1,
> +///     Done = 2,
> +/// };
> +///
> +/// // SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip
> +/// // transmutable to `i32`.
> +/// unsafe impl AllowAtomic for State {
> +///     type Repr = i32;
> +/// }
> +///
> +/// let s = Atomic::new(State::Uninit);
> +///
> +/// assert_eq!(State::Uninit, s.load(Relaxed));
> +/// ```

This example doesn't really seem like a good idea on this type... Maybe
on `AllowAtomic` instead? This type should just have examples of how to
use `Atomic<T>`.

> +///
> +/// # Guarantees
> +///
> +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race,
> +/// this is guaranteed by the safety requirement of [`Self::from_ptr()`] and the extra safety
> +/// requirement of the usage on pointers returned by [`Self::as_ptr()`].

I'd rather think we turn this into an invariant that says any operations
on `self.0` through a shared reference is atomic.

> +#[repr(transparent)]
> +pub struct Atomic<T: AllowAtomic>(UnsafeCell<T>);
> +
> +// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> +unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
> +
> +/// Types that support basic atomic operations.
> +///
> +/// # Round-trip transmutability

This can stay here for the moment, but it should probably be somewhere
more central.

> +///
> +/// Implementing [`AllowAtomic`] requires that the type is round-trip transmutable to its
> +/// representation:

I would remove this sentence and just define round-trip transmutability
in a standalone fashion.

> +///
> +/// - Any value of [`Self`] must be sound to [`transmute()`] to a [`Self::Repr`], and this also
> +///   means that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`] for reading.
> +/// - If a value of [`Self::Repr`] is a result a [`transmute()`] from a [`Self`], it must be
> +///   sound to [`transmute()`] the value back to a [`Self`].

This seems a bit verbose. How about this:

    `T` is round-trip transmutable to `U` if and only if all of these properties hold:
    * Any valid bit pattern for `T` is also a valid bit pattern for `U`.
    * Transmuting a value of type `T` to `U` and then to `T` again yields a value that is in all aspects
      equivalent to the original value.

> +///
> +/// This essentially means a valid bit pattern of `T: AllowAtomic` has to be a valid bit pattern
> +/// of `T::Repr`. This is needed because [`Atomic<T: AllowAtomic>`] operates on `T::Repr` to
> +/// implement atomic operations on `T`.
> +///
> +/// Note that this is more relaxed than bidirectional transmutability (i.e. [`transmute()`] is
> +/// always sound between `T` and `T::Repr`) because of the support for atomic variables over

s/between `T` and `T::Repr`/from `T` to `T::Repr` and back/

> +/// unit-only enums:

What are "unit-only" enums? Do you mean enums that don't have associated
data?

> +///
> +/// ```
> +/// #[repr(i32)]
> +/// enum State { Init = 0, Working = 1, Done = 2, }
> +/// ```
> +///
> +/// # Safety
> +///
> +/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
> +/// - [`Self`] and [`Self::Repr`] must have the [round-trip transmutability].

s/have the/be/
s/transmutability/transmutable/

> +///
> +/// # Limitations
> +///
> +/// Because C primitives are used to implement the atomic operations, and a C function requires a
> +/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
> +/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,
> +/// u16)` (a tuple with a `MaybeUninit` hole in it) are currently not supported. Note that
> +/// technically these types can be supported if some APIs are removed for them and the inner
> +/// implementation is tweaked, but the justification of support such a type is not strong enough at
> +/// the moment. This should be resolved if there is an implementation for `MaybeUninit<i32>` as
> +/// `AtomicImpl`.
> +///
> +/// [`transmute()`]: core::mem::transmute
> +/// [round-trip transmutability]: AllowAtomic#round-trip-transmutability
> +pub unsafe trait AllowAtomic: Sized + Send + Copy {
> +    /// The backing atomic implementation type.
> +    type Repr: AtomicImpl;
> +}
> +
> +#[inline(always)]
> +const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
> +    // SAFETY: Per the safety requirement of `AllowAtomic`, the transmute operation is sound.
> +    unsafe { core::mem::transmute_copy(&v) }
> +}
> +
> +/// # Safety
> +///
> +/// `r` must be a valid bit pattern of `T`.
> +#[inline(always)]
> +const unsafe fn from_repr<T: AllowAtomic>(r: T::Repr) -> T {
> +    // SAFETY: Per the safety requirement of the function, the transmute operation is sound.
> +    unsafe { core::mem::transmute_copy(&r) }
> +}
> +
> +impl<T: AllowAtomic> Atomic<T> {
> +    /// Creates a new atomic.

s/atomic/atomic `T`/

> +    pub const fn new(v: T) -> Self {
> +        Self(UnsafeCell::new(v))
> +    }
> +
> +    /// Creates a reference to [`Self`] from a pointer.

s/[`Self`]/an atomic `T`/

> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` has to be a valid pointer.

s/has to be a/is/
s/pointer//

> +    /// - `ptr` has to be valid for both reads and writes for the whole lifetime `'a`.

s/has to be/is/
s/both//
s/the whole lifetime//

> +    /// - For the duration of `'a`, other accesses to the object cannot cause data races (defined

s/the object/`*ptr`/
s/cannot/must not/

> +    ///   by [`LKMM`]) against atomic operations on the returned reference. Note that if all other
> +    ///   accesses are atomic, then this safety requirement is trivially fulfilled.
> +    ///
> +    /// [`LKMM`]: srctree/tools/memory-model
> +    ///
> +    /// # Examples
> +    ///
> +    /// Using [`Atomic::from_ptr()`] combined with [`Atomic::load()`] or [`Atomic::store()`] can
> +    /// achieve the same functionality as `READ_ONCE()`/`smp_load_acquire()` or
> +    /// `WRITE_ONCE()`/`smp_store_release()` in C side:
> +    ///
> +    /// ```rust

`rust` is the default, so you can just omit it.

> +    /// # use kernel::types::Opaque;
> +    /// use kernel::sync::atomic::{Atomic, Relaxed, Release};
> +    ///
> +    /// // Assume there is a C struct `Foo`.

s/F/f/

> +    /// mod cbindings {
> +    ///     #[repr(C)]
> +    ///     pub(crate) struct foo { pub(crate) a: i32, pub(crate) b: i32 }

Why not format this normally?

> +    /// }
> +    ///
> +    /// let tmp = Opaque::new(cbindings::foo { a: 1, b: 2});

Missing space before `}`.

> +    ///
> +    /// // struct foo *foo_ptr = ..;
> +    /// let foo_ptr = tmp.get();
> +    ///
> +    /// // SAFETY: `foo_ptr` is a valid pointer, and `.a` is in bounds.
> +    /// let foo_a_ptr = unsafe { &raw mut (*foo_ptr).a };
> +    ///
> +    /// // a = READ_ONCE(foo_ptr->a);
> +    /// //
> +    /// // SAFETY: `foo_a_ptr` is a valid pointer for read, and all accesses on it is atomic, so no
> +    /// // data race.
> +    /// let a = unsafe { Atomic::from_ptr(foo_a_ptr) }.load(Relaxed);
> +    /// # assert_eq!(a, 1);
> +    ///
> +    /// // smp_store_release(&foo_ptr->a, 2);
> +    /// //
> +    /// // SAFETY: `foo_a_ptr` is a valid pointer for write, and all accesses on it is atomic, so
> +    /// // no data race.
> +    /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
> +    /// ```
> +    ///
> +    /// However, this should be only used when communicating with C side or manipulating a C struct.
> +    pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
> +    where
> +        T: Sync,
> +    {
> +        // CAST: `T` is transparent to `Atomic<T>`.
> +        // SAFETY: Per function safety requirement, `ptr` is a valid pointer and the object will
> +        // live long enough. It's safe to return a `&Atomic<T>` because function safety requirement
> +        // guarantees other accesses won't cause data races.
> +        unsafe { &*ptr.cast::<Self>() }
> +    }
> +
> +    /// Returns a pointer to the underlying atomic variable.
> +    ///
> +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
> +    /// cannot cause data races defined by [`LKMM`].

I don't think this is correct. I could create an atomic and then share
it with the C side via this function, since I have exclusive access, the
writes to this pointer don't need to be atomic.

We also don't document additional postconditions like this... If you
really would have to do it like this (which you shouldn't given the
example above), you would have to make this function `unsafe`, otherwise
there is no way to ensure that people adhere to it (since it isn't part
of the safety docs).

> +    ///
> +    /// [`LKMM`]: srctree/tools/memory-model
> +    pub const fn as_ptr(&self) -> *mut T {
> +        self.0.get()
> +    }
> +
> +    /// Returns a mutable reference to the underlying atomic variable.
> +    ///
> +    /// This is safe because the mutable reference of the atomic variable guarantees the exclusive
> +    /// access.
> +    pub fn get_mut(&mut self) -> &mut T {
> +        // SAFETY: `self.as_ptr()` is a valid pointer to `T`. `&mut self` guarantees the exclusive
> +        // access, so it's safe to reborrow mutably.
> +        unsafe { &mut *self.as_ptr() }
> +    }
> +}
> +
> +impl<T: AllowAtomic> Atomic<T>
> +where
> +    T::Repr: AtomicHasBasicOps,
> +{
> +    /// Loads the value from the atomic variable.

s/variable/`T`/

> +    ///
> +    /// # Examples
> +    ///
> +    /// Simple usages:

I would remove this.

> +    ///
> +    /// ```rust
> +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> +    ///
> +    /// let x = Atomic::new(42i32);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    ///
> +    /// let x = Atomic::new(42i64);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    /// ```
> +    #[doc(alias("atomic_read", "atomic64_read"))]
> +    #[inline(always)]
> +    pub fn load<Ordering: AcquireOrRelaxed>(&self, _: Ordering) -> T {
> +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
> +        // valid pointer of `T::Repr`.

Well not exactly... The cast is fine due to the round-trip
transmutability, but you're not allowed to write arbitrary values to it.
Only values that are transmutable to `T`. So it is valid for reads and
valid for writes of values transmutable to `T`.

> +        let a = self.as_ptr().cast::<T::Repr>();
> +
> +        // SAFETY:
> +        // - For calling the atomic_read*() function:
> +        //   - `a` is a valid pointer for the function per the CAST justification above.
> +        //   - Per the type guarantees, the following atomic operation won't cause data races.

Which type guarantees? `Self`?

> +        // - For extra safety requirement of usage on pointers returned by `self.as_ptr()`:
> +        //   - Atomic operations are used here.
> +        let v = unsafe {
> +            match Ordering::TYPE {
> +                OrderingType::Relaxed => T::Repr::atomic_read(a),
> +                OrderingType::Acquire => T::Repr::atomic_read_acquire(a),
> +                _ => build_error!("Wrong ordering"),
> +            }
> +        };
> +
> +        // SAFETY: The atomic variable holds a valid `T`, so `v` is a valid bit pattern of `T`,
> +        // therefore it's safe to call `from_repr()`.

    // SAFETY: `v` comes from reading `a` which was derived from `self.as_ptr()` which points at a
    // valid `T`.

> +        unsafe { from_repr(v) }
> +    }
> +
> +    /// Stores a value to the atomic variable.

s/variable/`T`/

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```rust
> +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> +    ///
> +    /// let x = Atomic::new(42i32);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    ///
> +    /// x.store(43, Relaxed);
> +    ///
> +    /// assert_eq!(43, x.load(Relaxed));
> +    /// ```
> +    #[doc(alias("atomic_set", "atomic64_set"))]
> +    #[inline(always)]
> +    pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
> +        let v = into_repr(v);
> +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
> +        // valid pointer of `T::Repr`.

Ditto.

> +        let a = self.as_ptr().cast::<T::Repr>();
> +
> +        // SAFETY:
> +        // - For calling the atomic_set*() function:
> +        //   - `a` is a valid pointer for the function per the CAST justification above.
> +        //   - Per the type guarantees, the following atomic operation won't cause data races.
> +        // - For extra safety requirement of usage on pointers returned by `self.as_ptr()`:
> +        //   - Atomic operations are used here.
> +        // - For the bit validity of `Atomic<T>`:
> +        //   - `v` is a valid bit pattern of `T`, so it's sound to store it in an `Atomic<T>`.

Ditto.

---
Cheers,
Benno

> +        unsafe {
> +            match Ordering::TYPE {
> +                OrderingType::Relaxed => T::Repr::atomic_set(a, v),
> +                OrderingType::Release => T::Repr::atomic_set_release(a, v),
> +                _ => build_error!("Wrong ordering"),
> +            }
> +        };
> +    }
> +}

Powered by blists - more mailing lists