To provide using LKMM atomics for Rust code, a generic `Atomic<T>` is
added, currently `T` needs to be Send + Copy because these are the
straightforward usages and all basic types support this.

Implement `AllowAtomic` for `i32` and `i64`, and so far only basic
operations load() and store() are introduced.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs         |  14 ++
 rust/kernel/sync/atomic/generic.rs | 285 +++++++++++++++++++++++++++++
 2 files changed, 299 insertions(+)
 create mode 100644 rust/kernel/sync/atomic/generic.rs

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index e80ac049f36b..c5193c1c90fe 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -16,7 +16,21 @@
 //!
 //! [`LKMM`]: srctree/tools/memory-model/
 
+pub mod generic;
 pub mod ops;
 pub mod ordering;
 
+pub use generic::Atomic;
 pub use ordering::{Acquire, Full, Relaxed, Release};
+
+// SAFETY: `i32` has the same size and alignment with itself, and is round-trip transmutable to
+// itself.
+unsafe impl generic::AllowAtomic for i32 {
+    type Repr = i32;
+}
+
+// SAFETY: `i64` has the same size and alignment with itself, and is round-trip transmutable to
+// itself.
+unsafe impl generic::AllowAtomic for i64 {
+    type Repr = i64;
+}
diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
new file mode 100644
index 000000000000..b3e07328d857
--- /dev/null
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic atomic primitives.
+
+use super::ops::{AtomicHasBasicOps, AtomicImpl};
+use super::{ordering, ordering::OrderingType};
+use crate::build_error;
+use core::cell::UnsafeCell;
+
+/// A memory location which can be safely modified from multiple execution contexts.
+///
+/// This has the same size, alignment and bit validity as the underlying type `T`.
+///
+/// The atomic operations are implemented in a way that is fully compatible with the [Linux Kernel
+/// Memory (Consistency) Model][LKMM], hence they should be modeled as the corresponding
+/// [`LKMM`][LKMM] atomic primitives. With the help of [`Atomic::from_ptr()`] and
+/// [`Atomic::as_ptr()`], this provides a way to interact with [C-side atomic operations]
+/// (including those without the `atomic` prefix, e.g. `READ_ONCE()`, `WRITE_ONCE()`,
+/// `smp_load_acquire()` and `smp_store_release()`).
+///
+/// [LKMM]: srctree/tools/memory-model/
+/// [C-side atomic operations]: srctree/Documentation/atomic_t.txt
+#[repr(transparent)]
+pub struct Atomic<T: AllowAtomic>(UnsafeCell<T>);
+
+// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
+unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
+
+/// Types that support basic atomic operations.
+///
+/// # Round-trip transmutability
+///
+/// `T` is round-trip transmutable to `U` if and only if both of these properties hold:
+///
+/// - Any valid bit pattern for `T` is also a valid bit pattern for `U`.
+/// - Transmuting (e.g. using [`transmute()`]) a value of type `T` to `U` and then to `T` again
+///   yields a value that is in all aspects equivalent to the original value.
+///
+/// # Safety
+///
+/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
+/// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
+///
+/// Note that this is more relaxed than requiring the bi-directional transmutability (i.e.
+/// [`transmute()`] is always sound between `U` to `T`) because of the support for atomic variables
+/// over unit-only enums, see [Examples].
+///
+/// # Limitations
+///
+/// Because C primitives are used to implement the atomic operations, and a C function requires a
+/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
+/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,
+/// u16)` (a tuple with a `MaybeUninit` hole in it) are currently not supported. Note that
+/// technically these types can be supported if some APIs are removed for them and the inner
+/// implementation is tweaked, but the justification of support such a type is not strong enough at
+/// the moment. This should be resolved if there is an implementation for `MaybeUninit<i32>` as
+/// `AtomicImpl`.
+///
+/// # Examples
+///
+/// A unit-only enum that implements [`AllowAtomic`]:
+///
+/// ```
+/// use kernel::sync::atomic::{generic::AllowAtomic, Atomic, Relaxed};
+///
+/// #[derive(Clone, Copy, PartialEq, Eq)]
+/// #[repr(i32)]
+/// enum State {
+///     Uninit = 0,
+///     Working = 1,
+///     Done = 2,
+/// };
+///
+/// // SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip
+/// // transmutable to `i32`.
+/// unsafe impl AllowAtomic for State {
+///     type Repr = i32;
+/// }
+///
+/// let s = Atomic::new(State::Uninit);
+///
+/// assert_eq!(State::Uninit, s.load(Relaxed));
+/// ```
+/// [`transmute()`]: core::mem::transmute
+/// [round-trip transmutable]: AllowAtomic#round-trip-transmutability
+/// [Examples]: AllowAtomic#examples
+pub unsafe trait AllowAtomic: Sized + Send + Copy {
+    /// The backing atomic implementation type.
+    type Repr: AtomicImpl;
+}
+
+#[inline(always)]
+const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
+    // SAFETY: Per the safety requirement of `AllowAtomic`, the transmute operation is sound.
+    unsafe { core::mem::transmute_copy(&v) }
+}
+
+/// # Safety
+///
+/// `r` must be a valid bit pattern of `T`.
+#[inline(always)]
+const unsafe fn from_repr<T: AllowAtomic>(r: T::Repr) -> T {
+    // SAFETY: Per the safety requirement of the function, the transmute operation is sound.
+    unsafe { core::mem::transmute_copy(&r) }
+}
+
+impl<T: AllowAtomic> Atomic<T> {
+    /// Creates a new atomic `T`.
+    pub const fn new(v: T) -> Self {
+        Self(UnsafeCell::new(v))
+    }
+
+    /// Creates a reference to an atomic `T` from a pointer of `T`.
+    ///
+    /// # Safety
+    ///
+    /// - `ptr` is aligned to `align_of::<T>()`.
+    /// - `ptr` is valid for reads and writes for `'a`.
+    /// - For the duration of `'a`, other accesses to `*ptr` must not cause data races (defined
+    ///   by [`LKMM`]) against atomic operations on the returned reference. Note that if all other
+    ///   accesses are atomic, then this safety requirement is trivially fulfilled.
+    ///
+    /// [`LKMM`]: srctree/tools/memory-model
+    ///
+    /// # Examples
+    ///
+    /// Using [`Atomic::from_ptr()`] combined with [`Atomic::load()`] or [`Atomic::store()`] can
+    /// achieve the same functionality as `READ_ONCE()`/`smp_load_acquire()` or
+    /// `WRITE_ONCE()`/`smp_store_release()` in C side:
+    ///
+    /// ```
+    /// # use kernel::types::Opaque;
+    /// use kernel::sync::atomic::{Atomic, Relaxed, Release};
+    ///
+    /// // Assume there is a C struct `foo`.
+    /// mod cbindings {
+    ///     #[repr(C)]
+    ///     pub(crate) struct foo {
+    ///         pub(crate) a: i32,
+    ///         pub(crate) b: i32
+    ///     }
+    /// }
+    ///
+    /// let tmp = Opaque::new(cbindings::foo { a: 1, b: 2 });
+    ///
+    /// // struct foo *foo_ptr = ..;
+    /// let foo_ptr = tmp.get();
+    ///
+    /// // SAFETY: `foo_ptr` is valid, and `.a` is in bounds.
+    /// let foo_a_ptr = unsafe { &raw mut (*foo_ptr).a };
+    ///
+    /// // a = READ_ONCE(foo_ptr->a);
+    /// //
+    /// // SAFETY: `foo_a_ptr` is valid for read, and all other accesses on it is atomic, so no
+    /// // data race.
+    /// let a = unsafe { Atomic::from_ptr(foo_a_ptr) }.load(Relaxed);
+    /// # assert_eq!(a, 1);
+    ///
+    /// // smp_store_release(&foo_ptr->a, 2);
+    /// //
+    /// // SAFETY: `foo_a_ptr` is valid for writes, and all other accesses on it is atomic, so
+    /// // no data race.
+    /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
+    /// ```
+    ///
+    /// However, this should be only used when communicating with C side or manipulating a C
+    /// struct.
+    pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
+    where
+        T: Sync,
+    {
+        // CAST: `T` is transparent to `Atomic<T>`.
+        // SAFETY: Per function safety requirement, `ptr` is a valid pointer and the object will
+        // live long enough. It's safe to return a `&Atomic<T>` because function safety requirement
+        // guarantees other accesses won't cause data races.
+        unsafe { &*ptr.cast::<Self>() }
+    }
+
+    /// Returns a pointer to the underlying atomic `T`.
+    ///
+    /// Note that use of the return pointer must not cause data races defined by [`LKMM`].
+    ///
+    /// # Guarantees
+    ///
+    /// The returned pointer is properly aligned (i.e. aligned to [`align_of::<T>()`])
+    ///
+    /// [`LKMM`]: srctree/tools/memory-model
+    /// [`align_of::<T>()`]: core::mem::align_of
+    pub const fn as_ptr(&self) -> *mut T {
+        // GUARANTEE: `self.0` has the same alignment of `T`.
+        self.0.get()
+    }
+
+    /// Returns a mutable reference to the underlying atomic `T`.
+    ///
+    /// This is safe because the mutable reference of the atomic `T` guarantees the exclusive
+    /// access.
+    pub fn get_mut(&mut self) -> &mut T {
+        // SAFETY: `self.as_ptr()` is a valid pointer to `T`. `&mut self` guarantees the exclusive
+        // access, so it's safe to reborrow mutably.
+        unsafe { &mut *self.as_ptr() }
+    }
+}
+
+impl<T: AllowAtomic> Atomic<T>
+where
+    T::Repr: AtomicHasBasicOps,
+{
+    /// Loads the value from the atomic `T`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::sync::atomic::{Atomic, Relaxed};
+    ///
+    /// let x = Atomic::new(42i32);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// let x = Atomic::new(42i64);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    /// ```
+    #[doc(alias("atomic_read", "atomic64_read"))]
+    #[inline(always)]
+    pub fn load<Ordering: ordering::AcquireOrRelaxed>(&self, _: Ordering) -> T {
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is a valid
+        // pointer of `T::Repr` for reads and valid for writes of values transmutable to `T`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - `a` is aligned to `align_of::<T::Repr>()` because of the safety requirement of
+        //   `AllowAtomic` and the guarantee of `Atomic::as_ptr()`.
+        // - `a` is a valid pointer per the CAST justification above.
+        let v = unsafe {
+            match Ordering::TYPE {
+                OrderingType::Relaxed => T::Repr::atomic_read(a),
+                OrderingType::Acquire => T::Repr::atomic_read_acquire(a),
+                _ => build_error!("Wrong ordering"),
+            }
+        };
+
+        // SAFETY: `v` comes from reading `a` which was derived from `self.as_ptr()` which points
+        // at a valid `T`.
+        unsafe { from_repr(v) }
+    }
+
+    /// Stores a value to the atomic `T`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::sync::atomic::{Atomic, Relaxed};
+    ///
+    /// let x = Atomic::new(42i32);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// x.store(43, Relaxed);
+    ///
+    /// assert_eq!(43, x.load(Relaxed));
+    /// ```
+    #[doc(alias("atomic_set", "atomic64_set"))]
+    #[inline(always)]
+    pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
+        let v = into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is a valid
+        // pointer of `T::Repr` for reads and valid for writes of values transmutable to `T`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // `*self` remains valid after `atomic_set*()` because `v` is transmutable to `T`.
+        //
+        // SAFETY:
+        // - `a` is aligned to `align_of::<T::Repr>()` because of the safety requirement of
+        //   `AllowAtomic` and the guarantee of `Atomic::as_ptr()`.
+        // - `a` is a valid pointer per the CAST justification above.
+        unsafe {
+            match Ordering::TYPE {
+                OrderingType::Relaxed => T::Repr::atomic_set(a, v),
+                OrderingType::Release => T::Repr::atomic_set_release(a, v),
+                _ => build_error!("Wrong ordering"),
+            }
+        };
+    }
+}
-- 
2.39.5 (Apple Git-154)

