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 add a subtrait of `AllowAtomic` describing
which types have and can do atomic arithemtic operations.

Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
`i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.

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         |  18 +++++
 rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index c5193c1c90fe..26f66cccd4e0 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -29,8 +29,26 @@ unsafe impl generic::AllowAtomic for i32 {
     type Repr = i32;
 }
 
+// SAFETY: `i32` is always sound to transmute back to itself.
+unsafe impl generic::AllowAtomicArithmetic for i32 {
+    type Delta = i32;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d
+    }
+}
+
 // 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: `i64` is always sound to transmute back to itself.
+unsafe impl generic::AllowAtomicArithmetic for i64 {
+    type Delta = i64;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d
+    }
+}
diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
index 1beb802843ee..412a2c811c3d 100644
--- a/rust/kernel/sync/atomic/generic.rs
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -111,6 +111,20 @@ const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
     unsafe { core::mem::transmute_copy(&r) }
 }
 
+/// Atomics that allows arithmetic operations with an integer type.
+///
+/// # Safety
+///
+/// Implementers must guarantee [`Self::Repr`] can always soundly [`transmute()`] to [`Self`] after
+/// arithmetic operations.
+pub unsafe trait AllowAtomicArithmetic: AllowAtomic {
+    /// The delta types for arithmetic operations.
+    type Delta;
+
+    /// Converts [`Self::Delta`] into the representation of the atomic type.
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr;
+}
+
 impl<T: AllowAtomic> Atomic<T> {
     /// Creates a new atomic.
     pub const fn new(v: T) -> Self {
@@ -457,3 +471,97 @@ fn try_cmpxchg<Ordering: Any>(&self, old: &mut T, new: T, _: Ordering) -> bool {
         ret
     }
 }
+
+impl<T: AllowAtomicArithmetic> Atomic<T>
+where
+    T::Repr: AtomicHasArithmeticOps,
+{
+    /// Atomic add.
+    ///
+    /// The addition is a wrapping addition.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// 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<Ordering: RelaxedOnly>(&self, v: T::Delta, _: Ordering) {
+        let v = T::delta_into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_add() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, the following atomic operation won't cause data races.
+        // - For extra safety requirement of usage on pointers returned by `self.as_ptr()`:
+        //   - Atomic operations are used here.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `T: AllowAtomicArithmetic` guarantees the arithmetic operation result is sound to
+        //     stored in an `Atomic<T>`.
+        unsafe {
+            T::Repr::atomic_add(a, v);
+        }
+    }
+
+    /// Atomic fetch and add.
+    ///
+    /// The addition is a wrapping addition.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// 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<Ordering: Any>(&self, v: T::Delta, _: Ordering) -> T {
+        let v = T::delta_into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_fetch_add*() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, the following atomic operation won't cause data races.
+        // - For extra safety requirement of usage on pointers returned by `self.as_ptr()`:
+        //   - Atomic operations are used here.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `T: AllowAtomicArithmetic` guarantees the arithmetic operation result is sound to
+        //     stored in an `Atomic<T>`.
+        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: Per safety requirement of `AllowAtomicArithmetic`, `ret` is a valid bit pattern
+        // of `T`.
+        unsafe { from_repr(ret) }
+    }
+}
-- 
2.39.5 (Apple Git-154)

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> 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 add a subtrait of `AllowAtomic` describing
> which types have and can do atomic arithemtic operations.
>
> Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
> using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
> `i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.
>
> 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         |  18 +++++
>  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)

I think it's better to name this trait `AtomicAdd` and make it generic:

    pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
        fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
    }

`sub` and `fetch_sub` can be added using a similar trait.

The generic allows you to implement it multiple times with different
meanings, for example:

    pub struct Nanos(u64);
    pub struct Micros(u64);
    pub struct Millis(u64);

    impl AllowAtomic for Nanos {
        type Repr = i64;
    }

    impl AtomicAdd<Millis> for Nanos {
        fn rhs_into_repr(rhs: Millis) -> i64 {
            transmute(rhs.0 * 1000_000)
        }
    }

    impl AtomicAdd<Micros> for Nanos {
        fn rhs_into_repr(rhs: Micros) -> i64 {
            transmute(rhs.0 * 1000)
        }
    }

    impl AtomicAdd<Nanos> for Nanos {
        fn rhs_into_repr(rhs: Nanos) -> i64 {
            transmute(rhs.0)
        }
    }

