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

[Boqun Feng]

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

Regards,
Boqun

> > +    //
> > +    // SAFETY: An empty asm block should be safe.
> 
>     // SAFETY: An empty asm block.
> 
> > +    unsafe {
> > +        core::arch::asm!("");
> > +    }
> 
>     unsafe { core::arch::asm!("") };
> 
> > +}
> > +
> > +/// A full memory barrier.
> > +///
> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> > +pub fn smp_mb() {
> > +    if cfg!(CONFIG_SMP) {
> > +        // SAFETY: `smp_mb()` is safe to call.
> > +        unsafe {
> > +            bindings::smp_mb();
> 
> Does this really work? How does the Rust compiler know this is a memory
> barrier?
> 
> ---
> Cheers,
> Benno
> 
> > +        }
> > +    } else {
> > +        barrier();
> > +    }
> > +}