One important set of atomic operations is the arithmetic operations,
i.e. add(), sub(), fetch_add(), add_return(), etc. However it may not
make senses for all the types that `AllowAtomic` to have arithmetic
operations, for example a `Foo(u32)` may not have a reasonable add() or
sub(), plus subword types (`u8` and `u16`) currently don't have
atomic arithmetic operations even on C side and might not have them in
the future in Rust (because they are usually suboptimal on a few
architecures). Therefore the plan is to add a few subtraits of
`AllowAtomic` describing which types have and can do atomic arithemtic
operations.

One trait `AllowAtomicAdd` is added, and only add() and fetch_add() are
added. The rest will be added in the future.

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 | 111 ++++++++++++++++++++++++++++-
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index c5193c1c90fe..54f5b4618337 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -29,8 +29,22 @@ unsafe impl generic::AllowAtomic for i32 {
     type Repr = i32;
 }
 
+// SAFETY: The wrapping add result of two `i32`s is a valid `i32`.
+unsafe impl generic::AllowAtomicAdd<i32> for i32 {
+    fn rhs_into_delta(rhs: i32) -> i32 {
+        rhs
+    }
+}
+
 // 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;
 }
+
+// SAFETY: The wrapping add result of two `i64`s is a valid `i64`.
+unsafe impl generic::AllowAtomicAdd<i64> for i64 {
+    fn rhs_into_delta(rhs: i64) -> i64 {
+        rhs
+    }
+}
diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
index 4e45d594d8ef..9e2394017202 100644
--- a/rust/kernel/sync/atomic/generic.rs
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -2,7 +2,7 @@
 
 //! Generic atomic primitives.
 
-use super::ops::{AtomicHasBasicOps, AtomicHasXchgOps, AtomicImpl};
+use super::ops::{AtomicHasArithmeticOps, AtomicHasBasicOps, AtomicHasXchgOps, AtomicImpl};
 use super::{ordering, ordering::OrderingType};
 use crate::build_error;
 use core::cell::UnsafeCell;
@@ -104,6 +104,18 @@ const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
     unsafe { core::mem::transmute_copy(&r) }
 }
 
+/// Types that support atomic add operations.
+///
+/// # Safety
+///
+/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
+/// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
+/// yield a value with a bit pattern also valid for `Self`.
+pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
+    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
+    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
+}
+
 impl<T: AllowAtomic> Atomic<T> {
     /// Creates a new atomic `T`.
     pub const fn new(v: T) -> Self {
@@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
         ret
     }
 }
+
+impl<T: AllowAtomic> Atomic<T>
+where
+    T::Repr: AtomicHasArithmeticOps,
+{
+    /// Atomic add.
+    ///
+    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::sync::atomic::{Atomic, Relaxed};
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// x.add(12, Relaxed);
+    ///
+    /// assert_eq!(54, x.load(Relaxed));
+    /// ```
+    #[inline(always)]
+    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
+    where
+        T: AllowAtomicAdd<Rhs>,
+    {
+        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
+        // `AllowAtomicAdd`.
+        //
+        // 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 {
+            T::Repr::atomic_add(a, v);
+        }
+    }
+
+    /// Atomic fetch and add.
+    ///
+    /// Atomically updates `*self` to `(*self).wrapping_add(v)`, and returns the value of `*self`
+    /// before the update.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::sync::atomic::{Atomic, Acquire, Full, Relaxed};
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// assert_eq!(54, { x.fetch_add(12, Acquire); x.load(Relaxed) });
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// assert_eq!(54, { x.fetch_add(12, Full); x.load(Relaxed) } );
+    /// ```
+    #[inline(always)]
+    pub fn fetch_add<Rhs, Ordering: ordering::Any>(&self, v: Rhs, _: Ordering) -> T
+    where
+        T: AllowAtomicAdd<Rhs>,
+    {
+        let v = T::rhs_into_delta(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_fetch_add*()` because of the safety requirement of
+        // `AllowAtomicAdd`.
+        //
+        // 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 ret = unsafe {
+            match Ordering::TYPE {
+                OrderingType::Full => T::Repr::atomic_fetch_add(a, v),
+                OrderingType::Acquire => T::Repr::atomic_fetch_add_acquire(a, v),
+                OrderingType::Release => T::Repr::atomic_fetch_add_release(a, v),
+                OrderingType::Relaxed => T::Repr::atomic_fetch_add_relaxed(a, v),
+            }
+        };
+
+        // SAFETY: `ret` comes from reading `a` which was derived from `self.as_ptr()` which points
+        // at a valid `T`.
+        unsafe { from_repr(ret) }
+    }
+}
-- 
2.39.5 (Apple Git-154)

On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> +/// Types that support atomic add operations.
> +///
> +/// # Safety
> +///
> +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to

I don't like "wrapping adding", either we use "`wrapping_add`" or we use
some other phrasing.

> +/// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
> +/// yield a value with a bit pattern also valid for `Self`.
> +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {

Why `Allow*`? I think `AtomicAdd` is better?

> +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
> +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
> +}
> +
>  impl<T: AllowAtomic> Atomic<T> {
>      /// Creates a new atomic `T`.
>      pub const fn new(v: T) -> Self {
> @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
>          ret
>      }
>  }
> +
> +impl<T: AllowAtomic> Atomic<T>
> +where
> +    T::Repr: AtomicHasArithmeticOps,
> +{
> +    /// Atomic add.
> +    ///
> +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> +    ///
> +    /// let x = Atomic::new(42);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    ///
> +    /// x.add(12, Relaxed);
> +    ///
> +    /// assert_eq!(54, x.load(Relaxed));
> +    /// ```
> +    #[inline(always)]
> +    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
> +    where
> +        T: AllowAtomicAdd<Rhs>,
> +    {
> +        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
> +        // `AllowAtomicAdd`.

This part should be moved to the CAST comment above, since we're not
only writing a value transmuted from `T` into `*self`.

---
Cheers,
Benno

> +        //
> +        // 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 {
> +            T::Repr::atomic_add(a, v);
> +        }
> +    }

On Tue, Jul 15, 2025 at 01:21:20PM +0200, Benno Lossin wrote:
> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> > +/// Types that support atomic add operations.
> > +///
> > +/// # Safety
> > +///
> > +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
> 
> I don't like "wrapping adding", either we use "`wrapping_add`" or we use
> some other phrasing.
> 

Let's use `wrapping_add` then.

    /// `wrapping_add` any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
    /// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
    /// yield a value with a bit pattern also valid for `Self`.

> > +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
> 
> Why `Allow*`? I think `AtomicAdd` is better?
> 

To be consistent with `AllowAtomic` (the super trait), if we use
`AtomicAdd` here, should we change `AllowAtomic` to `AtomicBase`?

> > +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
> > +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
> > +}
> > +
> >  impl<T: AllowAtomic> Atomic<T> {
> >      /// Creates a new atomic `T`.
> >      pub const fn new(v: T) -> Self {
> > @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
> >          ret
> >      }
> >  }
> > +
> > +impl<T: AllowAtomic> Atomic<T>
> > +where
> > +    T::Repr: AtomicHasArithmeticOps,
> > +{
> > +    /// Atomic add.
> > +    ///
> > +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> > +    ///
> > +    /// let x = Atomic::new(42);
> > +    ///
> > +    /// assert_eq!(42, x.load(Relaxed));
> > +    ///
> > +    /// x.add(12, Relaxed);
> > +    ///
> > +    /// assert_eq!(54, x.load(Relaxed));
> > +    /// ```
> > +    #[inline(always)]
> > +    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
> > +    where
> > +        T: AllowAtomicAdd<Rhs>,
> > +    {
> > +        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
> > +        // `AllowAtomicAdd`.
> 
> This part should be moved to the CAST comment above, since we're not
> only writing a value transmuted from `T` into `*self`.
> 

Hmm.. the CAST comment should explain why a pointer of `T` can be a
valid pointer of `T::Repr` because the atomic_add() below is going to
read through the pointer and write value back. The comment starting with
"`*self`" explains the value written is a valid `T`, therefore
conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
into `a`.

Basically

// CAST
let a = ..

^ explains what `a` is a valid for and why it's valid.

// `*self` remains

^ explains that we write a valid value to `a`.


So I don't think we need to move?

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > +        //
> > +        // 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 {
> > +            T::Repr::atomic_add(a, v);
> > +        }
> > +    }

