Message-Id: <DAJHY74JP27K.3VV3U3W0A9PCN@kernel.org>
Date: Wed, 11 Jun 2025 08:40:12 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
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 8:53 PM CEST, Boqun Feng wrote:
> 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.

Agreed :)

---
Cheers,
Benno

Powered by blists - more mailing lists