Message-Id: <DAIQG9ALK4QC.2P2C2MC4U9YVX@kernel.org>
Date: Tue, 10 Jun 2025 11:07:16 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <lkmm@...ts.linux.dev>,
 <linux-arch@...r.kernel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Gary Guo" <gary@...yguo.net>,
 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 v4 03/10] rust: sync: atomic: Add ordering annotation
 types

On Tue Jun 10, 2025 at 12:46 AM CEST, Boqun Feng wrote:
> Preparation for atomic primitives. Instead of a suffix like _acquire, a
> method parameter along with the corresponding generic parameter will be
> used to specify the ordering of an atomic operations. For example,
> atomic load() can be defined as:
>
> 	impl<T: ...> Atomic<T> {
> 	    pub fn load<O: AcquireOrRelaxed>(&self, _o: O) -> T { ... }
> 	}
>
> and acquire users would do:
>
> 	let r = x.load(Acquire);
>
> relaxed users:
>
> 	let r = x.load(Relaxed);
>
> doing the following:
>
> 	let r = x.load(Release);
>
> will cause a compiler error.
>
> Compared to suffixes, it's easier to tell what ordering variants an
> operation has, and it also make it easier to unify the implementation of
> all ordering variants in one method via generic. The `IS_RELAXED` and
> `ORDER` associate consts are for generic function to pick up the
> particular implementation specified by an ordering annotation.
>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>

Looks good, I got a few comments on the details below.

> ---
>  rust/kernel/sync/atomic.rs          |  3 +
>  rust/kernel/sync/atomic/ordering.rs | 94 +++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>  create mode 100644 rust/kernel/sync/atomic/ordering.rs
>
> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
> index 65e41dba97b7..9fe5d81fc2a9 100644
> --- a/rust/kernel/sync/atomic.rs
> +++ b/rust/kernel/sync/atomic.rs
> @@ -17,3 +17,6 @@
>  //! [`LKMM`]: srctree/tools/memory-mode/
>  
>  pub mod ops;
> +pub mod ordering;
> +
> +pub use ordering::{Acquire, Full, Relaxed, Release};
> diff --git a/rust/kernel/sync/atomic/ordering.rs b/rust/kernel/sync/atomic/ordering.rs
> new file mode 100644
> index 000000000000..14cda8c5d1b1
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/ordering.rs
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Memory orderings.
> +//!
> +//! The semantics of these orderings follows the [`LKMM`] definitions and rules.
> +//!
> +//! - [`Acquire`] and [`Release`] are similar to their counterpart in Rust memory model.
> +//! - [`Full`] means "fully-ordered", that is:
> +//!   - It provides ordering between all the preceding memory accesses and the annotated operation.
> +//!   - It provides ordering between the annotated operation and all the following memory accesses.
> +//!   - It provides ordering between all the preceding memory accesses and all the fllowing memory
> +//!     accesses.
> +//!   - All the orderings are the same strong as a full memory barrier (i.e. `smp_mb()`).

s/strong/strength/ ?

> +//! - [`Relaxed`] is similar to the counterpart in Rust memory model, except that dependency
> +//!   orderings are also honored in [`LKMM`]. Dependency orderings are described in "DEPENDENCY
> +//!   RELATIONS" in [`LKMM`]'s [`explanation`].
> +//!
> +//! [`LKMM`]: srctree/tools/memory-model/
> +//! [`explanation`]: srctree/tools/memory-model/Documentation/explanation.txt
> +
> +/// The annotation type for relaxed memory ordering.
> +pub struct Relaxed;
> +
> +/// The annotation type for acquire memory ordering.
> +pub struct Acquire;
> +
> +/// The annotation type for release memory ordering.
> +pub struct Release;
> +
> +/// The annotation type for fully-order memory ordering.
> +pub struct Full;

Is this ordering only ever used in combination with itself? (Since you
don't have a `FullOrAcquire` trait)

> +
> +/// The trait bound for operations that only support relaxed ordering.
> +pub trait RelaxedOnly: AcquireOrRelaxed + ReleaseOrRelaxed + All {}
> +
> +impl RelaxedOnly for Relaxed {}
> +
> +/// The trait bound for operations that only support acquire or relaxed ordering.
> +pub trait AcquireOrRelaxed: All {
> +    /// Describes whether an ordering is relaxed or not.
> +    const IS_RELAXED: bool = false;
> +}
> +
> +impl AcquireOrRelaxed for Acquire {}
> +
> +impl AcquireOrRelaxed for Relaxed {
> +    const IS_RELAXED: bool = true;
> +}
> +
> +/// The trait bound for operations that only support release or relaxed ordering.
> +pub trait ReleaseOrRelaxed: All {
> +    /// Describes whether an ordering is relaxed or not.
> +    const IS_RELAXED: bool = false;
> +}
> +
> +impl ReleaseOrRelaxed for Release {}
> +
> +impl ReleaseOrRelaxed for Relaxed {
> +    const IS_RELAXED: bool = true;
> +}
> +
> +/// Describes the exact memory ordering of an `impl` [`All`].
> +pub enum OrderingDesc {

Why not name this `Ordering`?

> +    /// Relaxed ordering.
> +    Relaxed,
> +    /// Acquire ordering.
> +    Acquire,
> +    /// Release ordering.
> +    Release,
> +    /// Fully-ordered.
> +    Full,
> +}
> +
> +/// The trait bound for annotating operations that should support all orderings.
> +pub trait All {
> +    /// Describes the exact memory ordering.
> +    const ORDER: OrderingDesc;

And then here: `ORDERING`.

---
Cheers,
Benno

> +}
> +
> +impl All for Relaxed {
> +    const ORDER: OrderingDesc = OrderingDesc::Relaxed;
> +}
> +
> +impl All for Acquire {
> +    const ORDER: OrderingDesc = OrderingDesc::Acquire;
> +}
> +
> +impl All for Release {
> +    const ORDER: OrderingDesc = OrderingDesc::Release;
> +}
> +
> +impl All for Full {
> +    const ORDER: OrderingDesc = OrderingDesc::Full;
> +}

Powered by blists - more mailing lists