Message-ID: <aEhrzxltkdnub_bR@tardis.local>
Date: Tue, 10 Jun 2025 10:30:55 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	lkmm@...ts.linux.dev, linux-arch@...r.kernel.org,
	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 11:07:16AM +0200, Benno Lossin wrote:
> 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.
> 

Thanks for taking a look!

> > ---
> >  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/ ?
> 

Good catch.

> > +//! - [`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)
> 

Yes, `Full` is an ordering that is stronger than `Acquire`.

> > +
> > +/// 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`?
> 

I was trying to avoid having an `Ordering` enum in a `ordering` mod.
Also I want to save the name "Ordering" for the generic type parameter
of an atomic operation, e.g.

    pub fn xchg<Ordering: ALL>(..)

this enum is more of an internal implementation detail, and users should
not use this enum directly, so I would like to avoid potential
confusion.

I have played a few sealed trait tricks on my end, but seems I cannot
achieve:

1) `OrderingDesc` is only accessible in the atomic mod.
2) `All` is only impl-able in the atomic mod, while it can be used as a
trait bound outside kernel crate.

Maybe there is a trick I'm missing?

> > +    /// 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`.

Make sense, thanks!

Regards,
Boqun

> 
> ---
> 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