Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

Boqun Feng
2025-07-11T19:51:50Z

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