For the safety requirement on the `AtomicAdd` trait, we might just
require bi-directional transmutability... Or can you imagine a case
where that is not guaranteed, but a weaker form is?

---
Cheers,
Benno

On Fri, Jul 11, 2025 at 10:53:45AM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > 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 add a subtrait of `AllowAtomic` describing
> > which types have and can do atomic arithemtic operations.
> >
> > Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
> > using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
> > `i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.
> >
> > 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         |  18 +++++
> >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
> >  2 files changed, 126 insertions(+)
> 
> I think it's better to name this trait `AtomicAdd` and make it generic:
> 
>     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>     }
> 
> `sub` and `fetch_sub` can be added using a similar trait.
> 

Seems a good idea, I will see what I can do. Thanks!

> The generic allows you to implement it multiple times with different
> meanings, for example:
> 
>     pub struct Nanos(u64);
>     pub struct Micros(u64);
>     pub struct Millis(u64);
> 
>     impl AllowAtomic for Nanos {
>         type Repr = i64;
>     }
> 
>     impl AtomicAdd<Millis> for Nanos {
>         fn rhs_into_repr(rhs: Millis) -> i64 {
>             transmute(rhs.0 * 1000_000)

We probably want to use `as` in real code?

>         }
>     }
> 
>     impl AtomicAdd<Micros> for Nanos {
>         fn rhs_into_repr(rhs: Micros) -> i64 {
>             transmute(rhs.0 * 1000)
>         }
>     }
> 
>     impl AtomicAdd<Nanos> for Nanos {
>         fn rhs_into_repr(rhs: Nanos) -> i64 {
>             transmute(rhs.0)
>         }
>     }
> 
> For the safety requirement on the `AtomicAdd` trait, we might just
> require bi-directional transmutability... Or can you imagine a case
> where that is not guaranteed, but a weaker form is?

I have a case that I don't think it's that useful, but it's similar to
the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
`i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
is not always sound. Maybe we could add a "TODO" in the safety section
of `AtomicAdd`, and revisit this later? Like:

/// (in # Safety)
/// TODO: The safety requirement may be tightened to bi-directional
/// transmutability. 

And maybe also add the `Even` example there?

Thoughts?

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

On Fri Jul 11, 2025 at 4:39 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 10:53:45AM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> > 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 add a subtrait of `AllowAtomic` describing
>> > which types have and can do atomic arithemtic operations.
>> >
>> > Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
>> > using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
>> > `i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.
>> >
>> > 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         |  18 +++++
>> >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
>> >  2 files changed, 126 insertions(+)
>> 
>> I think it's better to name this trait `AtomicAdd` and make it generic:
>> 
>>     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>>         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>>     }
>> 
>> `sub` and `fetch_sub` can be added using a similar trait.
>> 
>
> Seems a good idea, I will see what I can do. Thanks!
>
>> The generic allows you to implement it multiple times with different
>> meanings, for example:
>> 
>>     pub struct Nanos(u64);
>>     pub struct Micros(u64);
>>     pub struct Millis(u64);
>> 
>>     impl AllowAtomic for Nanos {
>>         type Repr = i64;

By the way, I find this a bit unfortunate... I think it would be nice to
be able to use `u64` and `u32` as reprs too.

Maybe we can add an additional trait `AtomicRepr` that gets implemented
by all integer types and then we can use that in the `Repr` instead.

This should definitely be a future patch series though.

>>     }
>> 
>>     impl AtomicAdd<Millis> for Nanos {
>>         fn rhs_into_repr(rhs: Millis) -> i64 {
>>             transmute(rhs.0 * 1000_000)
>
> We probably want to use `as` in real code?

I thought that `as` would panic on over/underflow... But it doesn't and
indeed just converts between the two same-sized types.

By the way, should we ask for `Repr` to always be of the same size as
`Self` when implementing `AllowAtomic`?

That might already be implied from the round-trip transmutability:
* `Self` can't have a smaller size, because transmuting `Self` into
  `Repr` would result in uninit bytes.
* `Repr` can't have a smaller size, because then transmuting a `Repr`
  (that was once a `Self`) back into `Self` will result in uninit bytes

We probably should mention this in the docs somewhere?

>>         }
>>     }
>> 
>>     impl AtomicAdd<Micros> for Nanos {
>>         fn rhs_into_repr(rhs: Micros) -> i64 {
>>             transmute(rhs.0 * 1000)
>>         }
>>     }
>> 
>>     impl AtomicAdd<Nanos> for Nanos {
>>         fn rhs_into_repr(rhs: Nanos) -> i64 {
>>             transmute(rhs.0)
>>         }
>>     }
>> 
>> For the safety requirement on the `AtomicAdd` trait, we might just
>> require bi-directional transmutability... Or can you imagine a case
>> where that is not guaranteed, but a weaker form is?
>
> I have a case that I don't think it's that useful, but it's similar to
> the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
> `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
> is not always sound. Maybe we could add a "TODO" in the safety section
> of `AtomicAdd`, and revisit this later? Like:
>
> /// (in # Safety)
> /// TODO: The safety requirement may be tightened to bi-directional
> /// transmutability. 
>
> And maybe also add the `Even` example there?