On Tue Jul 15, 2025 at 3:33 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 01:21:20PM +0200, Benno Lossin wrote:
>> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
>> > +/// Types that support atomic add operations.
>> > +///
>> > +/// # Safety
>> > +///
>> > +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>> 
>> I don't like "wrapping adding", either we use "`wrapping_add`" or we use
>> some other phrasing.
>> 
>
> Let's use `wrapping_add` then.
>
>     /// `wrapping_add` any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>     /// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
>     /// yield a value with a bit pattern also valid for `Self`.
>
>> > +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
>> 
>> Why `Allow*`? I think `AtomicAdd` is better?
>> 
>
> To be consistent with `AllowAtomic` (the super trait), if we use
> `AtomicAdd` here, should we change `AllowAtomic` to `AtomicBase`?

Ideally, we would name that trait just `Atomic` :) But it then
conflicts with the `Atomic<T>` struct (this would be motivation to put
them in different modules :). I like `AtomicBase` better than
`AllowAtomic`, but maybe there is a better name, how about `AtomicType`?

>> > +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
>> > +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
>> > +}
>> > +
>> >  impl<T: AllowAtomic> Atomic<T> {
>> >      /// Creates a new atomic `T`.
>> >      pub const fn new(v: T) -> Self {
>> > @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
>> >          ret
>> >      }
>> >  }
>> > +
>> > +impl<T: AllowAtomic> Atomic<T>
>> > +where
>> > +    T::Repr: AtomicHasArithmeticOps,
>> > +{
>> > +    /// Atomic add.
>> > +    ///
>> > +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
>> > +    ///
>> > +    /// # Examples
>> > +    ///
>> > +    /// ```
>> > +    /// use kernel::sync::atomic::{Atomic, Relaxed};
>> > +    ///
>> > +    /// let x = Atomic::new(42);
>> > +    ///
>> > +    /// assert_eq!(42, x.load(Relaxed));
>> > +    ///
>> > +    /// x.add(12, Relaxed);
>> > +    ///
>> > +    /// assert_eq!(54, x.load(Relaxed));
>> > +    /// ```
>> > +    #[inline(always)]
>> > +    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
>> > +    where
>> > +        T: AllowAtomicAdd<Rhs>,
>> > +    {
>> > +        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
>> > +        // `AllowAtomicAdd`.
>> 
>> This part should be moved to the CAST comment above, since we're not
>> only writing a value transmuted from `T` into `*self`.
>> 
>
> Hmm.. the CAST comment should explain why a pointer of `T` can be a
> valid pointer of `T::Repr` because the atomic_add() below is going to
> read through the pointer and write value back. The comment starting with
> "`*self`" explains the value written is a valid `T`, therefore
> conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
> into `a`.

I see, my interpretation was that if we put it on the cast, then the
operation that `atomic_add` does also is valid.

But I think this comment should either be part of the `CAST` or the
`SAFETY` comment. Going by your interpretation, it would make more sense
in the SAFETY one, since there you justify that you're actually writing
a value of type `T`.

---
Cheers,
Benno

> Basically
>
> // CAST
> let a = ..
>
> ^ explains what `a` is a valid for and why it's valid.
>
> // `*self` remains
>
> ^ explains that we write a valid value to `a`.
>
>
> So I don't think we need to move?

On Tue, Jul 15, 2025 at 05:45:34PM +0200, Benno Lossin wrote:
> On Tue Jul 15, 2025 at 3:33 PM CEST, Boqun Feng wrote:
> > On Tue, Jul 15, 2025 at 01:21:20PM +0200, Benno Lossin wrote:
> >> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
> >> > +/// Types that support atomic add operations.
> >> > +///
> >> > +/// # Safety
> >> > +///
> >> > +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
> >> 
> >> I don't like "wrapping adding", either we use "`wrapping_add`" or we use
> >> some other phrasing.
> >> 
> >
> > Let's use `wrapping_add` then.
> >
> >     /// `wrapping_add` any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
> >     /// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
> >     /// yield a value with a bit pattern also valid for `Self`.
> >
> >> > +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
> >> 
> >> Why `Allow*`? I think `AtomicAdd` is better?
> >> 
> >
> > To be consistent with `AllowAtomic` (the super trait), if we use
> > `AtomicAdd` here, should we change `AllowAtomic` to `AtomicBase`?
> 
> Ideally, we would name that trait just `Atomic` :) But it then
> conflicts with the `Atomic<T>` struct (this would be motivation to put
> them in different modules :). I like `AtomicBase` better than

Oh, if we move `Atomic<T>` to atomic.rs and keep atomic::generic, then
we can name it atomic::generic::Atomic ;-)

