Skip to contentSkip to conversation

rust: sync: Atomic pointer

PATCH Applied Boqun Feng 2 participants 7 messages Jan 20 – Jan 23

Subsystems (4)
lore
Share & tools
lore

Boqun Feng

author
#0 lore raw

v2 cover rust: sync: Atomic pointer

I've already queued the first 3 patches in v1, but comments on them are welcome as well. The RCU pointer patch is not included in this v2 since there is not much update and I'm still hoping to get some reviews on that for another respin.

Changes since v1:

* Add a clarification patch of CONFIG_ARCH_SUPPORTS_ATOMIC_RMW as suggested by Dirk and Benno.

* Add support for Atomic<*const T>.

v1: https://lore.kernel.org/rust-for-linux/[email protected]/

Regards,
Boqun

Boqun Feng (2):
rust: sync: atomic: Clarify the need of
CONFIG_ARCH_SUPPORTS_ATOMIC_RMW
rust: sync: atomic: Add Atomic<*{mut,const} T> support

 rust/helpers/atomic_ext.c            |  3 ++
 rust/kernel/sync/atomic.rs           | 12 +++++++-
 rust/kernel/sync/atomic/internal.rs  | 37 ++++++++++++++--------
 rust/kernel/sync/atomic/predefine.rs | 46 ++++++++++++++++++++++++++++
 4 files changed, 85 insertions(+), 13 deletions(-)
Signature · 1 line
2.51.0
author
#1 lore raw

PATCH v2 1/2 rust: sync: atomic: Clarify the need of CONFIG_ARCH_SUPPORTS_ATOMIC_RMW

Currently, since all the architectures that support Rust all have CONFIG_ARCH_SUPPORTS_ATOMIC_RMW selected, the helpers of atomic load/store on i8 and i16 relies on CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.

The plan for CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architectures is adding their (probably lock-based) atomic load/store for i8 and i16 as their atomic_{read,set}() and atomic64_{read,set}() counterpart when they plans to support Rust.

Hence use a statis_assert!() to check this and remind the future us the need of the helpers. This is more clear than the #[cfg] on impl blocks of i8 and i16.

---
 rust/kernel/sync/atomic/internal.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
rust/kernel/sync/atomic/internal.rs +13 -6
index 0dac58bca2b3..ef516bcb02ee 100644
@@ -37,16 +37,23 @@ pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
     type Delta;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