Ahh that's interesting... I don't think the comment in the tightening
direction makes sense, either we start out with bi-directional
transmutability, or we don't do it at all.

I think an `Even` example is motivation enough to have it. So let's not
tighten it. But I think we should improve the safety requirement:

    /// The valid bit patterns of `Self` must be a superset of the bit patterns reachable through
    /// addition on any values of type [`Self::Repr`] obtained by transmuting values of type `Self`.

or
    
    /// Adding any two values of type [`Self::Repr`] obtained through transmuting values of type `Self`
    /// must yield a value with a bit pattern also valid for `Self`.

I feel like the second one sounds better.

Also is overflowing an atomic variable UB in LKMM? Because if it is,
then `struct MultipleOf<const M: u64>(u64)` is also something that would
be supported. Otherwise only powers of two would be supported.

---
Cheers,
Benno

On Fri, Jul 11, 2025 at 08:55:42PM +0200, Benno Lossin wrote:
[...]
> >> The generic allows you to implement it multiple times with different
> >> meanings, for example:
> >> 
> >>     pub struct Nanos(u64);
> >>     pub struct Micros(u64);
> >>     pub struct Millis(u64);
> >> 
> >>     impl AllowAtomic for Nanos {
> >>         type Repr = i64;
> 
> By the way, I find this a bit unfortunate... I think it would be nice to
> be able to use `u64` and `u32` as reprs too.
> 

I don't think that's necessary, because actually a MaybeUninit<i32> and 
MaybeUninit<i64> would cover all the cases, and even with `u64` and
`u32` being reprs, you still need to trasmute somewhere for non integer
types. But I'm also open to support them, let's discuss this later
separately ;-)