> `AllowAtomic`, but maybe there is a better name, how about `AtomicType`?
> 

AtomicType may be better than AtomicBase to me.

> >> > +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
> >> > +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
> >> > +}
> >> > +
> >> >  impl<T: AllowAtomic> Atomic<T> {
> >> >      /// Creates a new atomic `T`.
> >> >      pub const fn new(v: T) -> Self {
> >> > @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
> >> >          ret
> >> >      }
> >> >  }
> >> > +
> >> > +impl<T: AllowAtomic> Atomic<T>
> >> > +where
> >> > +    T::Repr: AtomicHasArithmeticOps,
> >> > +{
> >> > +    /// Atomic add.
> >> > +    ///
> >> > +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
> >> > +    ///
> >> > +    /// # Examples
> >> > +    ///
> >> > +    /// ```
> >> > +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> >> > +    ///
> >> > +    /// let x = Atomic::new(42);
> >> > +    ///
> >> > +    /// assert_eq!(42, x.load(Relaxed));
> >> > +    ///
> >> > +    /// x.add(12, Relaxed);
> >> > +    ///
> >> > +    /// assert_eq!(54, x.load(Relaxed));
> >> > +    /// ```
> >> > +    #[inline(always)]
> >> > +    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
> >> > +    where
> >> > +        T: AllowAtomicAdd<Rhs>,
> >> > +    {
> >> > +        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
> >> > +        // `AllowAtomicAdd`.
> >> 
> >> This part should be moved to the CAST comment above, since we're not
> >> only writing a value transmuted from `T` into `*self`.
> >> 
> >
> > Hmm.. the CAST comment should explain why a pointer of `T` can be a
> > valid pointer of `T::Repr` because the atomic_add() below is going to
> > read through the pointer and write value back. The comment starting with
> > "`*self`" explains the value written is a valid `T`, therefore
> > conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
> > into `a`.
> 
> I see, my interpretation was that if we put it on the cast, then the
> operation that `atomic_add` does also is valid.
> 
> But I think this comment should either be part of the `CAST` or the
> `SAFETY` comment. Going by your interpretation, it would make more sense
> in the SAFETY one, since there you justify that you're actually writing
> a value of type `T`.
> 

Hmm.. you're probably right. There are two safety things about
atomic_add():

- Whether calling it is safe
- Whether the operation on `a` (a pointer to `T` essentially) is safe.

How about the following:

        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
        // `AllowAtomicAdd`.
        //
        // SAFETY:
        // - For calling `atomic_add()`:
        //   - `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.
        // - For accessing `*a`: the value written is transmutable to `T`
        //   due to the safety requirement of `AllowAtomicAdd`.
        unsafe { T::Repr::atomic_add(a, v) };

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > Basically
> >
> > // CAST
> > let a = ..
> >
> > ^ explains what `a` is a valid for and why it's valid.
> >
> > // `*self` remains
> >
> > ^ explains that we write a valid value to `a`.
> >
> >
> > So I don't think we need to move?

On Tue Jul 15, 2025 at 6:13 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 05:45:34PM +0200, Benno Lossin wrote:
>> On Tue Jul 15, 2025 at 3:33 PM CEST, Boqun Feng wrote:
>> > On Tue, Jul 15, 2025 at 01:21:20PM +0200, Benno Lossin wrote:
>> >> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
>> >> > +/// Types that support atomic add operations.
>> >> > +///
>> >> > +/// # Safety
>> >> > +///
>> >> > +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>> >> 
>> >> I don't like "wrapping adding", either we use "`wrapping_add`" or we use
>> >> some other phrasing.
>> >> 
>> >
>> > Let's use `wrapping_add` then.
>> >
>> >     /// `wrapping_add` any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>> >     /// any value of type `Self::Repr` obtained through transmuting a value of type `Self` to must
>> >     /// yield a value with a bit pattern also valid for `Self`.
>> >
>> >> > +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
>> >> 
>> >> Why `Allow*`? I think `AtomicAdd` is better?
>> >> 
>> >
>> > To be consistent with `AllowAtomic` (the super trait), if we use
>> > `AtomicAdd` here, should we change `AllowAtomic` to `AtomicBase`?
>> 
>> Ideally, we would name that trait just `Atomic` :) But it then
>> conflicts with the `Atomic<T>` struct (this would be motivation to put
>> them in different modules :). I like `AtomicBase` better than
>
> Oh, if we move `Atomic<T>` to atomic.rs and keep atomic::generic, then
> we can name it atomic::generic::Atomic ;-)

That would be an argument for having the `generic` module :)

Well, I'm not so sure about having two types with the same name right
away, so maybe let's discuss this in our meeting.

>> `AllowAtomic`, but maybe there is a better name, how about `AtomicType`?
>> 
>
> AtomicType may be better than AtomicBase to me.

