Message-ID: <aEh_HEwSh2w0Ajkq@tardis.local>
Date: Tue, 10 Jun 2025 11:53:16 -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 10:58:30AM -0700, Boqun Feng wrote:
> On Tue, Jun 10, 2025 at 10:30:55AM -0700, Boqun Feng wrote:
> [...]
> > > > +/// 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?
> > 
> 
> Something like this seems to work:
> 
>     pub(super) mod private {
>         /// Describes the exact memory ordering of an `impl` [`All`].
>         pub enum Ordering {
>             /// Relaxed ordering.
>             Relaxed,
>             /// Acquire ordering.
>             Acquire,
>             /// Release ordering.
>             Release,
>             /// Fully-ordered.
>             Full,
>         }
>     
>         pub trait HasOrderingDesc {
>             /// Describes the exact memory ordering.
>             const ORDERING: Ordering;
>         }
>     }
> 
>     /// The trait bound for annotating operations that should support all orderings.
>     pub trait All: private::HasOrderingDesc { }
> 
>     impl private::HasOrderingDesc for Relaxed {
>         const ORDERING: private::Ordering = private::Ordering::Relaxed;
>     }
> 
> the trick is to seal the enum and the trait together.
> 
> Regards,
> Boqun
> 
> > > > +    /// 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`.
> > 

After a second thought, the following is probably what I will go for:

    /// 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,
    }
    
    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;
    }

That is:

1) Rename "OrderingDesc" into "OrderingType", and make it public.
2) Provide a sealed trait (`OrderingUnit`) for all the unit types
   that describe ordering.
3) Instead of "ORDER" or "ORDERING", name the enum constant "TYPE".


An example shows why is probably an xchg() implementation, if I was to
follow the previous naming suggestion, it will be:

    match Ordering::ORDERING {
        <some mode path>::Ordering::Relaxed => atomic_xchg_relaxed(...),
	...
    }

with the current one, it will be:

    match Ordering::TYPE {
        // assume we "use ordering::OrderingType"
        OrderingType::Relaxed => atomic_xchg_relaxed(...),
	...
    }

I think this version is much better.

Regards,
Boqun

> [..]

Powered by blists - more mailing lists