On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> To provide using LKMM atomics for Rust code, a generic `Atomic<T>` is
> added, currently `T` needs to be Send + Copy because these are the
> straightforward usages and all basic types support this.
>
> Implement `AllowAtomic` for `i32` and `i64`, and so far only basic
> operations load() and store() are introduced.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/sync/atomic.rs         |  14 ++
>  rust/kernel/sync/atomic/generic.rs | 285 +++++++++++++++++++++++++++++
>  2 files changed, 299 insertions(+)
>  create mode 100644 rust/kernel/sync/atomic/generic.rs
>
> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
> index e80ac049f36b..c5193c1c90fe 100644
> --- a/rust/kernel/sync/atomic.rs
> +++ b/rust/kernel/sync/atomic.rs
> @@ -16,7 +16,21 @@
>  //!
>  //! [`LKMM`]: srctree/tools/memory-model/
>  
> +pub mod generic;

Hmm, maybe just re-export the stuff? I don't think there's an advantage
to having the generic module be public.

>  pub mod ops;
>  pub mod ordering;
>  
> +pub use generic::Atomic;
>  pub use ordering::{Acquire, Full, Relaxed, Release};
> +
> +// SAFETY: `i32` has the same size and alignment with itself, and is round-trip transmutable to
> +// itself.
> +unsafe impl generic::AllowAtomic for i32 {
> +    type Repr = i32;
> +}
> +
> +// SAFETY: `i64` has the same size and alignment with itself, and is round-trip transmutable to
> +// itself.
> +unsafe impl generic::AllowAtomic for i64 {
> +    type Repr = i64;
> +}
> diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> new file mode 100644
> index 000000000000..b3e07328d857
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/generic.rs
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic atomic primitives.
> +
> +use super::ops::{AtomicHasBasicOps, AtomicImpl};
> +use super::{ordering, ordering::OrderingType};
> +use crate::build_error;
> +use core::cell::UnsafeCell;
> +
> +/// A memory location which can be safely modified from multiple execution contexts.
> +///
> +/// This has the same size, alignment and bit validity as the underlying type `T`.