Yeah I like it better too.

>> >> > +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
>> >> > +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
>> >> > +}
>> >> > +
>> >> >  impl<T: AllowAtomic> Atomic<T> {
>> >> >      /// Creates a new atomic `T`.
>> >> >      pub const fn new(v: T) -> Self {
>> >> > @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
>> >> >          ret
>> >> >      }
>> >> >  }
>> >> > +
>> >> > +impl<T: AllowAtomic> Atomic<T>
>> >> > +where
>> >> > +    T::Repr: AtomicHasArithmeticOps,
>> >> > +{
>> >> > +    /// Atomic add.
>> >> > +    ///
>> >> > +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
>> >> > +    ///
>> >> > +    /// # Examples
>> >> > +    ///
>> >> > +    /// ```
>> >> > +    /// use kernel::sync::atomic::{Atomic, Relaxed};
>> >> > +    ///
>> >> > +    /// let x = Atomic::new(42);
>> >> > +    ///
>> >> > +    /// assert_eq!(42, x.load(Relaxed));
>> >> > +    ///
>> >> > +    /// x.add(12, Relaxed);
>> >> > +    ///
>> >> > +    /// assert_eq!(54, x.load(Relaxed));
>> >> > +    /// ```
>> >> > +    #[inline(always)]
>> >> > +    pub fn add<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
>> >> > +    where
>> >> > +        T: AllowAtomicAdd<Rhs>,
>> >> > +    {
>> >> > +        let v = T::rhs_into_delta(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_add()` because of the safety requirement of
>> >> > +        // `AllowAtomicAdd`.
>> >> 
>> >> This part should be moved to the CAST comment above, since we're not
>> >> only writing a value transmuted from `T` into `*self`.
>> >> 
>> >
>> > Hmm.. the CAST comment should explain why a pointer of `T` can be a
>> > valid pointer of `T::Repr` because the atomic_add() below is going to
>> > read through the pointer and write value back. The comment starting with
>> > "`*self`" explains the value written is a valid `T`, therefore
>> > conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
>> > into `a`.
>> 
>> I see, my interpretation was that if we put it on the cast, then the
>> operation that `atomic_add` does also is valid.
>> 
>> But I think this comment should either be part of the `CAST` or the
>> `SAFETY` comment. Going by your interpretation, it would make more sense
>> in the SAFETY one, since there you justify that you're actually writing
>> a value of type `T`.
>> 
>
> Hmm.. you're probably right. There are two safety things about
> atomic_add():
>
> - Whether calling it is safe
> - Whether the operation on `a` (a pointer to `T` essentially) is safe.

Well part of calling `T::Repr::atomic_add` is that the pointer is valid.
But it actually isn't valid for all operations, only for the specific
one you have here. If we want to be 100% correct, we actually need to
change the safety comment of `atomic_add` to say that it only requires
the result of `*a + v` to be writable... But that is most likely very
annoying... (note that we also have this issue for `store`)

I'm not too sure on what the right way to do this is. The formal answer
is to "just do it right", but then safety comments really just devolve
into formally proving the correctness of the program. I think -- for now
at least :) -- that we shouldn't do this here & now (since we also have
a lot of other code that isn't using normal good safety comments, let
alone formally correct ones).

> How about the following:
>
>         let v = T::rhs_into_delta(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_add()` because of the safety requirement of
>         // `AllowAtomicAdd`.
>         //
>         // SAFETY:
>         // - For calling `atomic_add()`:
>         //   - `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.
>         // - For accessing `*a`: the value written is transmutable to `T`
>         //   due to the safety requirement of `AllowAtomicAdd`.
>         unsafe { T::Repr::atomic_add(a, v) };

That looks fine for now. But isn't this duplicating the sentence
starting with `*self`?

---
Cheers,
Benno

On Tue, Jul 15, 2025 at 08:39:04PM +0200, Benno Lossin wrote:
[...]
> >> > Hmm.. the CAST comment should explain why a pointer of `T` can be a
> >> > valid pointer of `T::Repr` because the atomic_add() below is going to
> >> > read through the pointer and write value back. The comment starting with
> >> > "`*self`" explains the value written is a valid `T`, therefore
> >> > conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
> >> > into `a`.
> >> 
> >> I see, my interpretation was that if we put it on the cast, then the
> >> operation that `atomic_add` does also is valid.
> >> 
> >> But I think this comment should either be part of the `CAST` or the
> >> `SAFETY` comment. Going by your interpretation, it would make more sense
> >> in the SAFETY one, since there you justify that you're actually writing
> >> a value of type `T`.
> >> 
> >
> > Hmm.. you're probably right. There are two safety things about
> > atomic_add():
> >
> > - Whether calling it is safe
> > - Whether the operation on `a` (a pointer to `T` essentially) is safe.
> 
> Well part of calling `T::Repr::atomic_add` is that the pointer is valid.

Here by saying "calling `T::Repr::atomic_add`", I think you mean the
whole operation, so yeah, we have to consider the validy for `T` of the
result. But what I'm trying to do is reasoning this in 2 steps:

First, let's treat it as an `atomic_add(*mut i32, i32)`, then as long as
we provide a valid `*mut i32`, it's safe to call. 

And second assume we call it with a valid pointer to `T::Repr`, and a
delta from `rhs_into_delta()`, then per the safety guarantee of
`AllowAtomicAdd`, the value written at the pointer is a valid `T`.

Based on these, we can prove the whole operation is safe for the given
input.

> But it actually isn't valid for all operations, only for the specific
> one you have here. If we want to be 100% correct, we actually need to
> change the safety comment of `atomic_add` to say that it only requires
> the result of `*a + v` to be writable... But that is most likely very
> annoying... (note that we also have this issue for `store`)
> 
> I'm not too sure on what the right way to do this is. The formal answer
> is to "just do it right", but then safety comments really just devolve
> into formally proving the correctness of the program. I think -- for now
> at least :) -- that we shouldn't do this here & now (since we also have
> a lot of other code that isn't using normal good safety comments, let
> alone formally correct ones).
> 
> > How about the following:
> >
> >         let v = T::rhs_into_delta(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_add()` because of the safety requirement of
> >         // `AllowAtomicAdd`.
> >         //
> >         // SAFETY:
> >         // - For calling `atomic_add()`:
> >         //   - `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.
> >         // - For accessing `*a`: the value written is transmutable to `T`
> >         //   due to the safety requirement of `AllowAtomicAdd`.
> >         unsafe { T::Repr::atomic_add(a, v) };
> 
> That looks fine for now. But isn't this duplicating the sentence
> starting with `*self`?

Oh sorry, I meant to remove the sentence starting with `*self`. :(

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

On Tue Jul 15, 2025 at 10:13 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 08:39:04PM +0200, Benno Lossin wrote:
> [...]
>> >> > Hmm.. the CAST comment should explain why a pointer of `T` can be a
>> >> > valid pointer of `T::Repr` because the atomic_add() below is going to
>> >> > read through the pointer and write value back. The comment starting with
>> >> > "`*self`" explains the value written is a valid `T`, therefore
>> >> > conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
>> >> > into `a`.
>> >> 
>> >> I see, my interpretation was that if we put it on the cast, then the
>> >> operation that `atomic_add` does also is valid.
>> >> 
>> >> But I think this comment should either be part of the `CAST` or the
>> >> `SAFETY` comment. Going by your interpretation, it would make more sense
>> >> in the SAFETY one, since there you justify that you're actually writing
>> >> a value of type `T`.
>> >> 
>> >
>> > Hmm.. you're probably right. There are two safety things about
>> > atomic_add():
>> >
>> > - Whether calling it is safe
>> > - Whether the operation on `a` (a pointer to `T` essentially) is safe.
>> 
>> Well part of calling `T::Repr::atomic_add` is that the pointer is valid.
>
> Here by saying "calling `T::Repr::atomic_add`", I think you mean the
> whole operation, so yeah, we have to consider the validy for `T` of the
> result.

I meant just the call to `atomic_add`.

> But what I'm trying to do is reasoning this in 2 steps:
>
> First, let's treat it as an `atomic_add(*mut i32, i32)`, then as long as
> we provide a valid `*mut i32`, it's safe to call. 

But the thing is, we're not supplying a valid `*mut i32`. Because the
pointer points to a value that is not actually an `i32`. You're only
allowed to write certain values and so you basically have to treat it as
a transmute + write. And so you need to include a justification for this
transmute in the write itself. 

For example, if we had `bool: AllowAtomic`, then writing a `2` in store
would be insta-UB, since we then have a `&UnsafeCell<bool>` pointing at
`2`.

This is part of library vs language UB, writing `2` into a bool and
having a reference is language-UB (ie instant UB) and writing a `2` into
a variable of type `i32` that is somewhere cast to `bool` is library-UB
(since it will lead to language-UB later). 

The safety comments become simpler when you use `UnsafeCell<T::Repr>`
instead :) since that changes this language-UB into library-UB. (the
only safety comment that is more complex then is `get_mut`, but that's
only a single one)

If you don't want that, then we can solve this in two ways:

(1) add a guarantee on `atomic_add` (and all other operations) that it
    will write `*a + v` to `a` and nothing else.
(2) make the safety requirement only require writes of the addition to
    be valid.

My preference precedence is: use `T::Repr`, (2) and finally (1). (2)
will be very wordy on all operations & the safety comments in this file,
but it's clean from a formal perspective. (1) works by saying "what
we're supplying is actually not a valid `*mut i32`, but since the
guarantee of the function ensures that only specific things are written,
it's fine" which isn't very clean. And the `T::Repr` approach avoids all
this by just storing value of `T::Repr` circumventing the whole issue.
Then we only need to justify why we can point a `&mut T` at it and that
we can do by having an invariant that should be simple to keep.

We probably should talk about this in our meeting :)

---
Cheers,
Benno

> And second assume we call it with a valid pointer to `T::Repr`, and a
> delta from `rhs_into_delta()`, then per the safety guarantee of
> `AllowAtomicAdd`, the value written at the pointer is a valid `T`.
>
> Based on these, we can prove the whole operation is safe for the given
> input.
>
>> But it actually isn't valid for all operations, only for the specific
>> one you have here. If we want to be 100% correct, we actually need to
>> change the safety comment of `atomic_add` to say that it only requires
>> the result of `*a + v` to be writable... But that is most likely very
>> annoying... (note that we also have this issue for `store`)
>> 
>> I'm not too sure on what the right way to do this is. The formal answer
>> is to "just do it right", but then safety comments really just devolve
>> into formally proving the correctness of the program. I think -- for now
>> at least :) -- that we shouldn't do this here & now (since we also have
>> a lot of other code that isn't using normal good safety comments, let
>> alone formally correct ones).
>> 
>> > How about the following:
>> >
>> >         let v = T::rhs_into_delta(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_add()` because of the safety requirement of
>> >         // `AllowAtomicAdd`.
>> >         //
>> >         // SAFETY:
>> >         // - For calling `atomic_add()`:
>> >         //   - `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.
>> >         // - For accessing `*a`: the value written is transmutable to `T`
>> >         //   due to the safety requirement of `AllowAtomicAdd`.
>> >         unsafe { T::Repr::atomic_add(a, v) };

On Wed, Jul 16, 2025 at 12:25:30PM +0200, Benno Lossin wrote:
> On Tue Jul 15, 2025 at 10:13 PM CEST, Boqun Feng wrote:
> > On Tue, Jul 15, 2025 at 08:39:04PM +0200, Benno Lossin wrote:
> > [...]
> >> >> > Hmm.. the CAST comment should explain why a pointer of `T` can be a
> >> >> > valid pointer of `T::Repr` because the atomic_add() below is going to
> >> >> > read through the pointer and write value back. The comment starting with
> >> >> > "`*self`" explains the value written is a valid `T`, therefore
> >> >> > conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
> >> >> > into `a`.
> >> >> 
> >> >> I see, my interpretation was that if we put it on the cast, then the
> >> >> operation that `atomic_add` does also is valid.
> >> >> 
> >> >> But I think this comment should either be part of the `CAST` or the
> >> >> `SAFETY` comment. Going by your interpretation, it would make more sense
> >> >> in the SAFETY one, since there you justify that you're actually writing
> >> >> a value of type `T`.
> >> >> 
> >> >
> >> > Hmm.. you're probably right. There are two safety things about
> >> > atomic_add():
> >> >
> >> > - Whether calling it is safe
> >> > - Whether the operation on `a` (a pointer to `T` essentially) is safe.
> >> 
> >> Well part of calling `T::Repr::atomic_add` is that the pointer is valid.
> >
> > Here by saying "calling `T::Repr::atomic_add`", I think you mean the
> > whole operation, so yeah, we have to consider the validy for `T` of the
> > result.
> 
> I meant just the call to `atomic_add`.
> 
> > But what I'm trying to do is reasoning this in 2 steps:
> >
> > First, let's treat it as an `atomic_add(*mut i32, i32)`, then as long as
> > we provide a valid `*mut i32`, it's safe to call. 
> 
> But the thing is, we're not supplying a valid `*mut i32`. Because the
> pointer points to a value that is not actually an `i32`. You're only
> allowed to write certain values and so you basically have to treat it as
> a transmute + write. And so you need to include a justification for this
> transmute in the write itself. 
> 
> For example, if we had `bool: AllowAtomic`, then writing a `2` in store
> would be insta-UB, since we then have a `&UnsafeCell<bool>` pointing at
> `2`.
> 
> This is part of library vs language UB, writing `2` into a bool and
> having a reference is language-UB (ie instant UB) and writing a `2` into
> a variable of type `i32` that is somewhere cast to `bool` is library-UB
> (since it will lead to language-UB later). 
> 

But we are not writing `2` in this case, right? 

A) we have a pointer `*mut i32`, and the memory location is valid for
   writing an `i32`, so we can pass it to a function that may write
   an `i32` to it.

