Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Ralf Jung]

Hi Boqun,

On 15.07.25 17:56, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 05:35:47PM +0200, Ralf Jung wrote:
> > Hi all,
> >
> > On 15.07.25 17:21, Boqun Feng wrote:
> > > On Mon, Jul 14, 2025 at 05:42:39PM +0200, Ralf Jung wrote:
> > > > Hi all,
> > > >
> > > > On 11.07.25 20:20, Boqun Feng wrote:
> > > > > On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> > > > > > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > > > > > > diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..df4015221503
> > > > > > > --- /dev/null
> > > > > > > +++ b/rust/kernel/sync/barrier.rs
> > > > > > > @@ -0,0 +1,65 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +
> > > > > > > +//! Memory barriers.
> > > > > > > +//!
> > > > > > > +//! These primitives have the same semantics as their C counterparts: and the precise definitions
> > > > > > > +//! of semantics can be found at [`LKMM`].
> > > > > > > +//!
> > > > > > > +//! [`LKMM`]: srctree/tools/memory-model/
> > > > > > > +
> > > > > > > +/// A compiler barrier.
> > > > > > > +///
> > > > > > > +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
> > > > > > > +pub(crate) fn barrier() {
> > > > > > > +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
> > > > > > > +    // it suffices as a compiler barrier.
> > > > > >
> > > > > > I don't know about this, but it also isn't my area of expertise... I
> > > > > > think I heard Ralf talk about this at Rust Week, but I don't remember...
> > > > >
> > > > > Easy, let's Cc Ralf ;-)
> > > > >
> > > > > Ralf, I believe the question here is:
> > > > >
> > > > > In kernel C, we define a compiler barrier (barrier()), which is
> > > > > implemented as:
> > > > >
> > > > > # define barrier() __asm__ __volatile__("": : :"memory")
> > > > >
> > > > > Now we want to have a Rust version, and I think an empty `asm!()` should
> > > > > be enough as an equivalent as a barrier() in C, because an empty
> > > > > `asm!()` in Rust implies "memory" as the clobber:
> > > > >
> > > > > 	https://godbolt.org/z/3z3fnWYjs
> > > > >
> > > > > ?
> > > > >
> > > > > I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
> > > > > barrier() and other barriers work for all memory accesses not just
> > > > > atomics, so the problem "So, if your program contains no atomic
> > > > > accesses, but some atomic fences, those fences do nothing." doesn't
> > > > > exist for us. And our barrier() is strictly weaker than other barriers.
> > > > >
> > > > > And based on my understanding of the consensus on Rust vs LKMM, "do
> > > > > whatever kernel C does and rely on whatever kernel C relies" is the
> > > > > general suggestion, so I think an empty `asm!()` works here. Of course
> > > > > if in practice, we find an issue, I'm happy to look for solutions ;-)
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347
> > > >
> > > > If I understood correctly, this is about using "compiler barriers" to order
> > > > volatile accesses that the LKMM uses in lieu of atomic accesses?
> > > > I can't give a principled answer here, unfortunately -- as you know, the
> > > > mapping of LKMM through the compiler isn't really in a state where we can
> > > > make principled formal statements. And making principled formal statements
> > > > is my main expertise so I am a bit out of my depth here. ;)
> > >
> > > Understood ;-)
> > >
> > > > So I agree with your 2nd paragraph: I would say just like the fact that you
> > > > are using volatile accesses in the first place, this falls under "do
> > > > whatever the C code does, it shouldn't be any more broken in Rust than it is
> > > > in C".
> > > >
> > > > However, saying that it in general "prevents reordering all memory accesses"
> > > > is unlikely to be fully correct -- if the compiler can prove that the inline
> > > > asm block could not possibly have access to a local variable (e.g. because
> > > > it never had its address taken), its accesses can still be reordered. This
> > > > applies both to C compilers and Rust compilers. Extra annotations such as
> > > > `noalias` (or `restrict` in C) can also give rise to reorderings around
> > > > arbitrary code, including such barriers. This is not a problem for
> > > > concurrent code since it would anyway be wrong to claim that some pointer
> > > > doesn't have aliases when it is accessed by multiple threads, but it shows
> > >
> > > Right, it shouldn't be a problem for most of the concurrent code, and
> > > thank you for bringing this up. I believe we can rely on the barrier
> > > behavior if the memory accesses on both sides are done via aliased
> > > references/pointers, which should be the same as C code relies on.
> > >
> > > One thing though is we don't use much of `restrict` in kernel C, so I
> > > wonder the compiler's behavior in the following code:
> > >
> > >       let mut x = KBox::new_uninit(GFP_KERNEL)?;
> > >       // ^ KBox is our own Box implementation based on kmalloc(), and it
> > >       // accepts a flag in new*() functions for different allocation
> > >       // behavior (can sleep or not, etc), of course we want it to behave
> > >       // like an std Box in term of aliasing.
> > >
> > >       let x = KBox::write(x, foo); // A
> > >
> > >       smp_mb():
> > >         // using Rust asm!() for explanation, it's really implemented in
> > >         // C.
> > >         asm!("mfence");
> > >
> > >       let a: &Atomic<*mut Foo> = ...; // `a` was null initially.
> > >
> > >       a.store(KBox::into_raw(x), Relaxed); // B
> > >
> > > Now we obviously want A and B to be ordered, because smp_mb() is
> > > supposed to be stronger than Release ordering. So if another thread does
> > > an Acquire read or uses address dependency:
> > >
> > >       let a: &Atomic<*mut Foo> = ...;
> > >       let foo_ptr = a.load(Acquire); // or load(Relaxed);
> > >
> > >       if !foo_ptr.is_null() {
> > >           let y: KBox<Foo> = unsafe { KBox::from_raw(foo_ptr) };
> > > 	// ^ this should be safe.
> > >       }
> > >
> > > Is it something Rust AM could guarantee?
> >
> > If we pretend these are normal Rust atomics, and we look at the acquire
> > read, then yeah that should work -- the asm block can act like a release
> > fence. With the LKMM, it's not a "guarantee" in the same sense any more
> > since it lacks the formal foundations, but "it shouldn't be worse than in
> > C".
>
> > The Rust/C/C++ memory models do not allow that last example with a relaxed
> > load and an address dependency. In C/C++ this requires "consume", which Rust
>
> Sorry I wasn't clear, of course I wasn't going to start a discussion
> about address dependency and formal guarantee about it ;-)
>
> What I meant was the "prevent reordering A and B because of the asm!()"
> at the release side, because normally we won't use a restrict pointer to
> a kmalloc() result, so I'm curious whether Box make the behavior
> different:
>
>      let mut b = Box::new_uninit(...);
>      let b = Box::write(b, ...); // <- this is a write done via noalias
>      asm!(...);
>      a.store(Box::from_raw(b), Relaxed);
>
> But looks like we can just model the asm() as a Rust release fence, so
> it should work. Thanks!

Yeah... this is actually a subtle case and there are some adjacent compiler bugs 
(when doing the same with local variables, not Box), but those are bugs (and 
they affect both C and Rust).

Kind regards,
Ralf