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>
---
 MAINTAINERS                    |   4 +-
 rust/kernel/sync.rs            |   1 +
 rust/kernel/sync/atomic.rs     |  19 ++++
 rust/kernel/sync/atomic/ops.rs | 195 +++++++++++++++++++++++++++++++++
 4 files changed, 218 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/sync/atomic.rs
 create mode 100644 rust/kernel/sync/atomic/ops.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 0c1d245bf7b8..5eef524975ca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3894,7 +3894,7 @@ F:	drivers/input/touchscreen/atmel_mxt_ts.c
 ATOMIC INFRASTRUCTURE
 M:	Will Deacon <will@kernel.org>
 M:	Peter Zijlstra <peterz@infradead.org>
-R:	Boqun Feng <boqun.feng@gmail.com>
+M:	Boqun Feng <boqun.feng@gmail.com>
 R:	Mark Rutland <mark.rutland@arm.com>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
@@ -3903,6 +3903,8 @@ F:	arch/*/include/asm/atomic*.h
 F:	include/*/atomic*.h
 F:	include/linux/refcount.h
 F:	scripts/atomic/
+F:	rust/kernel/sync/atomic.rs
+F:	rust/kernel/sync/atomic/
 
 ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER
 M:	Bradley Grove <linuxdrivers@attotech.com>
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 36a719015583..b620027e0641 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -10,6 +10,7 @@
 use pin_init;
 
 mod arc;
+pub mod atomic;
 mod condvar;
 pub mod lock;
 mod locked_by;
diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
new file mode 100644
index 000000000000..c9c7c3617dd5
--- /dev/null
+++ b/rust/kernel/sync/atomic.rs
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Atomic primitives.
+//!
+//! These primitives have the same semantics as their C counterparts: and the precise definitions of
+//! semantics can be found at [`LKMM`]. Note that Linux Kernel Memory (Consistency) Model is the
+//! only model for Rust code in kernel, and Rust's own atomics should be avoided.
+//!
+//! # Data races
+//!
+//! [`LKMM`] atomics have different rules regarding data races:
+//!
+//! - A normal write from C side is treated as an atomic write if
+//!   CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y.
+//! - Mixed-size atomic accesses don't cause data races.
+//!
+//! [`LKMM`]: srctree/tools/memory-model/
+
+pub mod ops;
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::*;
+use crate::macros::paste;
+
+mod private {
+    /// Sealed trait marker to disable customized impls on atomic implementation traits.
+    pub trait Sealed {}
+}
+
+// `i32` and `i64` are only supported atomic implementations.
+impl private::Sealed for i32 {}
+impl private::Sealed for i64 {}
+
+/// A marker trait for types that implement atomic operations with C side primitives.
+///
+/// This trait is sealed, and only types that have directly mapping to the C side atomics should
+/// impl this:
+///
+/// - `i32` maps to `atomic_t`.
+/// - `i64` maps to `atomic64_t`.
+pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {}
+
+// `atomic_t` implements atomic operations on `i32`.
+impl AtomicImpl for i32 {}
+
+// `atomic64_t` implements atomic operations on `i64`.
+impl AtomicImpl for i64 {}
+
+// 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."]
+            #[doc = "  - Atomic write racing with normal read or normal write is data-race, unless the"]
+            #[doc = "    normal accesses are done at C side and considered as immune to data"]
+            #[doc = "    races, e.g. CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC."]
+            unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)?;
+        );
+    };
+    (
+        $func:ident [$variant:ident $($rest:ident)*]($($arg_sig:tt)*) $(-> $ret:ty)?
+    ) => {
+        paste!(
+            declare_atomic_method!(
+                [< $func _ $variant >]($($arg_sig)*) $(-> $ret)?
+            );
+        );
+
+        declare_atomic_method!(
+            $func [$($rest)*]($($arg_sig)*) $(-> $ret)?
+        );
+    };
+    (
+        $func:ident []($($arg_sig:tt)*) $(-> $ret:ty)?
+    ) => {
+        declare_atomic_method!(
+            $func($($arg_sig)*) $(-> $ret)?
+        );
+    }
+}
+
+// This macro generates the function implementation with given argument list and return type, and it
+// will replace "call(...)" expression with "$ctype _ $func" to call the real C function.
+macro_rules! impl_atomic_method {
+    (
+        ($ctype:ident) $func:ident($($arg:ident: $arg_type:ty),*) $(-> $ret:ty)? {
+            call($($c_arg:expr),*)
+        }
+    ) => {
+        paste!(
+            #[inline(always)]
+            unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)? {
+                // SAFETY: Per function safety requirement, all pointers are valid, and accesses
+                // won't cause data race per LKMM.
+                unsafe { [< $ctype _ $func >]($($c_arg,)*) }
+            }
+        );
+    };
+    (
+        ($ctype:ident) $func:ident[$variant:ident $($rest:ident)*]($($arg_sig:tt)*) $(-> $ret:ty)? {
+            call($($arg:tt)*)
+        }
+    ) => {
+        paste!(
+            impl_atomic_method!(
+                ($ctype) [< $func _ $variant >]($($arg_sig)*) $( -> $ret)? {
+                    call($($arg)*)
+            }
+            );
+        );
+        impl_atomic_method!(
+            ($ctype) $func [$($rest)*]($($arg_sig)*) $( -> $ret)? {
+                call($($arg)*)
+            }
+        );
+    };
+    (
+        ($ctype:ident) $func:ident[]($($arg_sig:tt)*) $( -> $ret:ty)? {
+            call($($arg:tt)*)
+        }
+    ) => {
+        impl_atomic_method!(
+            ($ctype) $func($($arg_sig)*) $(-> $ret)? {
+                call($($arg)*)
+            }
+        );
+    }
+}
+
+// Delcares $ops trait with methods and implements the trait for `i32` and `i64`.
+macro_rules! declare_and_impl_atomic_methods {
+    ($ops:ident ($doc:literal) {
+        $(
+            $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
+                call($($arg:tt)*)
+            }
+        )*
+    }) => {
+        #[doc = $doc]
+        pub trait $ops: AtomicImpl {
+            $(
+                declare_atomic_method!(
+                    $func[$($variant)*]($($arg_sig)*) $(-> $ret)?
+                );
+            )*
+        }
+
+        impl $ops for i32 {
+            $(
+                impl_atomic_method!(
+                    (atomic) $func[$($variant)*]($($arg_sig)*) $(-> $ret)? {
+                        call($($arg)*)
+                    }
+                );
+            )*
+        }
+
+        impl $ops for i64 {
+            $(
+                impl_atomic_method!(
+                    (atomic64) $func[$($variant)*]($($arg_sig)*) $(-> $ret)? {
+                        call($($arg)*)
+                    }
+                );
+            )*
+        }
+    }
+}
+
+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)
+        }
+    }
+);
+
+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())
+        }
+    }
+);
-- 
2.39.5 (Apple Git-154)

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

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

