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

Boqun Feng
2025-07-10T15:12:46Z

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

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