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

[Boqun Feng]

On Sat, Jun 21, 2025 at 12:18:42PM +0100, Gary Guo wrote:
[...]
> > +
> > +/// 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.
> 

I would like to make it public so that users can define their own method
with ordering out of atomic mod (even out of kernel crate):

    pub fn my_ordering_func<Ordering: All>(..., o: Ordering) {
        match Ordering::TYPE {

	}
    }

I just realized to do so I need to make OrderingUnit pub too (with a
sealed supertrait of course).

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

Seem reasonable.

> > +
> > +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;
> > +}
[...]
> > +
> > +/// 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`.
> 

Right, I somehow missed how monomorphization works. I can drop this.
Thanks!

> > +}
> > +
[...]
> > +/// 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

Mostly for documentation purpose, i.e. users can figure out the ordering
from the trait bounds of the function. I will say we can probably drop
it when we find a Release-only or Acquire-only function, but even then
the current definition won't affect users, so I lean torwards keeping
it.

Regards,
Boqun

> function that takes a `Relaxed` directly?
>