Let's also mention that this disables any niche optimizations (due to
the unsafe cell).

> +///
> +/// The atomic operations are implemented in a way that is fully compatible with the [Linux Kernel
> +/// Memory (Consistency) Model][LKMM], hence they should be modeled as the corresponding
> +/// [`LKMM`][LKMM] atomic primitives. With the help of [`Atomic::from_ptr()`] and
> +/// [`Atomic::as_ptr()`], this provides a way to interact with [C-side atomic operations]
> +/// (including those without the `atomic` prefix, e.g. `READ_ONCE()`, `WRITE_ONCE()`,
> +/// `smp_load_acquire()` and `smp_store_release()`).
> +///
> +/// [LKMM]: srctree/tools/memory-model/
> +/// [C-side atomic operations]: srctree/Documentation/atomic_t.txt

Did you check that these links looks good in rustdoc?

> +#[repr(transparent)]
> +pub struct Atomic<T: AllowAtomic>(UnsafeCell<T>);
> +
> +// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> +unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
> +
> +/// Types that support basic atomic operations.
> +///
> +/// # Round-trip transmutability
> +///
> +/// `T` is round-trip transmutable to `U` if and only if both of these properties hold:
> +///
> +/// - Any valid bit pattern for `T` is also a valid bit pattern for `U`.
> +/// - Transmuting (e.g. using [`transmute()`]) a value of type `T` to `U` and then to `T` again
> +///   yields a value that is in all aspects equivalent to the original value.
> +///
> +/// # Safety
> +///
> +/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
> +/// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
> +///
> +/// Note that this is more relaxed than requiring the bi-directional transmutability (i.e.
> +/// [`transmute()`] is always sound between `U` to `T`) because of the support for atomic variables

s/ to / and /

> +/// over unit-only enums, see [Examples].
> +///
> +/// # Limitations
> +///
> +/// Because C primitives are used to implement the atomic operations, and a C function requires a
> +/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
> +/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,

s/no uninitialized/initialized/

> +/// u16)` (a tuple with a `MaybeUninit` hole in it) are currently not supported. Note that

s/a tuple with a `MaybeUninit` hole in it/padding bytes are uninitialized/

> +/// technically these types can be supported if some APIs are removed for them and the inner
> +/// implementation is tweaked, but the justification of support such a type is not strong enough at
> +/// the moment. This should be resolved if there is an implementation for `MaybeUninit<i32>` as
> +/// `AtomicImpl`.
> +///
> +/// # Examples
> +///
> +/// A unit-only enum that implements [`AllowAtomic`]:
> +///
> +/// ```
> +/// use kernel::sync::atomic::{generic::AllowAtomic, Atomic, Relaxed};
> +///
> +/// #[derive(Clone, Copy, PartialEq, Eq)]
> +/// #[repr(i32)]
> +/// enum State {
> +///     Uninit = 0,
> +///     Working = 1,
> +///     Done = 2,
> +/// };
> +///
> +/// // SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip
> +/// // transmutable to `i32`.
> +/// unsafe impl AllowAtomic for State {
> +///     type Repr = i32;
> +/// }
> +///
> +/// let s = Atomic::new(State::Uninit);
> +///
> +/// assert_eq!(State::Uninit, s.load(Relaxed));
> +/// ```
> +/// [`transmute()`]: core::mem::transmute
> +/// [round-trip transmutable]: AllowAtomic#round-trip-transmutability
> +/// [Examples]: AllowAtomic#examples
> +pub unsafe trait AllowAtomic: Sized + Send + Copy {
> +    /// The backing atomic implementation type.
> +    type Repr: AtomicImpl;
> +}
> +
> +#[inline(always)]
> +const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
> +    // SAFETY: Per the safety requirement of `AllowAtomic`, the transmute operation is sound.

Please explain the concrete parts of the safety requirements that you
are using here (ie round-trip-transmutability).