> Maybe we can add an additional trait `AtomicRepr` that gets implemented
> by all integer types and then we can use that in the `Repr` instead.
> 
> This should definitely be a future patch series though.
> 
> >>     }
> >> 
> >>     impl AtomicAdd<Millis> for Nanos {
> >>         fn rhs_into_repr(rhs: Millis) -> i64 {
> >>             transmute(rhs.0 * 1000_000)
> >
> > We probably want to use `as` in real code?
> 
> I thought that `as` would panic on over/underflow... But it doesn't and
> indeed just converts between the two same-sized types.
> 
> By the way, should we ask for `Repr` to always be of the same size as
> `Self` when implementing `AllowAtomic`?
> 
> That might already be implied from the round-trip transmutability:
> * `Self` can't have a smaller size, because transmuting `Self` into
>   `Repr` would result in uninit bytes.
> * `Repr` can't have a smaller size, because then transmuting a `Repr`
>   (that was once a `Self`) back into `Self` will result in uninit bytes
> 
> We probably should mention this in the docs somewhere?
> 

We have it already as the first safety requirement of `AllowAtomic`:

/// # Safety
///
/// - [`Self`] must have the same size and alignment as [`Self::Repr`].

Actually at the beginning, I missed the round-trip transmutablity
(thanks to you and Gary for bring that up), that's only safe requirement
I thought I needed ;-)

> >>         }
> >>     }
> >> 
> >>     impl AtomicAdd<Micros> for Nanos {
> >>         fn rhs_into_repr(rhs: Micros) -> i64 {
> >>             transmute(rhs.0 * 1000)
> >>         }
> >>     }
> >> 
> >>     impl AtomicAdd<Nanos> for Nanos {
> >>         fn rhs_into_repr(rhs: Nanos) -> i64 {
> >>             transmute(rhs.0)
> >>         }
> >>     }
> >> 
> >> For the safety requirement on the `AtomicAdd` trait, we might just
> >> require bi-directional transmutability... Or can you imagine a case
> >> where that is not guaranteed, but a weaker form is?
> >
> > I have a case that I don't think it's that useful, but it's similar to
> > the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
> > `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
> > is not always sound. Maybe we could add a "TODO" in the safety section
> > of `AtomicAdd`, and revisit this later? Like:
> >
> > /// (in # Safety)
> > /// TODO: The safety requirement may be tightened to bi-directional
> > /// transmutability. 
> >
> > And maybe also add the `Even` example there?
> 
> Ahh that's interesting... I don't think the comment in the tightening
> direction makes sense, either we start out with bi-directional
> transmutability, or we don't do it at all.
> 
> I think an `Even` example is motivation enough to have it. So let's not
> tighten it. But I think we should improve the safety requirement:
> 
>     /// The valid bit patterns of `Self` must be a superset of the bit patterns reachable through
>     /// addition on any values of type [`Self::Repr`] obtained by transmuting values of type `Self`.
> 
> or
>     
>     /// Adding any two values of type [`Self::Repr`] obtained through transmuting values of type `Self`
>     /// must yield a value with a bit pattern also valid for `Self`.
> 
> I feel like the second one sounds better.
> 

Me too! Let's use it then. Combining with your `AtomicAdd<Rhs>`
proposal:

    /// # Safety
    ///
    /// Adding any:
    /// - one being the value of [`Self::Repr`] obtained through transmuting value of type `Self`,
    /// - the other being the value of [`Self::Delta`] obtained through conversion of `rhs_into_delta()`,
    /// must yield a value with a bit pattern also valid for `Self`.
    pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
        type Delta = Self::Repr;
        fn rhs_into_delta(rhs: Rhs) -> Delta;
    }

Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
because of when pointer support is added, atomic addition is between
a `*mut ()` and a `isize`, not two `*mut()`.

> Also is overflowing an atomic variable UB in LKMM? Because if it is,

No, all atomic arithmetic operations are wrapping, I did add a comment
in Atomic::add() and Atomic::fetch_add() saying that. This also aligns
with Rust std atomic behaviors.

> then `struct MultipleOf<const M: u64>(u64)` is also something that would
> be supported. Otherwise only powers of two would be supported.

Yeah, seems we can only support PowerOfTwo<integer>.

(but technically you can detect overflow for those value-returning
atomics, but let's think about that later if there is a user)

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

On Fri Jul 11, 2025 at 9:51 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 08:55:42PM +0200, Benno Lossin wrote:
> [...]
>> >> The generic allows you to implement it multiple times with different
>> >> meanings, for example:
>> >> 
>> >>     pub struct Nanos(u64);
>> >>     pub struct Micros(u64);
>> >>     pub struct Millis(u64);
>> >> 
>> >>     impl AllowAtomic for Nanos {
>> >>         type Repr = i64;
>> 
>> By the way, I find this a bit unfortunate... I think it would be nice to
>> be able to use `u64` and `u32` as reprs too.
>> 
>
> I don't think that's necessary, because actually a MaybeUninit<i32> and 
> MaybeUninit<i64> would cover all the cases, and even with `u64` and
> `u32` being reprs, you still need to trasmute somewhere for non integer
> types. But I'm also open to support them, let's discuss this later
> separately ;-)

I think it just looks weird for me to build a type that contains a `u64`
and then not being able to choose that as the repr...

>> Maybe we can add an additional trait `AtomicRepr` that gets implemented
>> by all integer types and then we can use that in the `Repr` instead.
>> 
>> This should definitely be a future patch series though.
>> 
>> >>     }
>> >> 
>> >>     impl AtomicAdd<Millis> for Nanos {
>> >>         fn rhs_into_repr(rhs: Millis) -> i64 {
>> >>             transmute(rhs.0 * 1000_000)
>> >
>> > We probably want to use `as` in real code?
>> 
>> I thought that `as` would panic on over/underflow... But it doesn't and
>> indeed just converts between the two same-sized types.
>> 
>> By the way, should we ask for `Repr` to always be of the same size as
>> `Self` when implementing `AllowAtomic`?
>> 
>> That might already be implied from the round-trip transmutability:
>> * `Self` can't have a smaller size, because transmuting `Self` into
>>   `Repr` would result in uninit bytes.
>> * `Repr` can't have a smaller size, because then transmuting a `Repr`
>>   (that was once a `Self`) back into `Self` will result in uninit bytes
>> 
>> We probably should mention this in the docs somewhere?
>> 
>
> We have it already as the first safety requirement of `AllowAtomic`:
>
> /// # Safety
> ///
> /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
>
> Actually at the beginning, I missed the round-trip transmutablity
> (thanks to you and Gary for bring that up), that's only safe requirement
> I thought I needed ;-)

So technically we only need round-trip transmutablity & same alignment
(as size is implied as shown above), but I think it's much more
understandable if we keep it.

>> >>         }
>> >>     }
>> >> 
>> >>     impl AtomicAdd<Micros> for Nanos {
>> >>         fn rhs_into_repr(rhs: Micros) -> i64 {
>> >>             transmute(rhs.0 * 1000)
>> >>         }
>> >>     }
>> >> 
>> >>     impl AtomicAdd<Nanos> for Nanos {
>> >>         fn rhs_into_repr(rhs: Nanos) -> i64 {
>> >>             transmute(rhs.0)
>> >>         }
>> >>     }
>> >> 
>> >> For the safety requirement on the `AtomicAdd` trait, we might just
>> >> require bi-directional transmutability... Or can you imagine a case
>> >> where that is not guaranteed, but a weaker form is?
>> >
>> > I have a case that I don't think it's that useful, but it's similar to
>> > the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
>> > `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
>> > is not always sound. Maybe we could add a "TODO" in the safety section
>> > of `AtomicAdd`, and revisit this later? Like:
>> >
>> > /// (in # Safety)
>> > /// TODO: The safety requirement may be tightened to bi-directional
>> > /// transmutability. 
>> >
>> > And maybe also add the `Even` example there?
>> 
>> Ahh that's interesting... I don't think the comment in the tightening
>> direction makes sense, either we start out with bi-directional
>> transmutability, or we don't do it at all.
>> 
>> I think an `Even` example is motivation enough to have it. So let's not
>> tighten it. But I think we should improve the safety requirement:
>> 
>>     /// The valid bit patterns of `Self` must be a superset of the bit patterns reachable through
>>     /// addition on any values of type [`Self::Repr`] obtained by transmuting values of type `Self`.
>> 
>> or
>>     
>>     /// Adding any two values of type [`Self::Repr`] obtained through transmuting values of type `Self`
>>     /// must yield a value with a bit pattern also valid for `Self`.
>> 
>> I feel like the second one sounds better.
>> 
>
> Me too! Let's use it then. Combining with your `AtomicAdd<Rhs>`
> proposal:
>
>     /// # Safety
>     ///
>     /// Adding any:
>     /// - one being the value of [`Self::Repr`] obtained through transmuting value of type `Self`,
>     /// - the other being the value of [`Self::Delta`] obtained through conversion of `rhs_into_delta()`,
>     /// must yield a value with a bit pattern also valid for `Self`.

