Message-ID: <aHFrUa3VWaKTe0xr@tardis-2.local>
Date: Fri, 11 Jul 2025 12:51:45 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	lkmm@...ts.linux.dev, linux-arch@...r.kernel.org,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, Will Deacon <will@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Mark Rutland <mark.rutland@....com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Lyude Paul <lyude@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Mitchell Levy <levymitchell0@...il.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of
 arithmetic operations

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

Powered by blists - more mailing lists