> +    unsafe { core::mem::transmute_copy(&v) }
> +}
> +
> +/// # Safety
> +///
> +/// `r` must be a valid bit pattern of `T`.
> +#[inline(always)]
> +const unsafe fn from_repr<T: AllowAtomic>(r: T::Repr) -> T {
> +    // SAFETY: Per the safety requirement of the function, the transmute operation is sound.
> +    unsafe { core::mem::transmute_copy(&r) }
> +}
> +
> +impl<T: AllowAtomic> Atomic<T> {
> +    /// Creates a new atomic `T`.
> +    pub const fn new(v: T) -> Self {
> +        Self(UnsafeCell::new(v))
> +    }
> +
> +    /// Creates a reference to an atomic `T` from a pointer of `T`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` is aligned to `align_of::<T>()`.
> +    /// - `ptr` is valid for reads and writes for `'a`.
> +    /// - For the duration of `'a`, other accesses to `*ptr` must not cause data races (defined
> +    ///   by [`LKMM`]) against atomic operations on the returned reference. Note that if all other
> +    ///   accesses are atomic, then this safety requirement is trivially fulfilled.
> +    ///
> +    /// [`LKMM`]: srctree/tools/memory-model
> +    ///
> +    /// # Examples
> +    ///
> +    /// Using [`Atomic::from_ptr()`] combined with [`Atomic::load()`] or [`Atomic::store()`] can
> +    /// achieve the same functionality as `READ_ONCE()`/`smp_load_acquire()` or
> +    /// `WRITE_ONCE()`/`smp_store_release()` in C side:
> +    ///
> +    /// ```
> +    /// # use kernel::types::Opaque;
> +    /// use kernel::sync::atomic::{Atomic, Relaxed, Release};
> +    ///
> +    /// // Assume there is a C struct `foo`.
> +    /// mod cbindings {
> +    ///     #[repr(C)]
> +    ///     pub(crate) struct foo {
> +    ///         pub(crate) a: i32,
> +    ///         pub(crate) b: i32
> +    ///     }
> +    /// }
> +    ///
> +    /// let tmp = Opaque::new(cbindings::foo { a: 1, b: 2 });
> +    ///
> +    /// // struct foo *foo_ptr = ..;
> +    /// let foo_ptr = tmp.get();
> +    ///
> +    /// // SAFETY: `foo_ptr` is valid, and `.a` is in bounds.
> +    /// let foo_a_ptr = unsafe { &raw mut (*foo_ptr).a };
> +    ///
> +    /// // a = READ_ONCE(foo_ptr->a);
> +    /// //
> +    /// // SAFETY: `foo_a_ptr` is valid for read, and all other accesses on it is atomic, so no
> +    /// // data race.
> +    /// let a = unsafe { Atomic::from_ptr(foo_a_ptr) }.load(Relaxed);
> +    /// # assert_eq!(a, 1);
> +    ///
> +    /// // smp_store_release(&foo_ptr->a, 2);
> +    /// //
> +    /// // SAFETY: `foo_a_ptr` is valid for writes, and all other accesses on it is atomic, so
> +    /// // no data race.
> +    /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
> +    /// ```
> +    ///
> +    /// However, this should be only used when communicating with C side or manipulating a C
> +    /// struct.

This sentence should be above the `Safety` section.

> +    pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
> +    where
> +        T: Sync,
> +    {
> +        // CAST: `T` is transparent to `Atomic<T>`.
> +        // SAFETY: Per function safety requirement, `ptr` is a valid pointer and the object will
> +        // live long enough. It's safe to return a `&Atomic<T>` because function safety requirement
> +        // guarantees other accesses won't cause data races.
> +        unsafe { &*ptr.cast::<Self>() }
> +    }
> +
> +    /// Returns a pointer to the underlying atomic `T`.
> +    ///
> +    /// Note that use of the return pointer must not cause data races defined by [`LKMM`].
> +    ///
> +    /// # Guarantees
> +    ///
> +    /// The returned pointer is properly aligned (i.e. aligned to [`align_of::<T>()`])

You really only need this guarantee? Not validity etc?

> +    ///
> +    /// [`LKMM`]: srctree/tools/memory-model
> +    /// [`align_of::<T>()`]: core::mem::align_of
> +    pub const fn as_ptr(&self) -> *mut T {
> +        // GUARANTEE: `self.0` has the same alignment of `T`.
> +        self.0.get()
> +    }
> +
> +    /// Returns a mutable reference to the underlying atomic `T`.
> +    ///
> +    /// This is safe because the mutable reference of the atomic `T` guarantees the exclusive

s/the exclusive/exclusive/

---
Cheers,
Benno

> +    /// access.
> +    pub fn get_mut(&mut self) -> &mut T {
> +        // SAFETY: `self.as_ptr()` is a valid pointer to `T`. `&mut self` guarantees the exclusive
> +        // access, so it's safe to reborrow mutably.
> +        unsafe { &mut *self.as_ptr() }
> +    }
> +}

On Mon, Jul 14, 2025 at 12:30:12PM +0200, Benno Lossin wrote:
> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> > To provide using LKMM atomics for Rust code, a generic `Atomic<T>` is
> > added, currently `T` needs to be Send + Copy because these are the
> > straightforward usages and all basic types support this.
> >
> > Implement `AllowAtomic` for `i32` and `i64`, and so far only basic
> > operations load() and store() are introduced.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/sync/atomic.rs         |  14 ++
> >  rust/kernel/sync/atomic/generic.rs | 285 +++++++++++++++++++++++++++++
> >  2 files changed, 299 insertions(+)
> >  create mode 100644 rust/kernel/sync/atomic/generic.rs
> >
> > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
> > index e80ac049f36b..c5193c1c90fe 100644
> > --- a/rust/kernel/sync/atomic.rs
> > +++ b/rust/kernel/sync/atomic.rs
> > @@ -16,7 +16,21 @@
> >  //!
> >  //! [`LKMM`]: srctree/tools/memory-model/
> >  
> > +pub mod generic;
> 
> Hmm, maybe just re-export the stuff? I don't think there's an advantage
> to having the generic module be public.
> 

If `generic` is not public, then in the kernel::sync::atomic page, it
won't should up, and there is no mentioning of struct `Atomic` either.

If I made it public and also re-export the `Atomic`, there would be a
"Re-export" section mentioning all the re-exports, so I will keep
`generic` unless you have some tricks that I don't know.

Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
stay under `generic` (instead of re-export them at `atomic` mod level)
because they are about the generic part of `Atomic`, right?

> >  pub mod ops;
> >  pub mod ordering;
> >  
> > +pub use generic::Atomic;
> >  pub use ordering::{Acquire, Full, Relaxed, Release};
> > +
[...]
> > +/// A memory location which can be safely modified from multiple execution contexts.
> > +///
> > +/// This has the same size, alignment and bit validity as the underlying type `T`.
> 
> Let's also mention that this disables any niche optimizations (due to
> the unsafe cell).
> 

Done

> > +///
> > +/// The atomic operations are implemented in a way that is fully compatible with the [Linux Kernel
> > +/// Memory (Consistency) Model][LKMM], hence they should be modeled as the corresponding
> > +/// [`LKMM`][LKMM] atomic primitives. With the help of [`Atomic::from_ptr()`] and
> > +/// [`Atomic::as_ptr()`], this provides a way to interact with [C-side atomic operations]
> > +/// (including those without the `atomic` prefix, e.g. `READ_ONCE()`, `WRITE_ONCE()`,
> > +/// `smp_load_acquire()` and `smp_store_release()`).
> > +///
> > +/// [LKMM]: srctree/tools/memory-model/
> > +/// [C-side atomic operations]: srctree/Documentation/atomic_t.txt
> 
> Did you check that these links looks good in rustdoc?
> 