I think this will render wrongly in markdown & we shouldn't use a list,
so how about:

    /// Adding any value of type [`Self::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`.

My only gripe with this is that "Adding" isn't really well-defined...

>     pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
>         type Delta = Self::Repr;
>         fn rhs_into_delta(rhs: Rhs) -> Delta;
>     }
>
> Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
> because of when pointer support is added, atomic addition is between
> a `*mut ()` and a `isize`, not two `*mut()`.

Makes sense, but we don't have default associated types yet :(

>> Also is overflowing an atomic variable UB in LKMM? Because if it is,
>
> No, all atomic arithmetic operations are wrapping, I did add a comment
> in Atomic::add() and Atomic::fetch_add() saying that. This also aligns
> with Rust std atomic behaviors.

Apparently I didn't read your docs very well :)

>> then `struct MultipleOf<const M: u64>(u64)` is also something that would
>> be supported. Otherwise only powers of two would be supported.
>
> Yeah, seems we can only support PowerOfTwo<integer>.
>
> (but technically you can detect overflow for those value-returning
> atomics, but let's think about that later if there is a user)

Yeah, I doubt that a real use-case will pop up soon.

---
Cheers,
Benno

On Fri, Jul 11, 2025 at 11:03:24PM +0200, Benno Lossin wrote:
[...]
> > Actually at the beginning, I missed the round-trip transmutablity
> > (thanks to you and Gary for bring that up), that's only safe requirement
> > I thought I needed ;-)
> 
> So technically we only need round-trip transmutablity & same alignment
> (as size is implied as shown above), but I think it's much more
> understandable if we keep it.
> 