B) and at the same time, we prove that the value written was a valid
   `bool`.

There is no `2` written in the whole process, the proof contains two
parts, that is it. There is no language-UB or library-UB in the whole
process, and you're missing it.

It's like if you want to prove 3 < x < 5, you first prove that x > 3
and then x < 5. It's just that you don't prove it in one go.

> The safety comments become simpler when you use `UnsafeCell<T::Repr>`
> instead :) since that changes this language-UB into library-UB. (the
> only safety comment that is more complex then is `get_mut`, but that's
> only a single one)
> 
> If you don't want that, then we can solve this in two ways:
> 
> (1) add a guarantee on `atomic_add` (and all other operations) that it
>     will write `*a + v` to `a` and nothing else.
> (2) make the safety requirement only require writes of the addition to
>     be valid.
> 
> My preference precedence is: use `T::Repr`, (2) and finally (1). (2)
> will be very wordy on all operations & the safety comments in this file,
> but it's clean from a formal perspective. (1) works by saying "what
> we're supplying is actually not a valid `*mut i32`, but since the
> guarantee of the function ensures that only specific things are written,
> it's fine" which isn't very clean. And the `T::Repr` approach avoids all
> this by just storing value of `T::Repr` circumventing the whole issue.
> Then we only need to justify why we can point a `&mut T` at it and that
> we can do by having an invariant that should be simple to keep.
> 
> We probably should talk about this in our meeting :)
> 

