Re: [PATCH v7 2/9] rust: sync: Add basic atomic operation mapping framework

["Benno Lossin" <lossin@kernel.org>]

On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> Preparation for generic atomic implementation. To unify the
> implementation of a generic method over `i32` and `i64`, the C side
> atomic methods need to be grouped so that in a generic method, they can
> be referred as <type>::<method>, otherwise their parameters and return
> value are different between `i32` and `i64`, which would require using
> `transmute()` to unify the type into a `T`.
>
> Introduce `AtomicImpl` to represent a basic type in Rust that has the
> direct mapping to an atomic implementation from C. This trait is sealed,
> and currently only `i32` and `i64` impl this.
>
> Further, different methods are put into different `*Ops` trait groups,
> and this is for the future when smaller types like `i8`/`i16` are
> supported but only with a limited set of API (e.g. only set(), load(),
> xchg() and cmpxchg(), no add() or sub() etc).
>
> While the atomic mod is introduced, documentation is also added for
> memory models and data races.
>
> Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect
> my responsiblity on the Rust atomic mod.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> Benno, I actually followed your suggestion and put the safety
> requirement inline, and also I realized I don't need to mention about
> data race, because no data race is an implied safety requirement.

Thanks! I think it looks much better :)

> Note that macro-wise, I forced only #[doc] attributes can be put
> before `unsafe fn ..` because this is the only usage, and I don't
> think it's likely we want to support other attributes. We can always
> add them later.

Sounds good.

> +declare_and_impl_atomic_methods!(
> +    /// Basic atomic operations
> +    pub trait AtomicHasBasicOps {

I think we should drop the `Has` from the names. So this one can just be
`AtomicBasicOps`. Or how about `BasicAtomic`, or `AtomicBase`?

> +        /// Atomic read (load).
> +        ///
> +        /// # Safety
> +        /// - `ptr` is aligned to [`align_of::<Self>()`].
> +        /// - `ptr` is valid for reads.
> +        ///
> +        /// [`align_of::<Self>()`]: core::mem::align_of
> +        unsafe fn read[acquire](ptr: *mut Self) -> Self {
> +            bindings::#call(ptr.cast())
> +        }
> +
> +        /// Atomic set (store).
> +        ///
> +        /// # Safety
> +        /// - `ptr` is aligned to [`align_of::<Self>()`].
> +        /// - `ptr` is valid for writes.
> +        ///
> +        /// [`align_of::<Self>()`]: core::mem::align_of
> +        unsafe fn set[release](ptr: *mut Self, v: Self) {
> +            bindings::#call(ptr.cast(), v)
> +        }
> +    }
> +);
> +
> +declare_and_impl_atomic_methods!(
> +    /// Exchange and compare-and-exchange atomic operations
> +    pub trait AtomicHasXchgOps {

Same here `AtomicXchgOps` or `AtomicExchangeOps` or `AtomicExchange`?
(I would prefer to not abbreviate it to `Xchg`)

> +        /// Atomic exchange.
> +        ///
> +        /// Atomically updates `*ptr` to `v` and returns the old value.
> +        ///
> +        /// # Safety
> +        /// - `ptr` is aligned to [`align_of::<Self>()`].
> +        /// - `ptr` is valid for reads and writes.
> +        ///
> +        /// [`align_of::<Self>()`]: core::mem::align_of
> +        unsafe fn xchg[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> +            bindings::#call(ptr.cast(), v)
> +        }
> +
> +        /// Atomic compare and exchange.
> +        ///
> +        /// If `*ptr` == `*old`, atomically updates `*ptr` to `new`. Otherwise, `*ptr` is not
> +        /// modified, `*old` is updated to the current value of `*ptr`.
> +        ///
> +        /// Return `true` if the update of `*ptr` occured, `false` otherwise.
> +        ///
> +        /// # Safety
> +        /// - `ptr` is aligned to [`align_of::<Self>()`].
> +        /// - `ptr` is valid for reads and writes.
> +        /// - `old` is aligned to [`align_of::<Self>()`].
> +        /// - `old` is valid for reads and writes.
> +        ///
> +        /// [`align_of::<Self>()`]: core::mem::align_of
> +        unsafe fn try_cmpxchg[acquire, release, relaxed](ptr: *mut Self, old: *mut Self, new: Self) -> bool {
> +            bindings::#call(ptr.cast(), old, new)
> +        )}
> +    }
> +);
> +
> +declare_and_impl_atomic_methods!(
> +    /// Atomic arithmetic operations
> +    pub trait AtomicHasArithmeticOps {

Forgot to rename this one to `Add`? I think `AtomicAdd` sounds best for
this one.

---
Cheers,
Benno

> +        /// Atomic add (wrapping).
> +        ///
> +        /// Atomically updates `*ptr` to `(*ptr).wrapping_add(v)`.
> +        ///
> +        /// # Safety
> +        /// - `ptr` is aligned to `align_of::<Self>()`.
> +        /// - `ptr` is valid for reads and writes.
> +        ///
> +        /// [`align_of::<Self>()`]: core::mem::align_of
> +        unsafe fn add[](ptr: *mut Self, v: Self::Delta) {
> +            bindings::#call(v, ptr.cast())
> +        }
> +
> +        /// Atomic fetch and add (wrapping).
> +        ///
> +        /// Atomically updates `*ptr` to `(*ptr).wrapping_add(v)`, and returns the value of `*ptr`
> +        /// before the update.
> +        ///
> +        /// # Safety
> +        /// - `ptr` is aligned to `align_of::<Self>()`.
> +        /// - `ptr` is valid for reads and writes.
> +        ///
> +        /// [`align_of::<Self>()`]: core::mem::align_of
> +        unsafe fn fetch_add[acquire, release, relaxed](ptr: *mut Self, v: Self::Delta) -> Self {
> +            bindings::#call(v, ptr.cast())
> +        }
> +    }
> +);