Message-ID: <aGhiEZ4uNzEs4nah@Mac.home>
Date: Fri, 4 Jul 2025 16:21:53 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Gary Guo <gary@...yguo.net>, 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>,
	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>
Subject: Re: [PATCH v5 04/10] rust: sync: atomic: Add generic atomics

On Sat, Jul 05, 2025 at 12:38:05AM +0200, Benno Lossin wrote:
[..]
> >> >   (This is not a safety requirement)
> >> >
> >> >   from_repr() and into_repr(), if exist, should behave like transmute()
> >> >   on the bit pattern of the results, in other words, bit patterns of `T`
> >> >   or `T::Repr` should stay the same before and after these operations.
> >> >
> >> >   Of course if we remove them and replace with transmute(), same result.
> >> >
> >> >   This reflects the fact that customized atomic types should store
> >> >   unmodified bit patterns into atomic variables, and this make atomic
> >> >   operations don't have weird behavior [1] when combined with new(),
> >> >   from_ptr() and get_mut().
> >> 
> >> I remember that this was required to support types like `(u8, u16)`? If
> >
> > My bad, I forgot to put the link to [1]...
> >
> > [1]: https://lore.kernel.org/rust-for-linux/20250621123212.66fb016b.gary@garyguo.net/
> >
> > Basically, without requiring from_repr() and into_repr() to act as a
> > transmute(), you can have weird types in Atomic<T>.
> 
> Ah right, I forgot some context... Is this really a problem? I mean it's

It's not a problem for safety, so it's not a safety requirement. But I
really don't see a reason why we want to support this. Not supporting
this makes the atomic implementation reasoning easier.

> weird sure, but if someone needs this, then it's fine?
> 

They can always play the !value game outside atomic, i.e. !value before
store and !value after load, so I don't think it's reasonable request.

> > `(u8, u16)` (in case it's not clear to other audience, it's tuple with a
> > `u8` and a `u16` in it, so there is a 8-bit hole) is not going to
> > support until we have something like a `Atomic<MaybeUninit<i32>>`.
> 
> Ahh right we also had this issue, could you also include that in your
> writeup? :)
> 

Sure, I will put it in a limitation section maybe.

> >> yes, then it would be good to include a paragraph like the one above for
> >> enums :)
> >> 
> >> > * Provenance preservation.
> >> >
> >> >   (This is not a safety requirement for Atomic itself)
> >> >
> >> >   For a `Atomic<*mut T>`, it should preserve the provenance of the
> >> >   pointer that has been stored into it, i.e. the load result from a
> >> >   `Atomic<*mut T>` should have the same provenance.
> >> >
> >> >   Technically, without this, `Atomic<*mut T>` still work without any
> >> >   safety issue itself, but the user of it must maintain the provenance
> >> >   themselves before store or after load.
> >> >
> >> >   And it turns out it's not very hard to prove the current
> >> >   implementation achieve this:
> >> >
> >> >   - For a non-atomic operation done on the atomic variable, they are
> >> >     already using pointer operation, so the provenance has been
> >> >     preserved.
> >> >   - For an atomic operation, since they are done via inline asm code, in
> >> >     Rust's abstract machine, they can be treated as pointer read and
> >> >     write:
> >> >
> >> >     a) A load of the atomic can be treated as a pointer read and then
> >> >        exposing the provenance.
> >> >     b) A store of the atomic can be treated as a pointer write with a
> >> >        value created with the exposed provenance.
> >> >
> >> >     And our implementation, thanks to no arbitrary type coercion,
> >> >     already guarantee that for each a) there is a from_repr() after and
> >> >     for each b) there is a into_repr() before. And from_repr() acts as
> >> >     a with_exposed_provenance() and into_repr() acts as a
> >> >     expose_provenance(). Hence the provenance is preserved.
> >> 
> >> I'm not sure this point is correct, but I'm an atomics noob, so maybe
> >> Gary should take a look at this :)
> >> 
> >
> > Basically, what I'm trying to prove is that we can have a provenance-
> > preserved Atomic<*mut T> implementation based on the C atomics. Either
> > that is true, or we should write our own atomic pointer implementation.
> 
> That much I remembered :) But since you were going into the specifics
> above, I think we should try to be correct. But maybe natural language
> is the wrong medium for that, just write the rust code and we'll see...
> 

I don't thinking writing rust code can help us here other than duplicate
my reasoning above, so like:

    ipml *mut() {
        pub fn xchg(ptr: *mut *mut (), new: *mut ()) -> *mut () {
	    // SAFTEY: ..
	    // `atomic_long_xchg()` is implemented as asm(), so it can
	    // be treated as a normal pointer swap() hence preserve the
	    // provenance.
	    unsafe { atomic_long_xchg(ptr.cast::<atomic_long_t>(), new as ffi:c_long) }
	}

        pub fn cmpxchg(ptr: *mut *mut (), old: *mut (), new: *mut ()) -> *mut () {
	    // SAFTEY: ..
	    // `atomic_long_xchg()` is implemented as asm(), so it can
	    // be treated as a normal pointer compare_exchange() hence preserve the
	    // provenance.
	    unsafe { atomic_long_cmpxchg(ptr.cast::<atomic_long_t>(), old as ffi::c_long, new as ffi:c_long) }
	}

	<do it for a lot of functions>
    }

So I don't think that approach is worth doing. Again the provenance
preserving is a global property, either we have it as whole or we don't
have it, and adding precise comment of each function call won't change
the result. I don't see much difference between reasoning about a set of
functions vs. reasoning one function by one function with the same
reasoning.

If we have a reason to believe that C atomic doesn't support this we
just need to move to our own implementation. I know you (and probably
Gary) may feel the reasoning about provenance preserving a bit handwavy,
but this is probably the best we can get, and it's technically better
than using Rust native atomics, because that's just UB and no one would
help you.

(I made a copy-pasta on purpose above, just to make another point why
writing each function out is not worth)

Regards,
Boqun


> >> >   Note this is a global property and it has to proven at `Atomic<T>`
> >> >   level.
> >> 
> >> Thanks for he awesome writeup, do you want to put this in some comment
> >> or at least the commit log?
> >> 
> >
> > Yes, so the round-trip transmutability will be in the safe requirement
> > of `AllowAtomic`. And if we still keep `from_repr()` and `into_repr()`
> > (we can give them default implementation using trasnmute()), I will put
> > the "bit-equivalency of from_repr() and into_repr()" in the requirement
> > of `AllowAtomic` as well.
> >
> > For the "Provenance preservation", I will put it before `impl
> > AllowAtomic for *mut T`. (Remember we recently discover that doc comment
> > works for impl block as well? [2])
> 
> Yeah that sounds good!
> 
> ---
> Cheers,
> Benno
> 
> > [2]: https://lore.kernel.org/rust-for-linux/aD4NW2vDc9rKBDPy@tardis.local/

Powered by blists - more mailing lists