Yep.

> > +#[repr(transparent)]
> > +pub struct Atomic<T: AllowAtomic>(UnsafeCell<T>);
> > +
> > +// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> > +unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
> > +
> > +/// Types that support basic atomic operations.
> > +///
> > +/// # Round-trip transmutability
> > +///
> > +/// `T` is round-trip transmutable to `U` if and only if both of these properties hold:
> > +///
> > +/// - Any valid bit pattern for `T` is also a valid bit pattern for `U`.
> > +/// - Transmuting (e.g. using [`transmute()`]) a value of type `T` to `U` and then to `T` again
> > +///   yields a value that is in all aspects equivalent to the original value.
> > +///
> > +/// # Safety
> > +///
> > +/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
> > +/// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
> > +///
> > +/// Note that this is more relaxed than requiring the bi-directional transmutability (i.e.
> > +/// [`transmute()`] is always sound between `U` to `T`) because of the support for atomic variables
> 
> s/ to / and /
> 

Fixed.

> > +/// over unit-only enums, see [Examples].
> > +///
> > +/// # Limitations
> > +///
> > +/// Because C primitives are used to implement the atomic operations, and a C function requires a
> > +/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
> > +/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,
> 
> s/no uninitialized/initialized/
> 

hmm.. "with initialized bits" seems to me saying "it's OK as long as
some bits are initialized", how about "with all the bits initialized"?

> > +/// u16)` (a tuple with a `MaybeUninit` hole in it) are currently not supported. Note that
> 
> s/a tuple with a `MaybeUninit` hole in it/padding bytes are uninitialized/
> 

Done.

[...]
> > +
> > +#[inline(always)]
> > +const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
> > +    // SAFETY: Per the safety requirement of `AllowAtomic`, the transmute operation is sound.
> 
> Please explain the concrete parts of the safety requirements that you
> are using here (ie round-trip-transmutability).
> 

Done.

> > +    unsafe { core::mem::transmute_copy(&v) }
> > +}
> > +
> > +/// # Safety
> > +///
> > +/// `r` must be a valid bit pattern of `T`.
> > +#[inline(always)]
> > +const unsafe fn from_repr<T: AllowAtomic>(r: T::Repr) -> T {
> > +    // SAFETY: Per the safety requirement of the function, the transmute operation is sound.
> > +    unsafe { core::mem::transmute_copy(&r) }
> > +}
> > +
> > +impl<T: AllowAtomic> Atomic<T> {
> > +    /// Creates a new atomic `T`.
> > +    pub const fn new(v: T) -> Self {
> > +        Self(UnsafeCell::new(v))
> > +    }
> > +
> > +    /// Creates a reference to an atomic `T` from a pointer of `T`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - `ptr` is aligned to `align_of::<T>()`.
> > +    /// - `ptr` is valid for reads and writes for `'a`.
> > +    /// - For the duration of `'a`, other accesses to `*ptr` must not cause data races (defined
> > +    ///   by [`LKMM`]) against atomic operations on the returned reference. Note that if all other
> > +    ///   accesses are atomic, then this safety requirement is trivially fulfilled.
> > +    ///
> > +    /// [`LKMM`]: srctree/tools/memory-model
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// Using [`Atomic::from_ptr()`] combined with [`Atomic::load()`] or [`Atomic::store()`] can
> > +    /// achieve the same functionality as `READ_ONCE()`/`smp_load_acquire()` or
> > +    /// `WRITE_ONCE()`/`smp_store_release()` in C side:
> > +    ///
> > +    /// ```
> > +    /// # use kernel::types::Opaque;
> > +    /// use kernel::sync::atomic::{Atomic, Relaxed, Release};
> > +    ///
> > +    /// // Assume there is a C struct `foo`.
> > +    /// mod cbindings {
> > +    ///     #[repr(C)]
> > +    ///     pub(crate) struct foo {
> > +    ///         pub(crate) a: i32,
> > +    ///         pub(crate) b: i32
> > +    ///     }
> > +    /// }
> > +    ///
> > +    /// let tmp = Opaque::new(cbindings::foo { a: 1, b: 2 });
> > +    ///
> > +    /// // struct foo *foo_ptr = ..;
> > +    /// let foo_ptr = tmp.get();
> > +    ///
> > +    /// // SAFETY: `foo_ptr` is valid, and `.a` is in bounds.
> > +    /// let foo_a_ptr = unsafe { &raw mut (*foo_ptr).a };
> > +    ///
> > +    /// // a = READ_ONCE(foo_ptr->a);
> > +    /// //
> > +    /// // SAFETY: `foo_a_ptr` is valid for read, and all other accesses on it is atomic, so no
> > +    /// // data race.
> > +    /// let a = unsafe { Atomic::from_ptr(foo_a_ptr) }.load(Relaxed);
> > +    /// # assert_eq!(a, 1);
> > +    ///
> > +    /// // smp_store_release(&foo_ptr->a, 2);
> > +    /// //
> > +    /// // SAFETY: `foo_a_ptr` is valid for writes, and all other accesses on it is atomic, so
> > +    /// // no data race.
> > +    /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
> > +    /// ```
> > +    ///
> > +    /// However, this should be only used when communicating with C side or manipulating a C
> > +    /// struct.
> 
> This sentence should be above the `Safety` section.
> 

Hmm.. why? This is the further information about what the above
"Examples" section just mentioned?

> > +    pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
> > +    where
> > +        T: Sync,
> > +    {
> > +        // CAST: `T` is transparent to `Atomic<T>`.
> > +        // SAFETY: Per function safety requirement, `ptr` is a valid pointer and the object will
> > +        // live long enough. It's safe to return a `&Atomic<T>` because function safety requirement
> > +        // guarantees other accesses won't cause data races.
> > +        unsafe { &*ptr.cast::<Self>() }
> > +    }
> > +
> > +    /// Returns a pointer to the underlying atomic `T`.
> > +    ///
> > +    /// Note that use of the return pointer must not cause data races defined by [`LKMM`].
> > +    ///
> > +    /// # Guarantees
> > +    ///
> > +    /// The returned pointer is properly aligned (i.e. aligned to [`align_of::<T>()`])
> 
> You really only need this guarantee? Not validity etc?
> 

Nice find, I changed it to also guarantee the pointer is valid.

> > +    ///
> > +    /// [`LKMM`]: srctree/tools/memory-model
> > +    /// [`align_of::<T>()`]: core::mem::align_of
> > +    pub const fn as_ptr(&self) -> *mut T {
> > +        // GUARANTEE: `self.0` has the same alignment of `T`.
> > +        self.0.get()
> > +    }
> > +
> > +    /// Returns a mutable reference to the underlying atomic `T`.
> > +    ///
> > +    /// This is safe because the mutable reference of the atomic `T` guarantees the exclusive
> 
> s/the exclusive/exclusive/
> 

Done.

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > +    /// access.
> > +    pub fn get_mut(&mut self) -> &mut T {
> > +        // SAFETY: `self.as_ptr()` is a valid pointer to `T`. `&mut self` guarantees the exclusive
> > +        // access, so it's safe to reborrow mutably.
> > +        unsafe { &mut *self.as_ptr() }
> > +    }
> > +}