Agreed.

[...]
> >
> > Me too! Let's use it then. Combining with your `AtomicAdd<Rhs>`
> > proposal:
> >
> >     /// # Safety
> >     ///
> >     /// Adding any:
> >     /// - one being the value of [`Self::Repr`] obtained through transmuting value of type `Self`,
> >     /// - the other being the value of [`Self::Delta`] obtained through conversion of `rhs_into_delta()`,
> >     /// must yield a value with a bit pattern also valid for `Self`.
> 
> I think this will render wrongly in markdown & we shouldn't use a list,
> so how about:
> 
>     /// Adding any value of type [`Self::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`.
> 

Looks good to me.

> My only gripe with this is that "Adding" isn't really well-defined...
> 

I think it's good enough, and I created a GitHub tracking a few things
we decide to postpone:

	https://github.com/Rust-for-Linux/linux/issues/1180

> >     pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
> >         type Delta = Self::Repr;
> >         fn rhs_into_delta(rhs: Rhs) -> Delta;
> >     }
> >
> > Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
> > because of when pointer support is added, atomic addition is between
> > a `*mut ()` and a `isize`, not two `*mut()`.
> 
> Makes sense, but we don't have default associated types yet :(
> 

Oops, we are lucky enough to only have a few types to begin with ;-)
Maybe we can use `#[derive(AtomicAdd)] to select the default delta type
later.

Regards,
Boqun

On Fri, Jul 11, 2025 at 02:22:13PM -0700, Boqun Feng wrote:
[...]
> > >     pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
> > >         type Delta = Self::Repr;
> > >         fn rhs_into_delta(rhs: Rhs) -> Delta;
> > >     }
> > >
> > > Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
> > > because of when pointer support is added, atomic addition is between
> > > a `*mut ()` and a `isize`, not two `*mut()`.
> > 
> > Makes sense, but we don't have default associated types yet :(
> > 
> 
> Oops, we are lucky enough to only have a few types to begin with ;-)
> Maybe we can use `#[derive(AtomicAdd)] to select the default delta type
> later.
> 

Turn out I could give `AtomcImpl` an associate type:

    pub trait AtomicImpl: Sealed {
    	type Delta;
    }

and then

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

(Yeah, I named it `AllowAtomicAdd` ;-))

and I only need to provide `Delta` for three types (i32, i64 and *mut
()).

BTW, since atomic arithmetic is wrapping, do we want things like `impl
AllowAtomicAdd<u32> for i32`, similar to the concept of
`i32::wrapping_add_unsigned()` ;-)

Regards,
Boqun

> Regards,
> Boqun

On Fri, Jul 11, 2025 at 07:39:15AM -0700, Boqun Feng wrote:
[...]
> > > ---
> > >  rust/kernel/sync/atomic.rs         |  18 +++++
> > >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
> > >  2 files changed, 126 insertions(+)
> > 
> > I think it's better to name this trait `AtomicAdd` and make it generic:
> > 
> >     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
> >         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
> >     }
> > 
> > `sub` and `fetch_sub` can be added using a similar trait.
> > 
> 
> Seems a good idea, I will see what I can do. Thanks!
> 
> > The generic allows you to implement it multiple times with different
> > meanings, for example:
> > 
> >     pub struct Nanos(u64);
> >     pub struct Micros(u64);
> >     pub struct Millis(u64);
> > 
> >     impl AllowAtomic for Nanos {
> >         type Repr = i64;
> >     }
> > 
> >     impl AtomicAdd<Millis> for Nanos {
> >         fn rhs_into_repr(rhs: Millis) -> i64 {
> >             transmute(rhs.0 * 1000_000)
> 
> We probably want to use `as` in real code?
> 
> >         }
> >     }
> > 
> >     impl AtomicAdd<Micros> for Nanos {
> >         fn rhs_into_repr(rhs: Micros) -> i64 {
> >             transmute(rhs.0 * 1000)
> >         }
> >     }
> > 
> >     impl AtomicAdd<Nanos> for Nanos {
> >         fn rhs_into_repr(rhs: Nanos) -> i64 {
> >             transmute(rhs.0)
> >         }
> >     }
> > 
> > For the safety requirement on the `AtomicAdd` trait, we might just
> > require bi-directional transmutability... Or can you imagine a case
> > where that is not guaranteed, but a weaker form is?
> 
> I have a case that I don't think it's that useful, but it's similar to
> the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a

