[PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

Boqun Feng posted 9 patches 2 months, 4 weeks ago
There is a newer version of this series
[PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Boqun Feng 2 months, 4 weeks ago
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 `TYPE` associate
const is for generic function to pick up the particular implementation
specified by an ordering annotation.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs          |  3 +
 rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
 2 files changed, 100 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 c9c7c3617dd5..e80ac049f36b 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -17,3 +17,6 @@
 //! [`LKMM`]: srctree/tools/memory-model/
 
 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..5fffbaa2fa6d
--- /dev/null
+++ b/rust/kernel/sync/atomic/ordering.rs
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Memory orderings.
+//!
+//! The semantics of these orderings follows the [`LKMM`] definitions and rules.
+//!
+//! - [`Acquire`] provides ordering between the load part of the annotated operation and all the
+//!   following memory accesses, and if there is a store part, the store part has the [`Relaxed`]
+//!   ordering.
+//! - [`Release`] provides ordering between all the preceding memory accesses and the store part of
+//!   the annotated operation, and if there is a load part, the load part has the [`Relaxed`]
+//!   ordering.
+//! - [`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 strength as a full memory barrier (i.e. `smp_mb()`).
+//! - [`Relaxed`] provides no ordering except the dependency orderings. 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.
+#[doc(hidden)]
+pub enum OrderingType {
+    /// Relaxed ordering.
+    Relaxed,
+    /// Acquire ordering.
+    Acquire,
+    /// Release ordering.
+    Release,
+    /// Fully-ordered.
+    Full,
+}
+
+mod internal {
+    /// Sealed trait, can be only implemented inside atomic mod.
+    pub trait Sealed {}
+
+    impl Sealed for super::Relaxed {}
+    impl Sealed for super::Acquire {}
+    impl Sealed for super::Release {}
+    impl Sealed for super::Full {}
+}
+
+/// The trait bound for annotating operations that support any ordering.
+pub trait Any: internal::Sealed {
+    /// Describes the exact memory ordering.
+    const TYPE: OrderingType;
+}
+
+impl Any for Relaxed {
+    const TYPE: OrderingType = OrderingType::Relaxed;
+}
+
+impl Any for Acquire {
+    const TYPE: OrderingType = OrderingType::Acquire;
+}
+
+impl Any for Release {
+    const TYPE: OrderingType = OrderingType::Release;
+}
+
+impl Any for Full {
+    const TYPE: OrderingType = OrderingType::Full;
+}
+
+/// The trait bound for operations that only support acquire or relaxed ordering.
+pub trait AcquireOrRelaxed: Any {}
+
+impl AcquireOrRelaxed for Acquire {}
+impl AcquireOrRelaxed for Relaxed {}
+
+/// The trait bound for operations that only support release or relaxed ordering.
+pub trait ReleaseOrRelaxed: Any {}
+
+impl ReleaseOrRelaxed for Release {}
+impl ReleaseOrRelaxed for Relaxed {}
+
+/// The trait bound for operations that only support relaxed ordering.
+pub trait RelaxedOnly: AcquireOrRelaxed + ReleaseOrRelaxed + Any {}
+
+impl RelaxedOnly for Relaxed {}
-- 
2.39.5 (Apple Git-154)
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Benno Lossin 2 months, 4 weeks ago
On Thu Jul 10, 2025 at 8:00 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 `TYPE` associate
> const is for generic function to pick up the particular implementation
> specified by an ordering annotation.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

One naming comment below, with that fixed:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> ---
>  rust/kernel/sync/atomic.rs          |  3 +
>  rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 rust/kernel/sync/atomic/ordering.rs

> +/// The trait bound for annotating operations that support any ordering.
> +pub trait Any: internal::Sealed {

I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
should require people to write `ordering::Any` because otherwise it's
pretty confusing.

---
Cheers,
Benno

> +    /// Describes the exact memory ordering.
> +    const TYPE: OrderingType;
> +}
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Andreas Hindborg 2 months, 4 weeks ago
"Benno Lossin" <lossin@kernel.org> writes:

> On Thu Jul 10, 2025 at 8:00 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 `TYPE` associate
>> const is for generic function to pick up the particular implementation
>> specified by an ordering annotation.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>
> One naming comment below, with that fixed:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
>> ---
>>  rust/kernel/sync/atomic.rs          |  3 +
>>  rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
>>  2 files changed, 100 insertions(+)
>>  create mode 100644 rust/kernel/sync/atomic/ordering.rs
>
>> +/// The trait bound for annotating operations that support any ordering.
>> +pub trait Any: internal::Sealed {
>
> I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
> should require people to write `ordering::Any` because otherwise it's
> pretty confusing.

I agree with this observation.


Best regards,
Andreas Hindborg
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Boqun Feng 2 months, 4 weeks ago
On Thu, Jul 10, 2025 at 02:00:59PM +0200, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
> 
> > On Thu Jul 10, 2025 at 8:00 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 `TYPE` associate
> >> const is for generic function to pick up the particular implementation
> >> specified by an ordering annotation.
> >>
> >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >
> > One naming comment below, with that fixed:
> >
> > Reviewed-by: Benno Lossin <lossin@kernel.org>
> >
> >> ---
> >>  rust/kernel/sync/atomic.rs          |  3 +
> >>  rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
> >>  2 files changed, 100 insertions(+)
> >>  create mode 100644 rust/kernel/sync/atomic/ordering.rs
> >
> >> +/// The trait bound for annotating operations that support any ordering.
> >> +pub trait Any: internal::Sealed {
> >
> > I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
> > should require people to write `ordering::Any` because otherwise it's
> > pretty confusing.
> 
> I agree with this observation.
> 

I'm OK to do the change, but let me show my arguments ;-)

* First, we are using a language that supports namespaces,
  so I feel it's a bit unnecessary to use a different name just because
  it conflicts with `core::any::Any`. Doing so kinda undermines the
  namespace concepts. And we may have other `Any`s in the future, are we
  sure at the moment we should keyword `Any`?

* Another thing is that this trait won't be used very often outside
  definition of functions that having ordering variants, currently the
  only users are all inside atomic/generic.rs.

I probably choose the `ordering::Any` approach if you guys insist.

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 
>
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Benno Lossin 2 months, 4 weeks ago
On Thu Jul 10, 2025 at 4:42 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 02:00:59PM +0200, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>> > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> >> +/// The trait bound for annotating operations that support any ordering.
>> >> +pub trait Any: internal::Sealed {
>> >
>> > I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
>> > should require people to write `ordering::Any` because otherwise it's
>> > pretty confusing.
>> 
>> I agree with this observation.
>> 
>
> I'm OK to do the change, but let me show my arguments ;-)
>
> * First, we are using a language that supports namespaces,
>   so I feel it's a bit unnecessary to use a different name just because
>   it conflicts with `core::any::Any`. Doing so kinda undermines the
>   namespace concepts. And we may have other `Any`s in the future, are we
>   sure at the moment we should keyword `Any`?

I don't think `Any` is a good name for something this specific anyways.
If it were something private, then sure use `Any`, but since this is
public, I don't think `Any` is a good name.

> * Another thing is that this trait won't be used very often outside
>   definition of functions that having ordering variants, currently the
>   only users are all inside atomic/generic.rs.

I don't think this is a good argument to keep a bad name.

> I probably choose the `ordering::Any` approach if you guys insist.

I don't think we have a lint for that, so I'd prefer if we avoid that...

Someone is going to just `use ...::ordering::Any` and then have a
function `fn<T: Any>(_: T)` in their code and that will be very
confusing.

---
Cheers,
Benno
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Miguel Ojeda 2 months, 4 weeks ago
On Thu, Jul 10, 2025 at 5:05 PM Benno Lossin <lossin@kernel.org> wrote:
>
> I don't think we have a lint for that, so I'd prefer if we avoid that...
>
> Someone is going to just `use ...::ordering::Any` and then have a
> function `fn<T: Any>(_: T)` in their code and that will be very
> confusing.

I guess there could be a lint that detects a given item being `use`d
which we could use in some cases like this.

I took a quick look, and I see `enum_glob_use`, but that seems global
without a way to filter, and it doesn't cover direct `use`s.

Then there is `wildcard_imports`, and that seems fairly usable (it has
an allowed list), but of course doesn't cover non-wildcard ones.

Cheers,
Miguel
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Miguel Ojeda 2 months, 4 weeks ago
On Thu, Jul 10, 2025 at 8:32 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I guess there could be a lint that detects a given item being `use`d
> which we could use in some cases like this.

Filled: https://github.com/rust-lang/rust-clippy/issues/15244.

Cheers,
Miguel
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Boqun Feng 2 months, 4 weeks ago
On Thu, Jul 10, 2025 at 05:05:25PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 4:42 PM CEST, Boqun Feng wrote:
> > On Thu, Jul 10, 2025 at 02:00:59PM +0200, Andreas Hindborg wrote:
> >> "Benno Lossin" <lossin@kernel.org> writes:
> >> > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> >> >> +/// The trait bound for annotating operations that support any ordering.
> >> >> +pub trait Any: internal::Sealed {
> >> >
> >> > I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
> >> > should require people to write `ordering::Any` because otherwise it's
> >> > pretty confusing.
> >> 
> >> I agree with this observation.
> >> 
> >
> > I'm OK to do the change, but let me show my arguments ;-)
> >
> > * First, we are using a language that supports namespaces,
> >   so I feel it's a bit unnecessary to use a different name just because
> >   it conflicts with `core::any::Any`. Doing so kinda undermines the
> >   namespace concepts. And we may have other `Any`s in the future, are we
> >   sure at the moment we should keyword `Any`?
> 
> I don't think `Any` is a good name for something this specific anyways.

Well, that's the point of namespace: providing contexts for a name, and
the contexts can be very specific. I'm sure we both have used "any" in
English to refer something specific ;-)

> If it were something private, then sure use `Any`, but since this is
> public, I don't think `Any` is a good name.
> 

This essentially means we keyword `Any` as a public trait name, then we
should document it somewhere, along with other names we want to keyword.

Regards,
Boqun

> > * Another thing is that this trait won't be used very often outside
> >   definition of functions that having ordering variants, currently the
> >   only users are all inside atomic/generic.rs.
> 
> I don't think this is a good argument to keep a bad name.
> 
> > I probably choose the `ordering::Any` approach if you guys insist.
> 
> I don't think we have a lint for that, so I'd prefer if we avoid that...
> 
> Someone is going to just `use ...::ordering::Any` and then have a
> function `fn<T: Any>(_: T)` in their code and that will be very
> confusing.
> 
> ---
> Cheers,
> Benno
Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types
Posted by Benno Lossin 2 months, 4 weeks ago
On Thu Jul 10, 2025 at 5:57 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 05:05:25PM +0200, Benno Lossin wrote:
>> If it were something private, then sure use `Any`, but since this is
>> public, I don't think `Any` is a good name.
>> 
>
> This essentially means we keyword `Any` as a public trait name, then we
> should document it somewhere, along with other names we want to keyword.

Then let's restrict `Any`.

---
Cheers,
Benno