I have a better solution:

in ops.rs

    pub struct AtomicRepr<T: AtomicImpl>(UnsafeCell<T>)

    impl AtomicArithmeticOps for i32 {
        // a *safe* function
        fn atomic_add(a: &AtomicRepr, v: i32) {
	    ...
	}
    }

in generic.rs

    pub struct Atomic<T>(AtoimcRepr<T::Repr>);

    impl<T: AtomicAdd> Atomic<T> {
        fn add(&self, v: .., ...) {
	    T::Repr::atomic_add(&self.0, ...);
	}
    }

see:

	https://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git/log/?h=rust-atomic-impl

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > And second assume we call it with a valid pointer to `T::Repr`, and a
> > delta from `rhs_into_delta()`, then per the safety guarantee of
> > `AllowAtomicAdd`, the value written at the pointer is a valid `T`.
> >
> > Based on these, we can prove the whole operation is safe for the given
> > input.
> >
> >> But it actually isn't valid for all operations, only for the specific
> >> one you have here. If we want to be 100% correct, we actually need to
> >> change the safety comment of `atomic_add` to say that it only requires
> >> the result of `*a + v` to be writable... But that is most likely very
> >> annoying... (note that we also have this issue for `store`)
> >> 
> >> I'm not too sure on what the right way to do this is. The formal answer
> >> is to "just do it right", but then safety comments really just devolve
> >> into formally proving the correctness of the program. I think -- for now
> >> at least :) -- that we shouldn't do this here & now (since we also have
> >> a lot of other code that isn't using normal good safety comments, let
> >> alone formally correct ones).
> >> 
> >> > How about the following:
> >> >
> >> >         let v = T::rhs_into_delta(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_add()` because of the safety requirement of
> >> >         // `AllowAtomicAdd`.
> >> >         //
> >> >         // SAFETY:
> >> >         // - For calling `atomic_add()`:
> >> >         //   - `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.
> >> >         // - For accessing `*a`: the value written is transmutable to `T`
> >> >         //   due to the safety requirement of `AllowAtomicAdd`.
> >> >         unsafe { T::Repr::atomic_add(a, v) };

On Wed Jul 16, 2025 at 4:13 PM CEST, Boqun Feng wrote:
> On Wed, Jul 16, 2025 at 12:25:30PM +0200, Benno Lossin wrote:
>> On Tue Jul 15, 2025 at 10:13 PM CEST, Boqun Feng wrote:
>> > On Tue, Jul 15, 2025 at 08:39:04PM +0200, Benno Lossin wrote:
>> > [...]
>> >> >> > Hmm.. the CAST comment should explain why a pointer of `T` can be a
>> >> >> > valid pointer of `T::Repr` because the atomic_add() below is going to
>> >> >> > read through the pointer and write value back. The comment starting with
>> >> >> > "`*self`" explains the value written is a valid `T`, therefore
>> >> >> > conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
>> >> >> > into `a`.
>> >> >> 
>> >> >> I see, my interpretation was that if we put it on the cast, then the
>> >> >> operation that `atomic_add` does also is valid.
>> >> >> 
>> >> >> But I think this comment should either be part of the `CAST` or the
>> >> >> `SAFETY` comment. Going by your interpretation, it would make more sense
>> >> >> in the SAFETY one, since there you justify that you're actually writing
>> >> >> a value of type `T`.
>> >> >> 
>> >> >
>> >> > Hmm.. you're probably right. There are two safety things about
>> >> > atomic_add():
>> >> >
>> >> > - Whether calling it is safe
>> >> > - Whether the operation on `a` (a pointer to `T` essentially) is safe.
>> >> 
>> >> Well part of calling `T::Repr::atomic_add` is that the pointer is valid.
>> >
>> > Here by saying "calling `T::Repr::atomic_add`", I think you mean the
>> > whole operation, so yeah, we have to consider the validy for `T` of the
>> > result.
>> 
>> I meant just the call to `atomic_add`.
>> 
>> > But what I'm trying to do is reasoning this in 2 steps:
>> >
>> > First, let's treat it as an `atomic_add(*mut i32, i32)`, then as long as
>> > we provide a valid `*mut i32`, it's safe to call. 
>> 
>> But the thing is, we're not supplying a valid `*mut i32`. Because the
>> pointer points to a value that is not actually an `i32`. You're only
>> allowed to write certain values and so you basically have to treat it as
>> a transmute + write. And so you need to include a justification for this
>> transmute in the write itself. 
>> 
>> For example, if we had `bool: AllowAtomic`, then writing a `2` in store
>> would be insta-UB, since we then have a `&UnsafeCell<bool>` pointing at
>> `2`.
>> 
>> This is part of library vs language UB, writing `2` into a bool and
>> having a reference is language-UB (ie instant UB) and writing a `2` into
>> a variable of type `i32` that is somewhere cast to `bool` is library-UB
>> (since it will lead to language-UB later). 
>> 
>
> But we are not writing `2` in this case, right? 
>
> A) we have a pointer `*mut i32`, and the memory location is valid for
>    writing an `i32`, so we can pass it to a function that may write
>    an `i32` to it.
>
> B) and at the same time, we prove that the value written was a valid
>    `bool`.
>
> There is no `2` written in the whole process, the proof contains two
> parts, that is it. There is no language-UB or library-UB in the whole
> process, and you're missing it.

