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

Benno Lossin
2025-07-10T11:04:45Z

On Thu Jul 10, 2025 at 8:00 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>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Overall this looks good from a functionality view. I have some cosmetic
comments for the macros below and a possibly bigger concern regarding
safety comments. But I think this is good enough for now, so:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> diff --git a/rust/kernel/sync/atomic/ops.rs b/rust/kernel/sync/atomic/ops.rs
> new file mode 100644
> index 000000000000..da04dd383962
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/ops.rs
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Atomic implementations.
> +//!
> +//! Provides 1:1 mapping of atomic implementations.
> +
> +use crate::bindings::*;

We shouldn't import all bindings, just use `bindings::` below.

> +// This macro generates the function signature with given argument list and return type.
> +macro_rules! declare_atomic_method {
> + (
> + $func:ident($($arg:ident : $arg_type:ty),*) $(-> $ret:ty)?
> + ) => {
> + paste!(
> + #[doc = concat!("Atomic ", stringify!($func))]
> + #[doc = "# Safety"]
> + #[doc = "- Any pointer passed to the function has to be a valid pointer"]
> + #[doc = "- Accesses must not cause data races per LKMM:"]
> + #[doc = " - Atomic read racing with normal read, normal write or atomic write is not data race."]

s/not/not a/

> + #[doc = " - Atomic write racing with normal read or normal write is data-race, unless the"]

s/data-race/a data race/

> + #[doc = " normal accesses are done at C side and considered as immune to data"]

#[doc = " normal access is done from the C side and considered immune to data"]

> + #[doc = " races, e.g. CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC."]

Missing '`'.

Also why aren't you using `///` instead of `#[doc =`? The only part
where you need interpolation is the first one.

> + unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)?;
> + );
> + };

> +declare_and_impl_atomic_methods!(
> + AtomicHasBasicOps ("Basic atomic operations") {
> + read[acquire](ptr: *mut Self) -> Self {
> + call(ptr.cast())
> + }
> +
> + set[release](ptr: *mut Self, v: Self) {
> + call(ptr.cast(), v)
> + }
> + }

I think this would look a bit better:

/// Basic atomic operations.
pub trait AtomicHasBasicOps {
unsafe fn read[acquire](ptr: *mut Self) -> Self {
bindings::#call(ptr.cast())
}

unsafe fn set[release](ptr: *mut Self, v: Self) {
bindings::#call(ptr.cast(), v)
}
}

And then we could also put the safety comments inline:

/// Basic atomic operations.
pub trait AtomicHasBasicOps {
/// Atomic read
///
/// # Safety
/// - Any pointer passed to the function has to be a valid pointer
/// - Accesses must not cause data races per LKMM:
/// - Atomic read racing with normal read, normal write or atomic write is not a data race.
/// - Atomic write racing with normal read or normal write is a data race, unless the
/// normal access is done from the C side and considered immune to data races, e.g.
/// `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
unsafe fn read[acquire](ptr: *mut Self) -> Self {
// SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
// cause data race per LKMM.
unsafe { bindings::#call(ptr.cast()) }
}

/// Atomic read
///
/// # Safety
/// - Any pointer passed to the function has to be a valid pointer
/// - Accesses must not cause data races per LKMM:
/// - Atomic read racing with normal read, normal write or atomic write is not a data race.
/// - Atomic write racing with normal read or normal write is a data race, unless the
/// normal access is done from the C side and considered immune to data races, e.g.
/// `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
unsafe fn set[release](ptr: *mut Self, v: Self) {
// SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
// cause data race per LKMM.
unsafe { bindings::#call(ptr.cast(), v) }
}
}

I'm not sure if this is worth it, but for reading the definitions of
these operations directly in the code this is going to be a lot more
readable. I don't think it's too bad to duplicate it.

I'm also not fully satisfied with the safety comment on
`bindings::#call`...

---
Cheers,
Benno

> +);
> +
> +declare_and_impl_atomic_methods!(
> + AtomicHasXchgOps ("Exchange and compare-and-exchange atomic operations") {
> + xchg[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> + call(ptr.cast(), v)
> + }
> +
> + try_cmpxchg[acquire, release, relaxed](ptr: *mut Self, old: *mut Self, new: Self) -> bool {
> + call(ptr.cast(), old, new)
> + }
> + }
> +);
> +
> +declare_and_impl_atomic_methods!(
> + AtomicHasArithmeticOps ("Atomic arithmetic operations") {
> + add[](ptr: *mut Self, v: Self) {
> + call(v, ptr.cast())
> + }
> +
> + fetch_add[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> + call(v, ptr.cast())
> + }
> + }
> +);