On Mon Jul 14, 2025 at 4:21 PM CEST, Boqun Feng wrote:
> On Mon, Jul 14, 2025 at 12:30:12PM +0200, Benno Lossin wrote:
>> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
>> > To provide using LKMM atomics for Rust code, a generic `Atomic<T>` is
>> > added, currently `T` needs to be Send + Copy because these are the
>> > straightforward usages and all basic types support this.
>> >
>> > Implement `AllowAtomic` for `i32` and `i64`, and so far only basic
>> > operations load() and store() are introduced.
>> >
>> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> > ---
>> >  rust/kernel/sync/atomic.rs         |  14 ++
>> >  rust/kernel/sync/atomic/generic.rs | 285 +++++++++++++++++++++++++++++
>> >  2 files changed, 299 insertions(+)
>> >  create mode 100644 rust/kernel/sync/atomic/generic.rs
>> >
>> > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
>> > index e80ac049f36b..c5193c1c90fe 100644
>> > --- a/rust/kernel/sync/atomic.rs
>> > +++ b/rust/kernel/sync/atomic.rs
>> > @@ -16,7 +16,21 @@
>> >  //!
>> >  //! [`LKMM`]: srctree/tools/memory-model/
>> >  
>> > +pub mod generic;
>> 
>> Hmm, maybe just re-export the stuff? I don't think there's an advantage
>> to having the generic module be public.
>> 
>
> If `generic` is not public, then in the kernel::sync::atomic page, it
> won't should up, and there is no mentioning of struct `Atomic` either.
>
> If I made it public and also re-export the `Atomic`, there would be a
> "Re-export" section mentioning all the re-exports, so I will keep
> `generic` unless you have some tricks that I don't know.

Just use `#[doc(inline)]` :)

    https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#inline-and-no_inline

> Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
> stay under `generic` (instead of re-export them at `atomic` mod level)
> because they are about the generic part of `Atomic`, right?

Why is that more natural? It only adds an extra path layer in any import
for atomics.

Unless you at some point want to add `concrete::Atomic<T>` etc, I would
just re-export them.

>> > +/// The atomic operations are implemented in a way that is fully compatible with the [Linux Kernel
>> > +/// Memory (Consistency) Model][LKMM], hence they should be modeled as the corresponding
>> > +/// [`LKMM`][LKMM] atomic primitives. With the help of [`Atomic::from_ptr()`] and
>> > +/// [`Atomic::as_ptr()`], this provides a way to interact with [C-side atomic operations]
>> > +/// (including those without the `atomic` prefix, e.g. `READ_ONCE()`, `WRITE_ONCE()`,
>> > +/// `smp_load_acquire()` and `smp_store_release()`).
>> > +///
>> > +/// [LKMM]: srctree/tools/memory-model/
>> > +/// [C-side atomic operations]: srctree/Documentation/atomic_t.txt
>> 
>> Did you check that these links looks good in rustdoc?
>> 
>
> Yep.

Nice :)

>> > +/// over unit-only enums, see [Examples].
>> > +///
>> > +/// # Limitations
>> > +///
>> > +/// Because C primitives are used to implement the atomic operations, and a C function requires a
>> > +/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
>> > +/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,
>> 
>> s/no uninitialized/initialized/
>> 
>
> hmm.. "with initialized bits" seems to me saying "it's OK as long as
> some bits are initialized", how about "with all the bits initialized"?

Sounds good. The double negation sounded a bit weird to me.

>> > +    /// However, this should be only used when communicating with C side or manipulating a C
>> > +    /// struct.
>> 
>> This sentence should be above the `Safety` section.
>> 
>
> Hmm.. why? This is the further information about what the above
> "Examples" section just mentioned?

I thought "this" is referring to "this function", if not then please be
more concrete :)

---
Cheers,
Benno

On Mon, Jul 14, 2025 at 05:05:40PM +0200, Benno Lossin wrote:
[...]
> >> >  //!
> >> >  //! [`LKMM`]: srctree/tools/memory-model/
> >> >  
> >> > +pub mod generic;
> >> 
> >> Hmm, maybe just re-export the stuff? I don't think there's an advantage
> >> to having the generic module be public.
> >> 
> >
> > If `generic` is not public, then in the kernel::sync::atomic page, it
> > won't should up, and there is no mentioning of struct `Atomic` either.
> >
> > If I made it public and also re-export the `Atomic`, there would be a
> > "Re-export" section mentioning all the re-exports, so I will keep
> > `generic` unless you have some tricks that I don't know.
> 
> Just use `#[doc(inline)]` :)
> 
>     https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#inline-and-no_inline
> 
> > Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
> > stay under `generic` (instead of re-export them at `atomic` mod level)
> > because they are about the generic part of `Atomic`, right?
> 
> Why is that more natural? It only adds an extra path layer in any import
> for atomics.
> 

Exactly, users need to go through extra steps if they want to use the
"generic" part of the atomic, and I think that makes user more aware of
what they are essentially doing:

- If you want to use the predefined types for atomic, just

  use kernel::sync::atomic::Atomic;

  and just operate on an `Atomic<_>`.

- If you want to bring your own type for atomic operations, you need to

  use kernel::sync::atomic::generic::AllowAtomic;

  (essentially you go into the "generic" part of the atomic)

  and provide your own implementation for `AllowAtomic` and then you
  could use it for your own type.

I feel it's natural because for extra features you fetch more modules
in.

Regards,
Boqun

