Re: [PATCH v5 10/10] rust: sync: Add memory barriers

[Andreas Hindborg <a.hindborg@kernel.org>]

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Thu, Jun 26, 2025 at 03:36:25PM +0200, Andreas Hindborg wrote:
>> "Boqun Feng" <boqun.feng@gmail.com> writes:
> [...]
>> > +//! [`LKMM`]: srctree/tools/memory-mode/
>>
>> Typo in link target.
>>
>> > +
>> > +/// A compiler barrier.
>> > +///
>> > +/// An explicic compiler barrier function that prevents the compiler from moving the memory
>> > +/// accesses either side of it to the other side.
>>
>> Typo in "explicit".
>>
>
> Fixed.
>
>> How about:
>>
>>   A compiler barrier. Prevents the compiler from reordering
>>   memory access instructions 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.
>> > +    //
>> > +    // SAFETY: An empty asm block should be safe.
>> > +    unsafe {
>> > +        core::arch::asm!("");
>> > +    }
>> > +}
>> > +
>> > +/// A full memory barrier.
>> > +///
>> > +/// A barrier function that prevents both the compiler and the CPU from moving the memory accesses
>> > +/// either side of it to the other side.
>>
>>
>>   A barrier that prevents compiler and CPU from reordering memory access
>>   instructions across the barrier.
>>
>> > +pub fn smp_mb() {
>> > +    if cfg!(CONFIG_SMP) {
>> > +        // SAFETY: `smp_mb()` is safe to call.
>> > +        unsafe {
>> > +            bindings::smp_mb();
>> > +        }
>> > +    } else {
>> > +        barrier();
>> > +    }
>> > +}
>> > +
>> > +/// A write-write memory barrier.
>> > +///
>> > +/// A barrier function that prevents both the compiler and the CPU from moving the memory write
>> > +/// accesses either side of it to the other side.
>>
>>   A barrier that prevents compiler and CPU from reordering memory write
>>   instructions across the barrier.
>>
>> > +pub fn smp_wmb() {
>> > +    if cfg!(CONFIG_SMP) {
>> > +        // SAFETY: `smp_wmb()` is safe to call.
>> > +        unsafe {
>> > +            bindings::smp_wmb();
>> > +        }
>> > +    } else {
>> > +        barrier();
>> > +    }
>> > +}
>> > +
>> > +/// A read-read memory barrier.
>> > +///
>> > +/// A barrier function that prevents both the compiler and the CPU from moving the memory read
>> > +/// accesses either side of it to the other side.
>>
>>   A barrier that prevents compiler and CPU from reordering memory read
>>   instructions across the barrier.
>>
>
> These are good wording, except that I will use "memory (read/write)
> accesses" instead of "memory (read/write) instructions" because:
>
> 1) "instructions" are at lower level than the language, and memory
>    barriers function are provided as synchonization primitives, so I
>    feel we should describe memory barrier effects at language level,
>    i.e. mention how it would interact with objects and accesses to them.
>
> 2) There are instructions can do read and write in one instruction, it
>    might be unclear when we say "prevents reordering an instruction"
>    whether both parts are included, for example:
>
>    r1 = atomic_add(x, 1); // <- this can be one instruction.
>    smp_rmb();
>    r2 = atomic_read(y);
>
>    people may think because the smp_rmb() prevents read instructions
>    reordering, and atomic_add() is one instruction in this case,
>    smp_rmb() can prevents the write part of that instruction from
>    reordering, but that's not the case.
>
>
> So I will do:
>
>    A barrier that prevents compiler and CPU from reordering memory read
>    accesses across the barrier.

Right, that makes sense 👍


Best regards,
Andreas Hindborg