Message-ID: <aG_Yah5FFHcA3IZy@Mac.home>
Date: Thu, 10 Jul 2025 08:12:42 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
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>, 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 01:04:38PM +0200, Benno Lossin wrote:
> 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>
> 

Thanks!

> > 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.
> 

Make sense!

> > +// 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 '`'.
> 

Fixed.

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

I think at a certain point I was not using `paste!()` and then using
`///` wouldn't generate them into rustdoc, but with `paste!()` your
suggestion makes sense, thanks!

> > +            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)
>         }
>     }
> 

Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
hard-coded:

macro_rules! declare_and_impl_atomic_methods {
    (#[doc = $doc:expr] pub trait $ops:ident {
        $(
            unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
                bindings::#call($($arg:tt)*)
            }
        )*
    }) => {

It shouldn't be very hard to make use of the actual visibility or
unsafe, but we currently don't have other visibility or safe function,
so it's simple to keep it as it is.

> 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

Copy-pasta ;-)

>         ///
>         /// # 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`...
> 

Based on the above, I'm not going to do the change (i.e. duplicating
the safe comments and improve them), and I would make an issue tracking
it, and we can revisit it when we have time. Sounds good?

Regards,
Boqun

> ---
> 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