> Unless you at some point want to add `concrete::Atomic<T>` etc, I would
> just re-export them.
> 
[...]

On Mon Jul 14, 2025 at 5:32 PM CEST, Boqun Feng wrote:
> On Mon, Jul 14, 2025 at 05:05:40PM +0200, Benno Lossin wrote:
> [...]
>> >> >  //!
>> >> >  //! [`LKMM`]: srctree/tools/memory-model/
>> >> >  
>> >> > +pub mod generic;
>> >> 
>> >> Hmm, maybe just re-export the stuff? I don't think there's an advantage
>> >> to having the generic module be public.
>> >> 
>> >
>> > If `generic` is not public, then in the kernel::sync::atomic page, it
>> > won't should up, and there is no mentioning of struct `Atomic` either.
>> >
>> > If I made it public and also re-export the `Atomic`, there would be a
>> > "Re-export" section mentioning all the re-exports, so I will keep
>> > `generic` unless you have some tricks that I don't know.
>> 
>> Just use `#[doc(inline)]` :)
>> 
>>     https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#inline-and-no_inline
>> 
>> > Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
>> > stay under `generic` (instead of re-export them at `atomic` mod level)
>> > because they are about the generic part of `Atomic`, right?
>> 
>> Why is that more natural? It only adds an extra path layer in any import
>> for atomics.
>> 
>
> Exactly, users need to go through extra steps if they want to use the
> "generic" part of the atomic, and I think that makes user more aware of
> what they are essentially doing:
>
> - If you want to use the predefined types for atomic, just
>
>   use kernel::sync::atomic::Atomic;
>
>   and just operate on an `Atomic<_>`.
>
> - If you want to bring your own type for atomic operations, you need to
>
>   use kernel::sync::atomic::generic::AllowAtomic;
>
>   (essentially you go into the "generic" part of the atomic)
>
>   and provide your own implementation for `AllowAtomic` and then you
>   could use it for your own type.
>
> I feel it's natural because for extra features you fetch more modules
> in.

The same would apply if you had to import `AllowAtomic` from atomic
directly? I don't really see the value in this.

---
Cheers,
Benno

On Tue, Jul 15, 2025 at 11:36:49AM +0200, Benno Lossin wrote:
> On Mon Jul 14, 2025 at 5:32 PM CEST, Boqun Feng wrote:
> > On Mon, Jul 14, 2025 at 05:05:40PM +0200, Benno Lossin wrote:
> > [...]
> >> >> >  //!
> >> >> >  //! [`LKMM`]: srctree/tools/memory-model/
> >> >> >  
> >> >> > +pub mod generic;
> >> >> 
> >> >> Hmm, maybe just re-export the stuff? I don't think there's an advantage
> >> >> to having the generic module be public.
> >> >> 
> >> >
> >> > If `generic` is not public, then in the kernel::sync::atomic page, it
> >> > won't should up, and there is no mentioning of struct `Atomic` either.
> >> >
> >> > If I made it public and also re-export the `Atomic`, there would be a
> >> > "Re-export" section mentioning all the re-exports, so I will keep
> >> > `generic` unless you have some tricks that I don't know.
> >> 
> >> Just use `#[doc(inline)]` :)
> >> 
> >>     https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#inline-and-no_inline
> >> 
> >> > Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
> >> > stay under `generic` (instead of re-export them at `atomic` mod level)
> >> > because they are about the generic part of `Atomic`, right?
> >> 
> >> Why is that more natural? It only adds an extra path layer in any import
> >> for atomics.
> >> 
> >
> > Exactly, users need to go through extra steps if they want to use the
> > "generic" part of the atomic, and I think that makes user more aware of
> > what they are essentially doing:
> >
> > - If you want to use the predefined types for atomic, just
> >
> >   use kernel::sync::atomic::Atomic;
> >
> >   and just operate on an `Atomic<_>`.
> >
> > - If you want to bring your own type for atomic operations, you need to
> >
> >   use kernel::sync::atomic::generic::AllowAtomic;
> >
> >   (essentially you go into the "generic" part of the atomic)
> >
> >   and provide your own implementation for `AllowAtomic` and then you
> >   could use it for your own type.
> >
> > I feel it's natural because for extra features you fetch more modules
> > in.
> 
> The same would apply if you had to import `AllowAtomic` from atomic
> directly? I don't really see the value in this.
> 

Because generic::AllowAtomic is more clear that the user is using the
generic part of the API, or the user is extending the Atomic type with
the generic ability. Grouping functionality with a namespace is
basically what I want to achieve here. It's similar to why do we put
`atomic` (and `lock`) under `sync`.

Regards,
Boqun

> ---
> Cheers,
> Benno

On Tue Jul 15, 2025 at 3:14 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 11:36:49AM +0200, Benno Lossin wrote:
>> On Mon Jul 14, 2025 at 5:32 PM CEST, Boqun Feng wrote:
>> > On Mon, Jul 14, 2025 at 05:05:40PM +0200, Benno Lossin wrote:
>> > [...]
>> >> >> >  //!
>> >> >> >  //! [`LKMM`]: srctree/tools/memory-model/
>> >> >> >  
>> >> >> > +pub mod generic;
>> >> >> 
>> >> >> Hmm, maybe just re-export the stuff? I don't think there's an advantage
>> >> >> to having the generic module be public.
>> >> >> 
>> >> >
>> >> > If `generic` is not public, then in the kernel::sync::atomic page, it
>> >> > won't should up, and there is no mentioning of struct `Atomic` either.
>> >> >
>> >> > If I made it public and also re-export the `Atomic`, there would be a
>> >> > "Re-export" section mentioning all the re-exports, so I will keep
>> >> > `generic` unless you have some tricks that I don't know.
>> >> 
>> >> Just use `#[doc(inline)]` :)
>> >> 
>> >>     https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#inline-and-no_inline
>> >> 
>> >> > Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
>> >> > stay under `generic` (instead of re-export them at `atomic` mod level)
>> >> > because they are about the generic part of `Atomic`, right?
>> >> 
>> >> Why is that more natural? It only adds an extra path layer in any import
>> >> for atomics.
>> >> 
>> >
>> > Exactly, users need to go through extra steps if they want to use the
>> > "generic" part of the atomic, and I think that makes user more aware of
>> > what they are essentially doing:
>> >
>> > - If you want to use the predefined types for atomic, just
>> >
>> >   use kernel::sync::atomic::Atomic;
>> >
>> >   and just operate on an `Atomic<_>`.
>> >
>> > - If you want to bring your own type for atomic operations, you need to
>> >
>> >   use kernel::sync::atomic::generic::AllowAtomic;
>> >
>> >   (essentially you go into the "generic" part of the atomic)
>> >
>> >   and provide your own implementation for `AllowAtomic` and then you
>> >   could use it for your own type.
>> >
>> > I feel it's natural because for extra features you fetch more modules
>> > in.
>> 
>> The same would apply if you had to import `AllowAtomic` from atomic
>> directly? I don't really see the value in this.
>> 
>
> Because generic::AllowAtomic is more clear that the user is using the
> generic part of the API, or the user is extending the Atomic type with
> the generic ability. Grouping functionality with a namespace is
> basically what I want to achieve here. It's similar to why do we put
> `atomic` (and `lock`) under `sync`.

