rust: sync: Atomic pointer
Subsystems (4)
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
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
@@ -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
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_RMWselected, the helpers of atomic load/store on i8 and i16 relies onCONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.The plan for
CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=narchitectures 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.
Suggested-by Dirk Behme <[email protected]>Suggested-by Benno Lossin <[email protected]>Signed-off-by Boqun Feng <[email protected]>
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@@ -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 ; }
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_RMWselected, the helpers of atomic load/store on i8 and i16 relies onCONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.The plan for
CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=narchitectures 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
Suggested-by Dirk Behme <[email protected]>Suggested-by Benno Lossin <[email protected]>Signed-off-by Boqun Feng <[email protected]>Reviewed-by Gary Guo <[email protected]>
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@@ -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:
Regards,
Boqun
Quoted 16 lines
Best,
GaryQuoted 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 ; }
Gary Guo
maintainerRe: [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_RMWselected, the helpers of atomic load/store on i8 and i16 relies onCONFIG_ARCH_SUPPORTS_ATOMIC_RMW=y. It's generally fine since most of architectures support that.The plan for
CONFIG_ARCH_SUPPORTS_ATOMIC_RMW=narchitectures 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
Suggested-by Dirk Behme <[email protected]>Suggested-by Benno Lossin <[email protected]>Signed-off-by Boqun Feng <[email protected]>Reviewed-by Gary Guo <[email protected]>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@@ -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_RMWsimilar is the following playground example:
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
PATCH v2 2/2 rust: sync: atomic: Add Atomic<*{mut,const} T> support
In reply to Boqun FengAtomic 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
@@ -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
@@ -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
@@ -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
Boqun Feng
AppliedQuoted 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_RMWas 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,
BoqunBoqun Feng (2):
rust: sync: atomic: Clarify the need of
CONFIG_ARCH_SUPPORTS_ATOMIC_RMW
rust: sync: atomic: Add Atomic<*{mut,const} T> supportrust/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