There is no UB here and the public API surface is sound.

The problem is the internal safe <-> unsafe code interaction & the
safety documentation.

> It's like if you want to prove 3 < x < 5, you first prove that x > 3
> and then x < 5. It's just that you don't prove it in one go.

That's true, but not analogous to this case. This is a better analogy:

You have a lemma that proves P given that x == 10. Now you want to use
this lemma, but you are only able to prove that x >= 10. You argue that
this is fine, because the proof of the lemma only uses the fact that
x >= 10.
    But in this situation you're not following the exact rules of the
formal system. To do that you would have to reformulate the lemma to
only ask for x >= 10.

The last part is what relaxing the safety requirements would be
(approach (2) given below).

>> The safety comments become simpler when you use `UnsafeCell<T::Repr>`
>> instead :) since that changes this language-UB into library-UB. (the
>> only safety comment that is more complex then is `get_mut`, but that's
>> only a single one)
>> 
>> If you don't want that, then we can solve this in two ways:
>> 
>> (1) add a guarantee on `atomic_add` (and all other operations) that it
>>     will write `*a + v` to `a` and nothing else.
>> (2) make the safety requirement only require writes of the addition to
>>     be valid.
>> 
>> My preference precedence is: use `T::Repr`, (2) and finally (1). (2)
>> will be very wordy on all operations & the safety comments in this file,
>> but it's clean from a formal perspective. (1) works by saying "what
>> we're supplying is actually not a valid `*mut i32`, but since the
>> guarantee of the function ensures that only specific things are written,
>> it's fine" which isn't very clean. And the `T::Repr` approach avoids all
>> this by just storing value of `T::Repr` circumventing the whole issue.
>> Then we only need to justify why we can point a `&mut T` at it and that
>> we can do by having an invariant that should be simple to keep.
>> 
>> We probably should talk about this in our meeting :)
>> 
>
> I have a better solution:
>
> in ops.rs
>
>     pub struct AtomicRepr<T: AtomicImpl>(UnsafeCell<T>)
>
>     impl AtomicArithmeticOps for i32 {
>         // a *safe* function
>         fn atomic_add(a: &AtomicRepr, v: i32) {
> 	    ...
> 	}
>     }
>
> in generic.rs
>
>     pub struct Atomic<T>(AtoimcRepr<T::Repr>);
>
>     impl<T: AtomicAdd> Atomic<T> {
>         fn add(&self, v: .., ...) {
> 	    T::Repr::atomic_add(&self.0, ...);
> 	}
>     }
>
> see:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git/log/?h=rust-atomic-impl

Hmm what does the additional indirection give you?

Otherwise this looks like the `T::Repr` approach that I detailed above,
so I like it :)

---
Cheers,
Benno

