Re: [PATCH v5 03/10] rust: sync: atomic: Add ordering annotation types

Gary Guo
2025-06-21T11:18:53Z

On Wed, 18 Jun 2025 09:49:27 -0700
Boqun Feng <boqun.feng@gmail.com> 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.

I quite like the design. Minor comments inline below.

>
> 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
> `TYPE` associate consts are for generic function to pick up the
> particular implementation specified by an ordering annotation.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> rust/kernel/sync/atomic.rs | 3 +
> rust/kernel/sync/atomic/ordering.rs | 106 ++++++++++++++++++++++++++++
> 2 files changed, 109 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..96757574ed7d
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/ordering.rs
> @@ -0,0 +1,106 @@
> +// 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()`).
> +//! - [`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;
> +
> +/// Describes the exact memory ordering.
> +pub enum OrderingType {
> + /// Relaxed ordering.
> + Relaxed,
> + /// Acquire ordering.
> + Acquire,
> + /// Release ordering.
> + Release,
> + /// Fully-ordered.
> + Full,
> +}

Does this need to be public? I think this can cause a confusion on what
this is in the rendered documentation.

IIUC this is for internal atomic impl only
and this is not useful otherwise. This can be moved into `internal` and
then `pub(super) use internal::OrderingType` to stop exposing it.

(Or, just `#[doc(hidden)]` so it doesn't show in the docs).

> +
> +mod internal {
> + /// Unit types for ordering annotation.
> + ///
> + /// Sealed trait, can be only implemented inside atomic mod.
> + pub trait OrderingUnit {
> + /// Describes the exact memory ordering.
> + const TYPE: super::OrderingType;
> + }
> +}
> +
> +impl internal::OrderingUnit for Relaxed {
> + const TYPE: OrderingType = OrderingType::Relaxed;
> +}
> +
> +impl internal::OrderingUnit for Acquire {
> + const TYPE: OrderingType = OrderingType::Acquire;
> +}
> +
> +impl internal::OrderingUnit for Release {
> + const TYPE: OrderingType = OrderingType::Release;
> +}
> +
> +impl internal::OrderingUnit for Full {
> + const TYPE: OrderingType = OrderingType::Full;
> +}
> +
> +/// The trait bound for annotating operations that should support all orderings.
> +pub trait All: internal::OrderingUnit {}
> +
> +impl All for Relaxed {}
> +impl All for Acquire {}
> +impl All for Release {}
> +impl All for Full {}
> +
> +/// 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;

This should not be needed. I'd prefer to the use site to just match on
`TYPE`.

> +}
> +
> +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;
> +}
> +
> +/// The trait bound for operations that only support relaxed ordering.
> +pub trait RelaxedOnly: AcquireOrRelaxed + ReleaseOrRelaxed + All {}
> +
> +impl RelaxedOnly for Relaxed {}

Any reason that this is needed at all? Should just be a non-generic
function that takes a `Relaxed` directly?