Message-Id: <DB8BQGJNFDAY.BGQ8CZSFFOLH@kernel.org>
Date: Thu, 10 Jul 2025 13:04:38 +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 2/9] rust: sync: Add basic atomic operation mapping
 framework

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@...gle.com>
> Signed-off-by: Boqun Feng <boqun.feng@...il.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@...nel.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())
> +        }
> +    }
> +);

Powered by blists - more mailing lists