On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> 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:
>> > +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 {

You should allow any kind of attribute (and multiple), that makes it
much simpler.

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

Yeah I also meant hardcoding them.

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

Sure, I feel like some kind of method duplication macro might be much
better here, like:

    multi_functions! {
        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, read_acquire>](ptr: *mut Self) -> Self;

            // ...
        }
    }

And then also allow it on impls. I don't really like the idea of
duplicating and thus hiding the safety docs... But I also see that just
copy pasting them everywhere isn't really a good solution either...

---
Cheers,
Benno

On Thu, Jul 10, 2025 at 05:46:56PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> > 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:
> >> > +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 {
> 
> You should allow any kind of attribute (and multiple), that makes it
> much simpler.
> 

I didn't know I could do that, updated:

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

Thanks!

> >         $(
> >             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.
[..]
> >> 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?
> 
> Sure, I feel like some kind of method duplication macro might be much
> better here, like:
> 
>     multi_functions! {
>         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, read_acquire>](ptr: *mut Self) -> Self;
> 
>             // ...
>         }
>     }
> 
> And then also allow it on impls. I don't really like the idea of
> duplicating and thus hiding the safety docs... But I also see that just

At least the rustdoc has safety section for each function. ;-)

> copy pasting them everywhere isn't really a good solution either...
> 

Yeah, perhaps there is no immediate resolution, but open to any
suggestion.

Regards,
Boqun

> ---
> Cheers,
> Benno

On Thu Jul 10, 2025 at 6:16 PM CEST, Boqun Feng wrote:
> At least the rustdoc has safety section for each function. ;-)

I don't usually use rustdoc to read function safety sections. Instead I
jump to their definition and read the code.

---
Cheers,
Benno

On Thu, Jul 10, 2025 at 09:21:17PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 6:16 PM CEST, Boqun Feng wrote:
> > At least the rustdoc has safety section for each function. ;-)
> 
> I don't usually use rustdoc to read function safety sections. Instead I
> jump to their definition and read the code.
> 

Understood. It's probalby the first time we use macros to generate a few
unsafe functions, so this is something new.

But let me ask you a question, as a programmer yourself, when you run
into a code base, and see something repeat in a similar pattern for 10+
times, what's your instinct? You would try to combine the them together,
right? That's why duplication seems not compelling to me. But surely, we
don't need to draw conclusion right now, however that's my opinion.
 
Regards,
Boqun

> ---
> Cheers,
> Benno

On Thu Jul 10, 2025 at 10:29 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 09:21:17PM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 6:16 PM CEST, Boqun Feng wrote:
>> > At least the rustdoc has safety section for each function. ;-)
>> 
>> I don't usually use rustdoc to read function safety sections. Instead I
>> jump to their definition and read the code.
>> 
>
> Understood. It's probalby the first time we use macros to generate a few
> unsafe functions, so this is something new.
>
> But let me ask you a question, as a programmer yourself, when you run
> into a code base, and see something repeat in a similar pattern for 10+
> times, what's your instinct? You would try to combine the them together,
> right? That's why duplication seems not compelling to me. But surely, we
> don't need to draw conclusion right now, however that's my opinion.

It all depends, if the definition never changes for a long time, I don't
mind the duplication. It's probably more effort to write macros to then
have less overall code.

It also makes it harder to read especially wrt safety docs. So if you
think you will have to make constant tweaks & additions to these, then
we should go a macro-heavy route. But if not, then I think making it
more readable is the way to go.

---
Cheers,
Benno