Message-Id: <DB4G6QHBZIQ2.BFT3RFRRHYB0@kernel.org>
Date: Sat, 05 Jul 2025 23:43:18 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
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 5, 2025 at 5:38 PM CEST, Boqun Feng wrote:
> On Sat, Jul 05, 2025 at 10:04:04AM +0200, Benno Lossin wrote:
> [...]
>> >> >
>> >> > 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: ..
>
> Note: provenance preserving is not about the safety of Atomic<*mut T>
> implementation, even if we don't preserve the provenance, calling
> `Atomic<*mut T>` function won't cause UB, it's just that any pointer you
> get from `Atomic<*mut T>` is a pointer without provenance.
>
> So what I meant in this example is all the safey comment is above and 
> the rest is not a safe comment.

Yeah it's not a safety requirement, but a guarantee.

> Hope it's clear.
>
>> > 	    // `atomic_long_xchg()` is implemented as asm(), so it can
>> > 	    // be treated as a normal pointer swap() hence preserve the
>> > 	    // provenance.
>> 
>> Oh I think Gary was talking specifically about Rust's `asm!`. I don't
>> know if C asm is going to play the same way... (inside LLVM they
>> probably are the same thing, but in the abstract machine?)
>> 
>
> You need to understand why Rust abstract machine model `asm!()` in
> that way: Rust abstract machine cannot see through `asm!()`, so it has
> to assume that `asm!() block can do anything that some equivalent Rust
> code does. Further more, this "can do anything that some equivalent Rust
> code does" is only one way to reason, the core part about this is Rust
> will be very conservative when using the `asm!()` result for
> optimization.

Yes that makes sense.

> It should apply to C asm!() as well because LLVM cannot know see through
> the asm block either. And based on the spirit, it might apply to any C
> code as well, because it's outside Rust abstract machine. But if you
> don't agree the reasoning, then we just cannot implement Atomic<*mut T>
> with the existing C API.

We probably should run this by t-opsem on the Rust zulip or ask about
this in the next Meeting with the Rust folks.

>> > 	    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,
>> 
>> YES :)
>> 
>> > but this is probably the best we can get, and it's technically better
>> 
>> I think we can at improve the safety docs situation.
>> 
>
> Once again, it's not about the safety of Atomic<*mut T> implementation.

"Safety docs" to me means all of these:
* `SAFETY` comments & `# Safety` sections,
* `INVARIANT` comments & `# Invariants` sections,
* `GUARANTEE` comments & `# Guarantees` sections.

Maybe there is a better name...

>> > than using Rust native atomics, because that's just UB and no one would
>> > help you.
>> 
>> I'm not arguing using those :)
>> 
>> > (I made a copy-pasta on purpose above, just to make another point why
>> > writing each function out is not worth)
>> 
>> Yeah that's true, but at the moment that safety comment is on the `impl`
>> block? I don't think that's the right place...
>> 
>
> Feel free to send any patch that improves this in your opinion ;-)

I'd prefer we do it right away. But we should just have one big comment
explaining it on the impl and then in the functions refer to it from a
`GUARANTEE` comment?

---
Cheers,
Benno

Powered by blists - more mailing lists