On Wed, Jul 16, 2025 at 05:36:05PM +0200, Benno Lossin wrote:
[..]
> >
> > I have a better solution:
> >
> > in ops.rs
> >
> >     pub struct AtomicRepr<T: AtomicImpl>(UnsafeCell<T>)
> >
> >     impl AtomicArithmeticOps for i32 {
> >         // a *safe* function
> >         fn atomic_add(a: &AtomicRepr, v: i32) {
> > 	    ...
> > 	}
> >     }
> >
> > in generic.rs
> >
> >     pub struct Atomic<T>(AtoimcRepr<T::Repr>);
> >
> >     impl<T: AtomicAdd> Atomic<T> {
> >         fn add(&self, v: .., ...) {
> > 	    T::Repr::atomic_add(&self.0, ...);
> > 	}
> >     }
> >
> > see:
> >
> > 	https://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git/log/?h=rust-atomic-impl
> 
> Hmm what does the additional indirection give you?
> 

What additional indirection you mean? You cannot make atomic_add() safe
with only `UnsafeCell<T::Repr>`.

Regards,
Boqun

> Otherwise this looks like the `T::Repr` approach that I detailed above,
> so I like it :)
> 
> ---
> Cheers,
> Benno

On Wed Jul 16, 2025 at 5:48 PM CEST, Boqun Feng wrote:
> On Wed, Jul 16, 2025 at 05:36:05PM +0200, Benno Lossin wrote:
> [..]
>> >
>> > I have a better solution:
>> >
>> > in ops.rs
>> >
>> >     pub struct AtomicRepr<T: AtomicImpl>(UnsafeCell<T>)
>> >
>> >     impl AtomicArithmeticOps for i32 {
>> >         // a *safe* function
>> >         fn atomic_add(a: &AtomicRepr, v: i32) {
>> > 	    ...
>> > 	}
>> >     }
>> >
>> > in generic.rs
>> >
>> >     pub struct Atomic<T>(AtoimcRepr<T::Repr>);
>> >
>> >     impl<T: AtomicAdd> Atomic<T> {
>> >         fn add(&self, v: .., ...) {
>> > 	    T::Repr::atomic_add(&self.0, ...);
>> > 	}
>> >     }
>> >
>> > see:
>> >
>> > 	https://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git/log/?h=rust-atomic-impl
>> 
>> Hmm what does the additional indirection give you?
>> 
>
> What additional indirection you mean? You cannot make atomic_add() safe
> with only `UnsafeCell<T::Repr>`.

What is the advantage of making it safe? It just moves the safety
comments into `ops.rs` which makes it harder to read due to the macros.

---
Cheers,
Benno

On Wed, Jul 16, 2025 at 07:16:02PM +0200, Benno Lossin wrote:
> On Wed Jul 16, 2025 at 5:48 PM CEST, Boqun Feng wrote:
> > On Wed, Jul 16, 2025 at 05:36:05PM +0200, Benno Lossin wrote:
> > [..]
> >> >
> >> > I have a better solution:
> >> >
> >> > in ops.rs
> >> >
> >> >     pub struct AtomicRepr<T: AtomicImpl>(UnsafeCell<T>)
> >> >
> >> >     impl AtomicArithmeticOps for i32 {
> >> >         // a *safe* function
> >> >         fn atomic_add(a: &AtomicRepr, v: i32) {
> >> > 	    ...
> >> > 	}
> >> >     }
> >> >
> >> > in generic.rs
> >> >
> >> >     pub struct Atomic<T>(AtoimcRepr<T::Repr>);
> >> >
> >> >     impl<T: AtomicAdd> Atomic<T> {
> >> >         fn add(&self, v: .., ...) {
> >> > 	    T::Repr::atomic_add(&self.0, ...);
> >> > 	}
> >> >     }
> >> >
> >> > see:
> >> >
> >> > 	https://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git/log/?h=rust-atomic-impl
> >> 
> >> Hmm what does the additional indirection give you?
> >> 
> >
> > What additional indirection you mean? You cannot make atomic_add() safe
> > with only `UnsafeCell<T::Repr>`.
> 
> What is the advantage of making it safe? It just moves the safety

Well, first we in general are in favor of safe functions when we can
make it safe, right? Second, at Atomic<T> level, the major unsafe stuff
comes from the T <-> T::Repr transmutable and making `Atomic<T>` a valid
`T`, the safety of i{32, 64}::atomic_add() would be a bit distraction
when implementing Atomic::add(). With i{32, 64}::atomic_add() being
safe, I can implementation Atomic::add() as:

impl<T: ..> Atomic<T> {
    #[inline(always)]
    pub fn add<Rhs>(&self, v: Rhs, _: ordering::Relaxed)
    where
        T: AtomicAdd<Rhs>,
    {
        let v = T::rhs_into_delta(v);

        // INVARIANT: `self.0` is a valid `T` due to safety requirement of `AtomicAdd`.
        T::Repr::atomic_add(&self.0, v);
    }
}

then all the safety related comments will be focused on why `self.0` is
still a valid `T` after the operation (if you want to be extra detailed
about it, it's fine, and can be done easily)

> comments into `ops.rs` which makes it harder to read due to the macros.

Does it? Add `i32` and `i64` level, you only need the pointer to be a
valid `* mut i{32, 64}`. So the following is pretty clear to me.

    /// Atomic arithmetic operations
    pub trait AtomicArithmeticOps {
        /// Atomic add (wrapping).
        ///
        /// Atomically updates `*a` to `(*a).wrapping_add(v)`.
        fn add[](a: &AtomicRepr<Self>, v: Self::Delta) {
            // SAFETY: `a.as_ptr()` is valid and properly aligned.
            bindings::#call(v, a.as_ptr().cast())
        }
    }

it is at least better than:

            $(
                $(#[$($p_attr)*])*
                $pvis unsafe fn $p_field<E>(
                    self,
                    slot: *mut $p_type,
                    init: impl $crate::PinInit<$p_type, E>,
                ) -> ::core::result::Result<(), E> {
                    // SAFETY: TODO.
                    unsafe { $crate::PinInit::__pinned_init(init, slot) }
                }
            )*

;-)

Regards,
Boqun

> 
> ---
> Cheers,
> Benno