+// The current helpers of load/store of atomic `i8` and `i16` use `{WRITE,READ}_ONCE()` hence the
+// atomicity is only guaranteed against read-modify-write operations if the architecture supports
+// native atomic RmW.
+//
+// In the future when a CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architecture plans to support Rust, the
+// load/store helpers that guarantee atomicity against RmW operations (usually via a lock) need to
+// be added.
+crate::static_assert!(
+    cfg!(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW),
+    "The current implementation of atomic i8/i16/ptr relies on the architecure being \
+    ARCH_SUPPORTS_ATOMIC_RMW"
+);
+
 impl AtomicImpl for i8 {
     type Delta = Self;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
 impl AtomicImpl for i16 {
     type Delta = Self;
 }
Signature · 1 line
2.51.0
maintainer Reviewed-by on v2 1/2
#2 lore raw

Re: [PATCH v2 1/2] rust: sync: atomic: Clarify the need of CONFIG_ARCH_SUPPORTS_ATOMIC_RMW

Quoted 10 lines · Boqun Feng

Currently, since all the architectures that support Rust all have CONFIG_ARCH_SUPPORTS_ATOMIC_RMW selected, the helpers of atomic load/store on i8 and i16 relies on CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.

The plan for CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architectures is adding their (probably lock-based) atomic load/store for i8 and i16 as their atomic_{read,set}() and atomic64_{read,set}() counterpart when they plans to support Rust.

Quoted 7 lines

Hence use a statis_assert!() to check this and remind the future us the need of the helpers. This is more clear than the #[cfg] on impl blocks of i8 and i16.

Quoted 16 lines
---
 rust/kernel/sync/atomic/internal.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
rust/kernel/sync/atomic/internal.rs +1 -3
index 0dac58bca2b3..ef516bcb02ee 100644
@@ -37,16 +37,23 @@ pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
     type Delta;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
+// The current helpers of load/store of atomic `i8` and `i16` use `{WRITE,READ}_ONCE()` hence the

rust/kernel/sync/atomic/internal.rs pub trait AtomicImpl: Sized + Send + Copy + private::Sealed { · line 50

Quoted 10 lines
rust/kernel/sync/atomic/internal.rs +10 -0
+// atomicity is only guaranteed against read-modify-write operations if the architecture supports
+// native atomic RmW.
+//
+// In the future when a CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architecture plans to support Rust, the
+// load/store helpers that guarantee atomicity against RmW operations (usually via a lock) need to
+// be added.
+crate::static_assert!(
+    cfg!(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW),
+    "The current implementation of atomic i8/i16/ptr relies on the architecure being \
+    ARCH_SUPPORTS_ATOMIC_RMW"

The printed string when assertion fails will have 5 spaces between "being" and "ARCH", although it probably doesn't matter..

Best,
Gary

Quoted 12 lines
rust/kernel/sync/atomic/internal.rs +2 -3
+);
+
 impl AtomicImpl for i8 {
     type Delta = Self;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
 impl AtomicImpl for i16 {
     type Delta = Self;
 }
author
#3 lore raw

Re: [PATCH v2 1/2] rust: sync: atomic: Clarify the need of CONFIG_ARCH_SUPPORTS_ATOMIC_RMW

Quoted 15 lines · Gary Guo
Quoted 14 lines · Boqun Feng

Currently, since all the architectures that support Rust all have CONFIG_ARCH_SUPPORTS_ATOMIC_RMW selected, the helpers of atomic load/store on i8 and i16 relies on CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.

The plan for CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architectures is adding their (probably lock-based) atomic load/store for i8 and i16 as their atomic_{read,set}() and atomic64_{read,set}() counterpart when they plans to support Rust.

Hence use a statis_assert!() to check this and remind the future us the need of the helpers. This is more clear than the #[cfg] on impl blocks of i8 and i16.

Quoted 6 lines
Quoted 3 lines

Thanks!

Quoted 21 lines
Quoted 21 lines
---
 rust/kernel/sync/atomic/internal.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
rust/kernel/sync/atomic/internal.rs +6 -3
index 0dac58bca2b3..ef516bcb02ee 100644
@@ -37,16 +37,23 @@ pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
     type Delta;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
+// The current helpers of load/store of atomic `i8` and `i16` use `{WRITE,READ}_ONCE()` hence the
+// atomicity is only guaranteed against read-modify-write operations if the architecture supports
+// native atomic RmW.
+//
+// In the future when a CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architecture plans to support Rust, the
+// load/store helpers that guarantee atomicity against RmW operations (usually via a lock) need to
Quoted 9 lines

rust/kernel/sync/atomic/internal.rs pub trait AtomicImpl: Sized + Send + Copy + private::Sealed { · line 50

Quoted 5 lines
rust/kernel/sync/atomic/internal.rs +5 -0
+// be added.
+crate::static_assert!(
+    cfg!(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW),
+    "The current implementation of atomic i8/i16/ptr relies on the architecure being \
+    ARCH_SUPPORTS_ATOMIC_RMW"

The printed string when assertion fails will have 5 spaces between "being" and "ARCH", although it probably doesn't matter..

Are you sure? My test result shows:

ERROR:root:error[E0080]: evaluation panicked: The current implementation of atomic i8/i16/ptr relies on the architecure being ARCH_SUPPORTS_ATOMIC_RMW

similar is the following playground example:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5dd0098247503be792bc35cda8f2630f

Regards,
Boqun

Quoted 16 lines

Best,
Gary

Quoted 12 lines
rust/kernel/sync/atomic/internal.rs +2 -3
+);
+
 impl AtomicImpl for i8 {
     type Delta = Self;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
 impl AtomicImpl for i16 {
     type Delta = Self;
 }
maintainer
#4 lore raw

Re: [PATCH v2 1/2] rust: sync: atomic: Clarify the need of CONFIG_ARCH_SUPPORTS_ATOMIC_RMW

Quoted 56 lines · Boqun Feng
Quoted 15 lines · Gary Guo
Quoted 14 lines · Boqun Feng

Currently, since all the architectures that support Rust all have CONFIG_ARCH_SUPPORTS_ATOMIC_RMW selected, the helpers of atomic load/store on i8 and i16 relies on CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.

The plan for CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architectures is adding their (probably lock-based) atomic load/store for i8 and i16 as their atomic_{read,set}() and atomic64_{read,set}() counterpart when they plans to support Rust.

Hence use a statis_assert!() to check this and remind the future us the need of the helpers. This is more clear than the #[cfg] on impl blocks of i8 and i16.

Quoted 6 lines
Quoted 3 lines

Thanks!

Quoted 21 lines
Quoted 21 lines
---
 rust/kernel/sync/atomic/internal.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
rust/kernel/sync/atomic/internal.rs +6 -3
index 0dac58bca2b3..ef516bcb02ee 100644
@@ -37,16 +37,23 @@ pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
     type Delta;
 }
 
-// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
-// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
-#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
+// The current helpers of load/store of atomic `i8` and `i16` use `{WRITE,READ}_ONCE()` hence the
+// atomicity is only guaranteed against read-modify-write operations if the architecture supports
+// native atomic RmW.
+//
+// In the future when a CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architecture plans to support Rust, the
+// load/store helpers that guarantee atomicity against RmW operations (usually via a lock) need to
Quoted 9 lines

rust/kernel/sync/atomic/internal.rs pub trait AtomicImpl: Sized + Send + Copy + private::Sealed { · line 50

Quoted 5 lines
rust/kernel/sync/atomic/internal.rs +5 -0
+// be added.
+crate::static_assert!(
+    cfg!(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW),
+    "The current implementation of atomic i8/i16/ptr relies on the architecure being \
+    ARCH_SUPPORTS_ATOMIC_RMW"

The printed string when assertion fails will have 5 spaces between "being" and "ARCH", although it probably doesn't matter..

Quoted 7 lines

Are you sure? My test result shows:

ERROR:root:error[E0080]: evaluation panicked: The current implementation of atomic i8/i16/ptr relies on the architecure being ARCH_SUPPORTS_ATOMIC_RMW

similar is the following playground example:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5dd0098247503be792bc35cda8f2630f

TIL! I thought Rust and C has the same behaviour with this respect.

I have been using indoc! to strip the additional whitespaces, but I suppose it's only for multiline strings but no line continuation.

Best,
Gary

author
#5 lore raw

PATCH v2 2/2 rust: sync: atomic: Add Atomic<*{mut,const} T> support

In reply to Boqun Feng

Atomic pointer support is an important piece of synchronization algorithm, e.g. RCU, hence provide the support for that.

Note that instead of relying on atomic_long or the implementation of Atomic<usize>, a new set of helpers (atomic_ptr_*) is introduced for atomic pointer specifically, this is because ptr2int casting would lose the provenance of a pointer and even though in theory there are a few tricks the provenance can be restored, it'll still be a simpler implementation if C could provide atomic pointers directly. The side effects of this approach are: we don't have the arithmetic and logical operations for pointers yet and the current implementation only works on ARCH_SUPPORTS_ATOMIC_RMW architectures, but these are implementation issues and can be added later.

---
 rust/helpers/atomic_ext.c            |  3 ++
 rust/kernel/sync/atomic.rs           | 12 +++++++-
 rust/kernel/sync/atomic/internal.rs  | 24 +++++++++------
 rust/kernel/sync/atomic/predefine.rs | 46 ++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 10 deletions(-)
rust/helpers/atomic_ext.c +3 -0
index 240218e2e708..c267d5190529 100644
@@ -36,6 +36,7 @@ __rust_helper void rust_helper_atomic_##tname##_set_release(type *ptr, type val)
 
 GEN_READ_SET_HELPERS(i8, s8)
 GEN_READ_SET_HELPERS(i16, s16)
+GEN_READ_SET_HELPERS(ptr, const void *)
 
 /*
  * xchg helpers depend on ARCH_SUPPORTS_ATOMIC_RMW and on the
@@ -59,6 +60,7 @@ rust_helper_atomic_##tname##_xchg##suffix(type *ptr, type new)			\
 
 GEN_XCHG_HELPERS(i8, s8)
 GEN_XCHG_HELPERS(i16, s16)
+GEN_XCHG_HELPERS(ptr, const void *)
 
 /*
  * try_cmpxchg helpers depend on ARCH_SUPPORTS_ATOMIC_RMW and on the
@@ -82,3 +84,4 @@ rust_helper_atomic_##tname##_try_cmpxchg##suffix(type *ptr, type *old, type new)
 
 GEN_TRY_CMPXCHG_HELPERS(i8, s8)
 GEN_TRY_CMPXCHG_HELPERS(i16, s16)
+GEN_TRY_CMPXCHG_HELPERS(ptr, const void *)
rust/kernel/sync/atomic.rs +11 -1
index 53177052b4b2..7ed8febe4a58 100644
@@ -52,6 +52,10 @@
 #[repr(transparent)]
 pub struct Atomic<T: AtomicType>(AtomicRepr<T::Repr>);
 
+// SAFETY: `Atomic<T>` is safe to transfer between execution contexts because of the safety
+// requirement of `AtomicType`.
+unsafe impl<T: AtomicType> Send for Atomic<T> {}
+
 // SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
 unsafe impl<T: AtomicType> Sync for Atomic<T> {}
 
@@ -69,6 +73,11 @@ unsafe impl<T: AtomicType> Sync for Atomic<T> {}
 ///
 /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
 /// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
+/// - [`Self`] must be safe to transfer between execution contexts, if it's [`Send`], this is
+///   automatically satisfied. The exception is pointer types that are even though marked as
+///   `!Send` (e.g. raw pointers and [`NonNull<T>`]) but requiring `unsafe` to do anything
+///   meaningful on them. This is because transferring pointer values between execution contexts is
+///   safe as long as the actual `unsafe` dereferencing is justified.
 ///
 /// Note that this is more relaxed than requiring the bi-directional transmutability (i.e.
 /// [`transmute()`] is always sound between `U` and `T`) because of the support for atomic
@@ -109,7 +118,8 @@ unsafe impl<T: AtomicType> Sync for Atomic<T> {}
 /// [`transmute()`]: core::mem::transmute
 /// [round-trip transmutable]: AtomicType#round-trip-transmutability
 /// [Examples]: AtomicType#examples
-pub unsafe trait AtomicType: Sized + Send + Copy {
+/// [`NonNull<T>`]: core::ptr::NonNull
+pub unsafe trait AtomicType: Sized + Copy {
     /// The backing atomic implementation type.
     type Repr: AtomicImpl;
 }
rust/kernel/sync/atomic/internal.rs +15 -9
index ef516bcb02ee..e301db4eaf91 100644
@@ -7,6 +7,7 @@
 use crate::bindings;
 use crate::macros::paste;
 use core::cell::UnsafeCell;
+use ffi::c_void;
 
 mod private {
     /// Sealed trait marker to disable customized impls on atomic implementation traits.
@@ -14,10 +15,11 @@ pub trait Sealed {}
 }
 
 // The C side supports atomic primitives only for `i32` and `i64` (`atomic_t` and `atomic64_t`),
-// while the Rust side also layers provides atomic support for `i8` and `i16`
-// on top of lower-level C primitives.
+// while the Rust side also provides atomic support for `i8`, `i16` and `*const c_void` on top of
+// lower-level C primitives.
 impl private::Sealed for i8 {}
 impl private::Sealed for i16 {}
+impl private::Sealed for *const c_void {}
 impl private::Sealed for i32 {}
 impl private::Sealed for i64 {}
 
@@ -26,10 +28,10 @@ impl private::Sealed for i64 {}
 /// This trait is sealed, and only types that map directly to the C side atomics
 /// or can be implemented with lower-level C primitives are allowed to implement this:
 ///
-/// - `i8` and `i16` are implemented with lower-level C primitives.
+/// - `i8`, `i16` and `*const c_void` are implemented with lower-level C primitives.
 /// - `i32` map to `atomic_t`
 /// - `i64` map to `atomic64_t`
-pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
+pub trait AtomicImpl: Sized + Copy + private::Sealed {
     /// The type of the delta in arithmetic or logical operations.
     ///
     /// For example, in `atomic_add(ptr, v)`, it's the type of `v`. Usually it's the same type of
@@ -37,9 +39,9 @@ pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
     type Delta;
 }
 
-// The current helpers of load/store of atomic `i8` and `i16` use `{WRITE,READ}_ONCE()` hence the
-// atomicity is only guaranteed against read-modify-write operations if the architecture supports
-// native atomic RmW.
+// The current helpers of load/store of atomic `i8`, `i16` and pointers use `{WRITE,READ}_ONCE()`
+// hence the atomicity is only guaranteed against read-modify-write operations if the architecture
+// supports native atomic RmW.
 //
 // In the future when a CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=n architecture plans to support Rust, the
 // load/store helpers that guarantee atomicity against RmW operations (usually via a lock) need to
@@ -58,6 +60,10 @@ impl AtomicImpl for i16 {
     type Delta = Self;
 }
 
+impl AtomicImpl for *const c_void {
+    type Delta = isize;
+}
+
 // `atomic_t` implements atomic operations on `i32`.
 impl AtomicImpl for i32 {
     type Delta = Self;
@@ -269,7 +275,7 @@ macro_rules! declare_and_impl_atomic_methods {
 }
 
 declare_and_impl_atomic_methods!(
-    [ i8 => atomic_i8, i16 => atomic_i16, i32 => atomic, i64 => atomic64 ]
+    [ i8 => atomic_i8, i16 => atomic_i16, *const c_void => atomic_ptr, i32 => atomic, i64 => atomic64 ]
     /// Basic atomic operations
     pub trait AtomicBasicOps {
         /// Atomic read (load).
@@ -287,7 +293,7 @@ fn set[release](a: &AtomicRepr<Self>, v: Self) {
 );
 
 declare_and_impl_atomic_methods!(
-    [ i8 => atomic_i8, i16 => atomic_i16, i32 => atomic, i64 => atomic64 ]
+    [ i8 => atomic_i8, i16 => atomic_i16, *const c_void => atomic_ptr, i32 => atomic, i64 => atomic64 ]
     /// Exchange and compare-and-exchange atomic operations
     pub trait AtomicExchangeOps {
         /// Atomic exchange.
rust/kernel/sync/atomic/predefine.rs +46 -0

62 lines not on this page · raw message

Signature · 1 line
2.51.0
author Applied
#6 lore raw
In reply to Boqun Feng
Quoted 5 lines · Boqun Feng

I've already queued the first 3 patches in v1, but comments on them are welcome as well. The RCU pointer patch is not included in this v2 since there is not much update and I'm still hoping to get some reviews on that for another respin.

Quoted 9 lines

Changes since v1:

* Add a clarification patch of CONFIG_ARCH_SUPPORTS_ATOMIC_RMW as suggested by Dirk and Benno.

* Add support for Atomic<*const T>.

v1: https://lore.kernel.org/rust-for-linux/[email protected]/

Queued for v7.1, thanks you all!

Regards,
Boqun

Quoted 18 lines

Regards,
Boqun

Boqun Feng (2):
rust: sync: atomic: Clarify the need of
CONFIG_ARCH_SUPPORTS_ATOMIC_RMW
rust: sync: atomic: Add Atomic<*{mut,const} T> support

 rust/helpers/atomic_ext.c            |  3 ++
 rust/kernel/sync/atomic.rs           | 12 +++++++-
 rust/kernel/sync/atomic/internal.rs  | 37 ++++++++++++++--------
 rust/kernel/sync/atomic/predefine.rs | 46 ++++++++++++++++++++++++++++
 4 files changed, 85 insertions(+), 13 deletions(-)
Signature · 1 line
2.51.0