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
© 2016 - 2025 Red Hat, Inc.