Oh I mis-read, it's not similar to `Micros` or `Millis`, but still,
`Even<i32>` itself should have the point.

> `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
> is not always sound. Maybe we could add a "TODO" in the safety section
> of `AtomicAdd`, and revisit this later? Like:
> 
> /// (in # Safety)
> /// TODO: The safety requirement may be tightened to bi-directional
> /// transmutability. 
> 
> And maybe also add the `Even` example there?
> 
> Thoughts?
> 

Or since we are going to use fine-grained traits, it's actually easy to
define the safety requirement of `AtomicAdd` (instead of
`AllowAtomicArithmetic) now:

    /// # Safety
    ///
    /// For a `T: AtomicAdd<Rhs>`, the addition result of a valid bit
    /// pattern of `T` and a `T::Repr` convert from `rhs_into_repr()` should
    /// be a valid bit pattern of `T`.
    pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
       fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
    }

Thoughts?

Regards,
Boqun

On Fri Jul 11, 2025 at 7:41 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 07:39:15AM -0700, Boqun Feng wrote:
> [...]
>> > > ---
>> > >  rust/kernel/sync/atomic.rs         |  18 +++++
>> > >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
>> > >  2 files changed, 126 insertions(+)
>> > 
>> > I think it's better to name this trait `AtomicAdd` and make it generic:
>> > 
>> >     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>> >         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>> >     }
>> > 
>> > `sub` and `fetch_sub` can be added using a similar trait.
>> > 
>> 
>> Seems a good idea, I will see what I can do. Thanks!
>> 
>> > The generic allows you to implement it multiple times with different
>> > meanings, for example:
>> > 
>> >     pub struct Nanos(u64);
>> >     pub struct Micros(u64);
>> >     pub struct Millis(u64);
>> > 
>> >     impl AllowAtomic for Nanos {
>> >         type Repr = i64;
>> >     }
>> > 
>> >     impl AtomicAdd<Millis> for Nanos {
>> >         fn rhs_into_repr(rhs: Millis) -> i64 {
>> >             transmute(rhs.0 * 1000_000)
>> 
>> We probably want to use `as` in real code?
>> 
>> >         }
>> >     }
>> > 
>> >     impl AtomicAdd<Micros> for Nanos {
>> >         fn rhs_into_repr(rhs: Micros) -> i64 {
>> >             transmute(rhs.0 * 1000)
>> >         }
>> >     }
>> > 
>> >     impl AtomicAdd<Nanos> for Nanos {
>> >         fn rhs_into_repr(rhs: Nanos) -> i64 {
>> >             transmute(rhs.0)
>> >         }
>> >     }
>> > 
>> > For the safety requirement on the `AtomicAdd` trait, we might just
>> > require bi-directional transmutability... Or can you imagine a case
>> > where that is not guaranteed, but a weaker form is?
>> 
>> I have a case that I don't think it's that useful, but it's similar to
>> the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
>
> Oh I mis-read, it's not similar to `Micros` or `Millis`, but still,
> `Even<i32>` itself should have the point.
>
>> `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
>> is not always sound. Maybe we could add a "TODO" in the safety section
>> of `AtomicAdd`, and revisit this later? Like:
>> 
>> /// (in # Safety)
>> /// TODO: The safety requirement may be tightened to bi-directional
>> /// transmutability. 
>> 
>> And maybe also add the `Even` example there?
>> 
>> Thoughts?
>> 
>
> Or since we are going to use fine-grained traits, it's actually easy to
> define the safety requirement of `AtomicAdd` (instead of
> `AllowAtomicArithmetic) now:
>
>     /// # Safety
>     ///
>     /// For a `T: AtomicAdd<Rhs>`, the addition result of a valid bit
>     /// pattern of `T` and a `T::Repr` convert from `rhs_into_repr()` should
>     /// be a valid bit pattern of `T`.

Let's combine our two safety requirement ideas (I forgot about my Rhs
change).

---
Cheers,
Benno

>     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>        fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>     }
>
> Thoughts?
>
> Regards,
> Boqun