For `sync::{atomic, lock}` this makes sense, because `sync` might get
other submodules in the future (eg `once`). But for atomics, I don't see
any new modules similar to `generic` and even if, `AllowAtomic` still
makes sense in the top-level atomic module. I don't think the
namespacing argument makes sense in this case.

---
Cheers,
Benno

On Mon, Jul 14, 2025 at 4:22 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hmm.. why? This is the further information about what the above
> "Examples" section just mentioned?

I think Benno may be trying to point out is may be confusing what
"this" may be referring to, i.e. is it referring to something concrete
about the example, or about `from_ptr` as a whole / in all cases?

Cheers,
Miguel

On Mon, Jul 14, 2025 at 04:34:39PM +0200, Miguel Ojeda wrote:
> On Mon, Jul 14, 2025 at 4:22 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Hmm.. why? This is the further information about what the above
> > "Examples" section just mentioned?
> 
> I think Benno may be trying to point out is may be confusing what
> "this" may be referring to, i.e. is it referring to something concrete
> about the example, or about `from_ptr` as a whole / in all cases?
> 

Ok, how about I do this:

diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
index 26d9ff3f7c35..e40010394536 100644
--- a/rust/kernel/sync/atomic/generic.rs
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -135,6 +135,9 @@ pub const fn new(v: T) -> Self {

     /// Creates a reference to an atomic `T` from a pointer of `T`.
     ///
+    /// This usually is used when when communicating with C side or manipulating a C struct, see
+    /// examples below.
+    ///
     /// # Safety
     ///
     /// - `ptr` is aligned to `align_of::<T>()`.
@@ -185,9 +188,6 @@ pub const fn new(v: T) -> Self {
     /// // no data race.
     /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
     /// ```
-    ///
-    /// However, this should be only used when communicating with C side or manipulating a C
-    /// struct.
     pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
     where
         T: Sync,

?

Regards,
Boqun

> Cheers,
> Miguel

On Mon Jul 14, 2025 at 4:53 PM CEST, Boqun Feng wrote:
> On Mon, Jul 14, 2025 at 04:34:39PM +0200, Miguel Ojeda wrote:
>> On Mon, Jul 14, 2025 at 4:22 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>> >
>> > Hmm.. why? This is the further information about what the above
>> > "Examples" section just mentioned?
>> 
>> I think Benno may be trying to point out is may be confusing what
>> "this" may be referring to, i.e. is it referring to something concrete
>> about the example, or about `from_ptr` as a whole / in all cases?
>> 
>
> Ok, how about I do this:
>
> diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> index 26d9ff3f7c35..e40010394536 100644
> --- a/rust/kernel/sync/atomic/generic.rs
> +++ b/rust/kernel/sync/atomic/generic.rs
> @@ -135,6 +135,9 @@ pub const fn new(v: T) -> Self {
>
>      /// Creates a reference to an atomic `T` from a pointer of `T`.
>      ///
> +    /// This usually is used when when communicating with C side or manipulating a C struct, see
> +    /// examples below.

I don't think it's necessary to mention the examples below, but this is
what I had in mind.

---
Cheers,
Benno

> +    ///
>      /// # Safety
>      ///
>      /// - `ptr` is aligned to `align_of::<T>()`.
> @@ -185,9 +188,6 @@ pub const fn new(v: T) -> Self {
>      /// // no data race.
>      /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
>      /// ```
> -    ///
> -    /// However, this should be only used when communicating with C side or manipulating a C
> -    /// struct.
>      pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
>      where
>          T: Sync,
>
> ?
>
> Regards,
> Boqun
>
>> Cheers,
>> Miguel

On Mon, Jul 14, 2025 at 07:21:53AM -0700, Boqun Feng wrote:
> On Mon, Jul 14, 2025 at 12:30:12PM +0200, Benno Lossin wrote:
> > On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> > > To provide using LKMM atomics for Rust code, a generic `Atomic<T>` is
> > > added, currently `T` needs to be Send + Copy because these are the
> > > straightforward usages and all basic types support this.
> > >
> > > Implement `AllowAtomic` for `i32` and `i64`, and so far only basic
> > > operations load() and store() are introduced.
> > >
> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > ---
> > >  rust/kernel/sync/atomic.rs         |  14 ++
> > >  rust/kernel/sync/atomic/generic.rs | 285 +++++++++++++++++++++++++++++
> > >  2 files changed, 299 insertions(+)
> > >  create mode 100644 rust/kernel/sync/atomic/generic.rs
> > >
> > > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
> > > index e80ac049f36b..c5193c1c90fe 100644
> > > --- a/rust/kernel/sync/atomic.rs
> > > +++ b/rust/kernel/sync/atomic.rs
> > > @@ -16,7 +16,21 @@
> > >  //!
> > >  //! [`LKMM`]: srctree/tools/memory-model/
> > >  
> > > +pub mod generic;
> > 
> > Hmm, maybe just re-export the stuff? I don't think there's an advantage
> > to having the generic module be public.
> > 
> 
> If `generic` is not public, then in the kernel::sync::atomic page, it

I meant the rustdoc of `kernel::sync::atomic` page.

Regards,
Boqun

> won't should up, and there is no mentioning of struct `Atomic` either.
> 
> If I made it public and also re-export the `Atomic`, there would be a
> "Re-export" section mentioning all the re-exports, so I will keep
> `generic` unless you have some tricks that I don't know.
> 
> Also I feel it's a bit naturally that `AllowAtomic` and `AllowAtomicAdd`
> stay under `generic` (instead of re-export them at `atomic` mod level)
> because they are about the generic part of `Atomic`, right?
> 
[...]