Re: [PATCH 2/3] rust: sync: generic memory barriers

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Gary,

On 4/2/2026 11:24 AM, Gary Guo wrote:
> From: Gary Guo <gary@garyguo.net>
>
> Implement a generic interface for memory barriers (full system/DMA/SMP).
> The interface uses a parameter to force user to specify their intent with
> barriers.
>
> It provides `Read`, `Write`, `Full` orderings which map to the existing
> `rmb()`, `wmb()` and `mb()`, but also `Acquire` and `Release` which is
> documented to have `LOAD->{LOAD,STORE}` ordering and `{LOAD,STORE}->WRITE`
> ordering, although for now they're still mapped to a full `mb()`. But in
> the future it could be mapped to a more efficient form depending on the
> architecture. I included them as many users do not need the STORE->LOAD
> ordering, and having them use `Acquire`/`Release` is more clear on their
> intent in what reordering is to be prevented.
>
> Generic is used here instead of providing individual standalone functions
> to reduce code duplication. For example, the `Acquire` -> `Full` upgrade
> here is uniformly implemented for all three types. The `CONFIG_SMP` check
> in `smp_mb` is uniformly implemented for all SMP barriers. This could
> extend to `virt_mb`'s if they're introduced in the future.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  rust/kernel/sync/atomic/ordering.rs |   2 +-
>  rust/kernel/sync/barrier.rs         | 194 ++++++++++++++++++++++++----

IMO this patch should be split up into different patches for CPU vs IO, and
perhaps even more patches separating out different barrier types.

>  2 files changed, 168 insertions(+), 28 deletions(-)
>
> diff --git a/rust/kernel/sync/atomic/ordering.rs b/rust/kernel/sync/atomic/ordering.rs
> index 3f103aa8db99..c4e732e7212f 100644
> --- a/rust/kernel/sync/atomic/ordering.rs
> +++ b/rust/kernel/sync/atomic/ordering.rs
[...]> +// Currently kernel only support `rmb`, `wmb` and full `mb`.
> +impl MemoryBarrier<Smp> for Read {
> +    #[inline]
> +    fn run() {
> +        // SAFETY: `smp_rmb()` is safe to call.
> +        unsafe { bindings::smp_rmb() };
> +    }
> +}
> +
> +impl MemoryBarrier<Smp> for Write {
> +    #[inline]
> +    fn run() {
>          // SAFETY: `smp_wmb()` is safe to call.
>          unsafe { bindings::smp_wmb() };
> -    } else {
> -        barrier();
>      }
>  }
>
> -/// A read-read memory barrier.
> +impl MemoryBarrier<Smp> for Full {
> +    #[inline]
> +    fn run() {
> +        // SAFETY: `smp_mb()` is safe to call.
> +        unsafe { bindings::smp_mb() };
> +    }
> +}
> +
> +/// Memory barrier.
>  ///
> -/// A barrier that prevents compiler and CPU from reordering memory read accesses across the
> -/// barrier.
> -#[inline(always)]
> -pub fn smp_rmb() {
> +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> +///
> +/// The specific forms of reordering can be specified using the parameter.
> +/// - `mb(Read)` provides a read-read barrier.
> +/// - `mb(Write)` provides a write-write barrier.
> +/// - `mb(Full)` provides a full barrier.
> +/// - `mb(Acquire)` prevents preceding read from being ordered against succeeding memory
> +///    operations.
> +/// - `mb(Release)` prevents preceding memory operations from being ordered against succeeding
> +///    writes.

I don't agree with this definition of Release. Release is always associated with
a specific store, likewise acquire with a load. The definition above also
doesn't make sense 'prevents preceding memory operations from being ordered
against succeeding writes', that's not what Release semantics are. Release
orders memory operations with a specific memory operation associated with
Release. Same for Acquire.

See also in Documentation/memory-barriers.txt, ACQUIRE and RELEASE are defined as being
tied to specific memory operations.

Or am I missing something subtle?

thanks,

-- 
Joel Fernandes