xchg() and cmpxchg() are basic operations on atomic. Provide these based
on C APIs.
Note that cmpxchg() use the similar function signature as
compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means
the operation succeeds and `Err(old)` means the operation fails.
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
rust/kernel/sync/atomic/generic.rs | 154 +++++++++++++++++++++++++++++
1 file changed, 154 insertions(+)
diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
index 73c26f9cf6b8..bcdbeea45dd8 100644
--- a/rust/kernel/sync/atomic/generic.rs
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -256,3 +256,157 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
};
}
}
+
+impl<T: AllowAtomic> Atomic<T>
+where
+ T::Repr: AtomicHasXchgOps,
+{
+ /// Atomic exchange.
+ ///
+ /// # Examples
+ ///
+ /// ```rust
+ /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed};
+ ///
+ /// let x = Atomic::new(42);
+ ///
+ /// assert_eq!(42, x.xchg(52, Acquire));
+ /// assert_eq!(52, x.load(Relaxed));
+ /// ```
+ #[doc(alias("atomic_xchg", "atomic64_xchg"))]
+ #[inline(always)]
+ pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T {
+ let v = T::into_repr(v);
+ let a = self.as_ptr().cast::<T::Repr>();
+
+ // SAFETY:
+ // - For calling the atomic_xchg*() function:
+ // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`,
+ // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer,
+ // - per the type invariants, the following atomic operation won't cause data races.
+ // - For extra safety requirement of usage on pointers returned by `self.as_ptr():
+ // - atomic operations are used here.
+ let ret = unsafe {
+ match Ordering::TYPE {
+ OrderingType::Full => T::Repr::atomic_xchg(a, v),
+ OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v),
+ OrderingType::Release => T::Repr::atomic_xchg_release(a, v),
+ OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v),
+ }
+ };
+
+ T::from_repr(ret)
+ }
+
+ /// Atomic compare and exchange.
+ ///
+ /// Compare: The comparison is done via the byte level comparison between the atomic variables
+ /// with the `old` value.
+ ///
+ /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type
+ /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a
+ /// failed cmpxchg should be treated as a relaxed read.
+ ///
+ /// Returns `Ok(value)` if cmpxchg succeeds, and `value` is guaranteed to be equal to `old`,
+ /// otherwise returns `Err(value)`, and `value` is the value of the atomic variable when
+ /// cmpxchg was happening.
+ ///
+ /// # Examples
+ ///
+ /// ```rust
+ /// use kernel::sync::atomic::{Atomic, Full, Relaxed};
+ ///
+ /// let x = Atomic::new(42);
+ ///
+ /// // Checks whether cmpxchg succeeded.
+ /// let success = x.cmpxchg(52, 64, Relaxed).is_ok();
+ /// # assert!(!success);
+ ///
+ /// // Checks whether cmpxchg failed.
+ /// let failure = x.cmpxchg(52, 64, Relaxed).is_err();
+ /// # assert!(failure);
+ ///
+ /// // Uses the old value if failed, probably re-try cmpxchg.
+ /// match x.cmpxchg(52, 64, Relaxed) {
+ /// Ok(_) => { },
+ /// Err(old) => {
+ /// // do something with `old`.
+ /// # assert_eq!(old, 42);
+ /// }
+ /// }
+ ///
+ /// // Uses the latest value regardlessly, same as atomic_cmpxchg() in C.
+ /// let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old);
+ /// # assert_eq!(42, latest);
+ /// assert_eq!(64, x.load(Relaxed));
+ /// ```
+ #[doc(alias(
+ "atomic_cmpxchg",
+ "atomic64_cmpxchg",
+ "atomic_try_cmpxchg",
+ "atomic64_try_cmpxchg"
+ ))]
+ #[inline(always)]
+ pub fn cmpxchg<Ordering: All>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> {
+ // Note on code generation:
+ //
+ // try_cmpxchg() is used to implement cmpxchg(), and if the helper functions are inlined,
+ // the compiler is able to figure out that branch is not needed if the users don't care
+ // about whether the operation succeeds or not. One exception is on x86, due to commit
+ // 44fe84459faf ("locking/atomic: Fix atomic_try_cmpxchg() semantics"), the
+ // atomic_try_cmpxchg() on x86 has a branch even if the caller doesn't care about the
+ // success of cmpxchg and only wants to use the old value. For example, for code like:
+ //
+ // let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old);
+ //
+ // It will still generate code:
+ //
+ // movl $0x40, %ecx
+ // movl $0x34, %eax
+ // lock
+ // cmpxchgl %ecx, 0x4(%rsp)
+ // jne 1f
+ // 2:
+ // ...
+ // 1: movl %eax, %ecx
+ // jmp 2b
+ //
+ // This might be "fixed" by introducing a try_cmpxchg_exclusive() that knows the "*old"
+ // location in the C function is always safe to write.
+ if self.try_cmpxchg(&mut old, new, o) {
+ Ok(old)
+ } else {
+ Err(old)
+ }
+ }
+
+ /// Atomic compare and exchange and returns whether the operation succeeds.
+ ///
+ /// "Compare" and "Ordering" part are the same as [`Atomic::cmpxchg()`].
+ ///
+ /// Returns `true` means the cmpxchg succeeds otherwise returns `false` with `old` updated to
+ /// the value of the atomic variable when cmpxchg was happening.
+ #[inline(always)]
+ fn try_cmpxchg<Ordering: All>(&self, old: &mut T, new: T, _: Ordering) -> bool {
+ let old = (old as *mut T).cast::<T::Repr>();
+ let new = T::into_repr(new);
+ let a = self.0.get().cast::<T::Repr>();
+
+ // SAFETY:
+ // - For calling the atomic_try_cmpchg*() function:
+ // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllowAtomic`,
+ // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer,
+ // - per the type invariants, the following atomic operation won't cause data races.
+ // - `old` is a valid pointer to write because it comes from a mutable reference.
+ // - For extra safety requirement of usage on pointers returned by `self.as_ptr():
+ // - atomic operations are used here.
+ unsafe {
+ match Ordering::TYPE {
+ OrderingType::Full => T::Repr::atomic_try_cmpxchg(a, old, new),
+ OrderingType::Acquire => T::Repr::atomic_try_cmpxchg_acquire(a, old, new),
+ OrderingType::Release => T::Repr::atomic_try_cmpxchg_release(a, old, new),
+ OrderingType::Relaxed => T::Repr::atomic_try_cmpxchg_relaxed(a, old, new),
+ }
+ }
+ }
+}
--
2.39.5 (Apple Git-154)
On Wed Jun 18, 2025 at 6:49 PM CEST, Boqun Feng wrote: > +impl<T: AllowAtomic> Atomic<T> > +where > + T::Repr: AtomicHasXchgOps, > +{ > + /// Atomic exchange. > + /// > + /// # Examples > + /// > + /// ```rust > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; > + /// > + /// let x = Atomic::new(42); > + /// > + /// assert_eq!(42, x.xchg(52, Acquire)); > + /// assert_eq!(52, x.load(Relaxed)); > + /// ``` > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] > + #[inline(always)] > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { Can we name this `exchange`? > + let v = T::into_repr(v); > + let a = self.as_ptr().cast::<T::Repr>(); > + > + // SAFETY: > + // - For calling the atomic_xchg*() function: > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > + // - per the type invariants, the following atomic operation won't cause data races. > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > + // - atomic operations are used here. > + let ret = unsafe { > + match Ordering::TYPE { > + OrderingType::Full => T::Repr::atomic_xchg(a, v), > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), > + } > + }; > + > + T::from_repr(ret) > + } > + > + /// Atomic compare and exchange. > + /// > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > + /// with the `old` value. > + /// > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > + /// failed cmpxchg should be treated as a relaxed read. This is a bit confusing to me. The operation has a store and a load operation and both can have different orderings (at least in Rust userland) depending on the success/failure of the operation. In userland, I can supply `AcqRel` and `Acquire` to ensure that I always have Acquire semantics on any read and `Release` semantics on any write (which I would think is a common case). How do I do this using your API? Don't I need `Acquire` semantics on the read in order for `compare_exchange` to give me the correct behavior in this example: pub struct Foo { data: Atomic<u64>, new: Atomic<bool>, ready: Atomic<bool>, } impl Foo { pub fn new() -> Self { Self { data: Atomic::new(0), new: Atomic::new(false), ready: Atomic::new(false), } } pub fn get(&self) -> Option<u64> { if self.new.compare_exchange(true, false, Release).is_ok() { let val = self.data.load(Acquire); self.ready.store(false, Release); Some(val) } else { None } } pub fn set(&self, val: u64) -> Result<(), u64> { if self.ready.compare_exchange(false, true, Release).is_ok() { self.data.store(val, Release); self.new.store(true, Release); } else { Err(val) } } } IIUC, you need `Acquire` ordering on both `compare_exchange` operations' reads for this to work, right? Because if they are relaxed, this could happen: Thread 0 | Thread 1 ------------------------------------------------|------------------------------------------------ get() { | set(42) { | if ready.cmpxchg(false, true, Rel).is_ok() { | data.store(42, Rel) | new.store(true, Rel) if new.cmpxchg(true, false, Rel).is_ok() { | let val = self.data.load(Acq); // reads 0 | ready.store(false, Rel); | Some(val) | } | } } | } So essentially, the `data.store` operation is not synchronized, because the read on `new` is not `Acquire`. > + /// > + /// Returns `Ok(value)` if cmpxchg succeeds, and `value` is guaranteed to be equal to `old`, > + /// otherwise returns `Err(value)`, and `value` is the value of the atomic variable when > + /// cmpxchg was happening. > + /// > + /// # Examples > + /// > + /// ```rust > + /// use kernel::sync::atomic::{Atomic, Full, Relaxed}; > + /// > + /// let x = Atomic::new(42); > + /// > + /// // Checks whether cmpxchg succeeded. > + /// let success = x.cmpxchg(52, 64, Relaxed).is_ok(); > + /// # assert!(!success); > + /// > + /// // Checks whether cmpxchg failed. > + /// let failure = x.cmpxchg(52, 64, Relaxed).is_err(); > + /// # assert!(failure); > + /// > + /// // Uses the old value if failed, probably re-try cmpxchg. > + /// match x.cmpxchg(52, 64, Relaxed) { > + /// Ok(_) => { }, > + /// Err(old) => { > + /// // do something with `old`. > + /// # assert_eq!(old, 42); > + /// } > + /// } > + /// > + /// // Uses the latest value regardlessly, same as atomic_cmpxchg() in C. > + /// let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old); > + /// # assert_eq!(42, latest); > + /// assert_eq!(64, x.load(Relaxed)); > + /// ``` > + #[doc(alias( > + "atomic_cmpxchg", > + "atomic64_cmpxchg", > + "atomic_try_cmpxchg", > + "atomic64_try_cmpxchg" > + ))] > + #[inline(always)] > + pub fn cmpxchg<Ordering: All>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> { `compare_exchange`? > + /// Atomic compare and exchange and returns whether the operation succeeds. > + /// > + /// "Compare" and "Ordering" part are the same as [`Atomic::cmpxchg()`]. > + /// > + /// Returns `true` means the cmpxchg succeeds otherwise returns `false` with `old` updated to > + /// the value of the atomic variable when cmpxchg was happening. > + #[inline(always)] > + fn try_cmpxchg<Ordering: All>(&self, old: &mut T, new: T, _: Ordering) -> bool { `try_compare_exchange`? --- Cheers, Benno > + let old = (old as *mut T).cast::<T::Repr>(); > + let new = T::into_repr(new); > + let a = self.0.get().cast::<T::Repr>(); > + > + // SAFETY: > + // - For calling the atomic_try_cmpchg*() function: > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllowAtomic`, > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > + // - per the type invariants, the following atomic operation won't cause data races. > + // - `old` is a valid pointer to write because it comes from a mutable reference. > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > + // - atomic operations are used here. > + unsafe { > + match Ordering::TYPE { > + OrderingType::Full => T::Repr::atomic_try_cmpxchg(a, old, new), > + OrderingType::Acquire => T::Repr::atomic_try_cmpxchg_acquire(a, old, new), > + OrderingType::Release => T::Repr::atomic_try_cmpxchg_release(a, old, new), > + OrderingType::Relaxed => T::Repr::atomic_try_cmpxchg_relaxed(a, old, new), > + } > + } > + } > +}
On Fri, Jun 27, 2025 at 10:58:43AM +0200, Benno Lossin wrote: > On Wed Jun 18, 2025 at 6:49 PM CEST, Boqun Feng wrote: > > +impl<T: AllowAtomic> Atomic<T> > > +where > > + T::Repr: AtomicHasXchgOps, > > +{ > > + /// Atomic exchange. > > + /// > > + /// # Examples > > + /// > > + /// ```rust > > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; > > + /// > > + /// let x = Atomic::new(42); > > + /// > > + /// assert_eq!(42, x.xchg(52, Acquire)); > > + /// assert_eq!(52, x.load(Relaxed)); > > + /// ``` > > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] > > + #[inline(always)] > > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { > > Can we name this `exchange`? > FYI, in Rust std, this operation is called `swap()`, what's the reason of using a name that is neither the Rust convention nor Linux kernel convention? As for naming, the reason I choose xchg() and cmpxchg() is because they are the name LKMM uses for a long time, to use another name, we have to have a very good reason to do so and I don't see a good reason that the other names are better, especially, in our memory model, we use xchg() and cmpxchg() a lot, and they are different than Rust version where you can specify orderings separately. Naming LKMM xchg()/cmpxchg() would cause more confusion I believe. Same answer for compare_exchange() vs cmpxchg(). > > + let v = T::into_repr(v); > > + let a = self.as_ptr().cast::<T::Repr>(); > > + > > + // SAFETY: > > + // - For calling the atomic_xchg*() function: > > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, > > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > > + // - per the type invariants, the following atomic operation won't cause data races. > > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > > + // - atomic operations are used here. > > + let ret = unsafe { > > + match Ordering::TYPE { > > + OrderingType::Full => T::Repr::atomic_xchg(a, v), > > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), > > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), > > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), > > + } > > + }; > > + > > + T::from_repr(ret) > > + } > > + > > + /// Atomic compare and exchange. > > + /// > > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > > + /// with the `old` value. > > + /// > > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > > + /// failed cmpxchg should be treated as a relaxed read. > > This is a bit confusing to me. The operation has a store and a load > operation and both can have different orderings (at least in Rust > userland) depending on the success/failure of the operation. In > userland, I can supply `AcqRel` and `Acquire` to ensure that I always > have Acquire semantics on any read and `Release` semantics on any write > (which I would think is a common case). How do I do this using your API? > Usually in kernel that means in a failure case you need to use a barrier afterwards, for example: if (old != cmpxchg(v, old, new)) { smp_mb(); // ^ following memory operations are ordered against. } > Don't I need `Acquire` semantics on the read in order for > `compare_exchange` to give me the correct behavior in this example: > > pub struct Foo { > data: Atomic<u64>, > new: Atomic<bool>, > ready: Atomic<bool>, > } > > impl Foo { > pub fn new() -> Self { > Self { > data: Atomic::new(0), > new: Atomic::new(false), > ready: Atomic::new(false), > } > } > > pub fn get(&self) -> Option<u64> { > if self.new.compare_exchange(true, false, Release).is_ok() { You should use `Full` if you want AcqRel-like behavior when succeed. > let val = self.data.load(Acquire); > self.ready.store(false, Release); > Some(val) > } else { > None > } > } > > pub fn set(&self, val: u64) -> Result<(), u64> { > if self.ready.compare_exchange(false, true, Release).is_ok() { Same. Regards, Boqun > self.data.store(val, Release); > self.new.store(true, Release); > } else { > Err(val) > } > } > } > > IIUC, you need `Acquire` ordering on both `compare_exchange` operations' > reads for this to work, right? Because if they are relaxed, this could > happen: > > Thread 0 | Thread 1 > ------------------------------------------------|------------------------------------------------ > get() { | set(42) { > | if ready.cmpxchg(false, true, Rel).is_ok() { > | data.store(42, Rel) > | new.store(true, Rel) > if new.cmpxchg(true, false, Rel).is_ok() { | > let val = self.data.load(Acq); // reads 0 | > ready.store(false, Rel); | > Some(val) | > } | } > } | } > > So essentially, the `data.store` operation is not synchronized, because > the read on `new` is not `Acquire`. > [...]
On Fri Jun 27, 2025 at 3:53 PM CEST, Boqun Feng wrote: > On Fri, Jun 27, 2025 at 10:58:43AM +0200, Benno Lossin wrote: >> On Wed Jun 18, 2025 at 6:49 PM CEST, Boqun Feng wrote: >> > +impl<T: AllowAtomic> Atomic<T> >> > +where >> > + T::Repr: AtomicHasXchgOps, >> > +{ >> > + /// Atomic exchange. >> > + /// >> > + /// # Examples >> > + /// >> > + /// ```rust >> > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; >> > + /// >> > + /// let x = Atomic::new(42); >> > + /// >> > + /// assert_eq!(42, x.xchg(52, Acquire)); >> > + /// assert_eq!(52, x.load(Relaxed)); >> > + /// ``` >> > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] >> > + #[inline(always)] >> > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { >> >> Can we name this `exchange`? >> > > FYI, in Rust std, this operation is called `swap()`, what's the reason > of using a name that is neither the Rust convention nor Linux kernel > convention? Ah, well then my suggestion would be `swap()` instead :) > As for naming, the reason I choose xchg() and cmpxchg() is because they > are the name LKMM uses for a long time, to use another name, we have to > have a very good reason to do so and I don't see a good reason > that the other names are better, especially, in our memory model, we use > xchg() and cmpxchg() a lot, and they are different than Rust version > where you can specify orderings separately. Naming LKMM xchg()/cmpxchg() > would cause more confusion I believe. I'm just not used to the name shortening from the kernel... I think it's fine to use them especially since the ordering parameters differ from std's atomics. Can you add aliases for the Rust names? > Same answer for compare_exchange() vs cmpxchg(). > >> > + let v = T::into_repr(v); >> > + let a = self.as_ptr().cast::<T::Repr>(); >> > + >> > + // SAFETY: >> > + // - For calling the atomic_xchg*() function: >> > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, >> > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, >> > + // - per the type invariants, the following atomic operation won't cause data races. >> > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): >> > + // - atomic operations are used here. >> > + let ret = unsafe { >> > + match Ordering::TYPE { >> > + OrderingType::Full => T::Repr::atomic_xchg(a, v), >> > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), >> > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), >> > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), >> > + } >> > + }; >> > + >> > + T::from_repr(ret) >> > + } >> > + >> > + /// Atomic compare and exchange. >> > + /// >> > + /// Compare: The comparison is done via the byte level comparison between the atomic variables >> > + /// with the `old` value. >> > + /// >> > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type >> > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a >> > + /// failed cmpxchg should be treated as a relaxed read. >> >> This is a bit confusing to me. The operation has a store and a load >> operation and both can have different orderings (at least in Rust >> userland) depending on the success/failure of the operation. In >> userland, I can supply `AcqRel` and `Acquire` to ensure that I always >> have Acquire semantics on any read and `Release` semantics on any write >> (which I would think is a common case). How do I do this using your API? >> > > Usually in kernel that means in a failure case you need to use a barrier > afterwards, for example: > > if (old != cmpxchg(v, old, new)) { > smp_mb(); > // ^ following memory operations are ordered against. > } Do we already have abstractions for those? >> Don't I need `Acquire` semantics on the read in order for >> `compare_exchange` to give me the correct behavior in this example: >> >> pub struct Foo { >> data: Atomic<u64>, >> new: Atomic<bool>, >> ready: Atomic<bool>, >> } >> >> impl Foo { >> pub fn new() -> Self { >> Self { >> data: Atomic::new(0), >> new: Atomic::new(false), >> ready: Atomic::new(false), >> } >> } >> >> pub fn get(&self) -> Option<u64> { >> if self.new.compare_exchange(true, false, Release).is_ok() { > > You should use `Full` if you want AcqRel-like behavior when succeed. I think it would be pretty valuable to document this. Also any other "direct" translations from the Rust memory model are useful. For example is `SeqCst` "equivalent" to `Full`? --- Cheers, Benno
On Sat, Jun 28, 2025 at 08:12:42AM +0200, Benno Lossin wrote: > On Fri Jun 27, 2025 at 3:53 PM CEST, Boqun Feng wrote: > > On Fri, Jun 27, 2025 at 10:58:43AM +0200, Benno Lossin wrote: > >> On Wed Jun 18, 2025 at 6:49 PM CEST, Boqun Feng wrote: > >> > +impl<T: AllowAtomic> Atomic<T> > >> > +where > >> > + T::Repr: AtomicHasXchgOps, > >> > +{ > >> > + /// Atomic exchange. > >> > + /// > >> > + /// # Examples > >> > + /// > >> > + /// ```rust > >> > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; > >> > + /// > >> > + /// let x = Atomic::new(42); > >> > + /// > >> > + /// assert_eq!(42, x.xchg(52, Acquire)); > >> > + /// assert_eq!(52, x.load(Relaxed)); > >> > + /// ``` > >> > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] > >> > + #[inline(always)] > >> > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { > >> > >> Can we name this `exchange`? > >> > > > > FYI, in Rust std, this operation is called `swap()`, what's the reason > > of using a name that is neither the Rust convention nor Linux kernel > > convention? > > Ah, well then my suggestion would be `swap()` instead :) > ;-) > > As for naming, the reason I choose xchg() and cmpxchg() is because they > > are the name LKMM uses for a long time, to use another name, we have to > > have a very good reason to do so and I don't see a good reason > > that the other names are better, especially, in our memory model, we use > > xchg() and cmpxchg() a lot, and they are different than Rust version > > where you can specify orderings separately. Naming LKMM xchg()/cmpxchg() > > would cause more confusion I believe. > > I'm just not used to the name shortening from the kernel... I think it's I guess it's a bit curse of knowledge from my side... > fine to use them especially since the ordering parameters differ from > std's atomics. > > Can you add aliases for the Rust names? > I can, but I also want to see a real user request ;-) As a bi-model user myself, I generally don't mind the name, as you can see C++ and Rust use different names as well, what I usually do is just "tell me what's the name of the function if I need to do this" ;-) > > Same answer for compare_exchange() vs cmpxchg(). > > > >> > + let v = T::into_repr(v); > >> > + let a = self.as_ptr().cast::<T::Repr>(); > >> > + > >> > + // SAFETY: > >> > + // - For calling the atomic_xchg*() function: > >> > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, > >> > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > >> > + // - per the type invariants, the following atomic operation won't cause data races. > >> > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > >> > + // - atomic operations are used here. > >> > + let ret = unsafe { > >> > + match Ordering::TYPE { > >> > + OrderingType::Full => T::Repr::atomic_xchg(a, v), > >> > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), > >> > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), > >> > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), > >> > + } > >> > + }; > >> > + > >> > + T::from_repr(ret) > >> > + } > >> > + > >> > + /// Atomic compare and exchange. > >> > + /// > >> > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > >> > + /// with the `old` value. > >> > + /// > >> > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > >> > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > >> > + /// failed cmpxchg should be treated as a relaxed read. > >> > >> This is a bit confusing to me. The operation has a store and a load > >> operation and both can have different orderings (at least in Rust > >> userland) depending on the success/failure of the operation. In > >> userland, I can supply `AcqRel` and `Acquire` to ensure that I always > >> have Acquire semantics on any read and `Release` semantics on any write > >> (which I would think is a common case). How do I do this using your API? > >> > > > > Usually in kernel that means in a failure case you need to use a barrier > > afterwards, for example: > > > > if (old != cmpxchg(v, old, new)) { > > smp_mb(); > > // ^ following memory operations are ordered against. > > } > > Do we already have abstractions for those? > You mean the smp_mb()? Yes it's in patch #10. > >> Don't I need `Acquire` semantics on the read in order for > >> `compare_exchange` to give me the correct behavior in this example: > >> > >> pub struct Foo { > >> data: Atomic<u64>, > >> new: Atomic<bool>, > >> ready: Atomic<bool>, > >> } > >> > >> impl Foo { > >> pub fn new() -> Self { > >> Self { > >> data: Atomic::new(0), > >> new: Atomic::new(false), > >> ready: Atomic::new(false), > >> } > >> } > >> > >> pub fn get(&self) -> Option<u64> { > >> if self.new.compare_exchange(true, false, Release).is_ok() { > > > > You should use `Full` if you want AcqRel-like behavior when succeed. > > I think it would be pretty valuable to document this. Also any other > "direct" translations from the Rust memory model are useful. For example I don't disagree. But I'm afraid it'll still a learning process for everyone. Usually as a kernel developer, when working on concurrent code, the thought process is not 1) "write it in Rust/C++ memory model" and then 2) "translate to LKMM atomics", it's usually just write directly because already learned patterns from kernel code. So while I'm confident that I can answer any translation question you come up with, but I don't have a full list yet. Also I don't know whether it's worth doing, because of the thought process thing I mentioned above. My sincere suggestion to anyone who wants to do concurrent programming in kernel is just "learn the LKMM" (or "use a lock" ;-)). There are good learning materials in LWN, also you can check out the tools/memory-model/ for the model, documentation and tools. Either you are familiar with a few concepts in memory model areas, or you have learned the LKMM, otherwise I'm afraid there's no short-cut for one to pick up LKMM atomics correctly and precisely with a few translation rules from Rust native atomics. The other thing to note is that there could be multiple "translations", for example for this particular case, we can also do: pub fn get(&self) -> Option<u64> { if self.new.cmpxchg(true, false, Release).is_ok() { smp_mb(); // Ordering the load part of cmpxchg() with the // following memory accesses, i.e. providing at // least the Acquire ordering. let val = self.data.load(Acquire); self.ready.store(false, Release); } else { None } } So whatever the document is, it might not be accurate/complete, and might be misleading. > is `SeqCst` "equivalent" to `Full`? No ;-) How many hours do you have? (It's a figurative question, I probably need to go to sleep now ;-)) For example, `SeqCst` on atomic read-modify-write operations maps to acquire+release atomics on ARM64 I believe, but a `Full` atomic is acquire+release plus a full memory barrier on ARM64. Also a `Full` atomic implies a full memory barrier (smp_mb()), but a `SeqCst` atomic is not a `SeqCst` fence. Regards, Boqun > > --- > Cheers, > Benno
On Sat Jun 28, 2025 at 9:31 AM CEST, Boqun Feng wrote: > On Sat, Jun 28, 2025 at 08:12:42AM +0200, Benno Lossin wrote: >> On Fri Jun 27, 2025 at 3:53 PM CEST, Boqun Feng wrote: >> > As for naming, the reason I choose xchg() and cmpxchg() is because they >> > are the name LKMM uses for a long time, to use another name, we have to >> > have a very good reason to do so and I don't see a good reason >> > that the other names are better, especially, in our memory model, we use >> > xchg() and cmpxchg() a lot, and they are different than Rust version >> > where you can specify orderings separately. Naming LKMM xchg()/cmpxchg() >> > would cause more confusion I believe. >> >> I'm just not used to the name shortening from the kernel... I think it's > > I guess it's a bit curse of knowledge from my side... > >> fine to use them especially since the ordering parameters differ from >> std's atomics. >> >> Can you add aliases for the Rust names? >> > > I can, but I also want to see a real user request ;-) As a bi-model user > myself, I generally don't mind the name, as you can see C++ and Rust use > different names as well, what I usually do is just "tell me what's the > name of the function if I need to do this" ;-) I think learning Rust in the kernel is different from learning a new language. Yes you're learning a specific dialect of Rust, but that's what every project does. You also added aliases for the C versions, so let's also add the Rust ones :) >> >> Don't I need `Acquire` semantics on the read in order for >> >> `compare_exchange` to give me the correct behavior in this example: >> >> >> >> pub struct Foo { >> >> data: Atomic<u64>, >> >> new: Atomic<bool>, >> >> ready: Atomic<bool>, >> >> } >> >> >> >> impl Foo { >> >> pub fn new() -> Self { >> >> Self { >> >> data: Atomic::new(0), >> >> new: Atomic::new(false), >> >> ready: Atomic::new(false), >> >> } >> >> } >> >> >> >> pub fn get(&self) -> Option<u64> { >> >> if self.new.compare_exchange(true, false, Release).is_ok() { >> > >> > You should use `Full` if you want AcqRel-like behavior when succeed. >> >> I think it would be pretty valuable to document this. Also any other >> "direct" translations from the Rust memory model are useful. For example > > I don't disagree. But I'm afraid it'll still a learning process for > everyone. Usually as a kernel developer, when working on concurrent > code, the thought process is not 1) "write it in Rust/C++ memory model" > and then 2) "translate to LKMM atomics", it's usually just write > directly because already learned patterns from kernel code. That's fair. Maybe just me clinging to the only straw that I have :) (it also isn't a good straw, I barely know my way around the atomics in std :) > So while I'm confident that I can answer any translation question you > come up with, but I don't have a full list yet. > > Also I don't know whether it's worth doing, because of the thought > process thing I mentioned above. Yeah makes sense. > My sincere suggestion to anyone who wants to do concurrent programming > in kernel is just "learn the LKMM" (or "use a lock" ;-)). There are good > learning materials in LWN, also you can check out the > tools/memory-model/ for the model, documentation and tools. I'm luckily not in the position of having to use atomics for anything :) > Either you are familiar with a few concepts in memory model areas, or > you have learned the LKMM, otherwise I'm afraid there's no short-cut for > one to pick up LKMM atomics correctly and precisely with a few > translation rules from Rust native atomics. > > The other thing to note is that there could be multiple "translations", > for example for this particular case, we can also do: > > pub fn get(&self) -> Option<u64> { > if self.new.cmpxchg(true, false, Release).is_ok() { > smp_mb(); // Ordering the load part of cmpxchg() with the > // following memory accesses, i.e. providing at > // least the Acquire ordering. > let val = self.data.load(Acquire); > self.ready.store(false, Release); > } else { > None > } > } > > So whatever the document is, it might not be accurate/complete, and > might be misleading. Yeah. >> is `SeqCst` "equivalent" to `Full`? > > No ;-) How many hours do you have? (It's a figurative question, I > probably need to go to sleep now ;-)) For example, `SeqCst` on atomic > read-modify-write operations maps to acquire+release atomics on ARM64 I > believe, but a `Full` atomic is acquire+release plus a full memory > barrier on ARM64. Also a `Full` atomic implies a full memory barrier > (smp_mb()), but a `SeqCst` atomic is not a `SeqCst` fence. Thanks for the quick explanation, I would have been satisfied with "No" :) --- Cheers, Benno
On Sat, Jun 28, 2025 at 10:00:34AM +0200, Benno Lossin wrote: > On Sat Jun 28, 2025 at 9:31 AM CEST, Boqun Feng wrote: > > On Sat, Jun 28, 2025 at 08:12:42AM +0200, Benno Lossin wrote: > >> On Fri Jun 27, 2025 at 3:53 PM CEST, Boqun Feng wrote: > >> > As for naming, the reason I choose xchg() and cmpxchg() is because they > >> > are the name LKMM uses for a long time, to use another name, we have to > >> > have a very good reason to do so and I don't see a good reason > >> > that the other names are better, especially, in our memory model, we use > >> > xchg() and cmpxchg() a lot, and they are different than Rust version > >> > where you can specify orderings separately. Naming LKMM xchg()/cmpxchg() > >> > would cause more confusion I believe. > >> > >> I'm just not used to the name shortening from the kernel... I think it's > > > > I guess it's a bit curse of knowledge from my side... > > > >> fine to use them especially since the ordering parameters differ from > >> std's atomics. > >> > >> Can you add aliases for the Rust names? > >> > > > > I can, but I also want to see a real user request ;-) As a bi-model user > > myself, I generally don't mind the name, as you can see C++ and Rust use > > different names as well, what I usually do is just "tell me what's the > > name of the function if I need to do this" ;-) > > I think learning Rust in the kernel is different from learning a new > language. Yes you're learning a specific dialect of Rust, but that's > what every project does. > > You also added aliases for the C versions, so let's also add the Rust > ones :) > Make senses, so added: --- a/rust/kernel/sync/atomic/generic.rs +++ b/rust/kernel/sync/atomic/generic.rs @@ -310,7 +310,7 @@ impl<T: AllowAtomic> Atomic<T> /// assert_eq!(42, x.xchg(52, Acquire)); /// assert_eq!(52, x.load(Relaxed)); /// ``` - #[doc(alias("atomic_xchg", "atomic64_xchg"))] + #[doc(alias("atomic_xchg", "atomic64_xchg", "swap"))] #[inline(always)] pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T { let v = T::into_repr(v); @@ -382,6 +382,7 @@ pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T { "atomic64_cmpxchg", "atomic_try_cmpxchg", "atomic64_try_cmpxchg" + "compare_exchange" ))] #[inline(always)] pub fn cmpxchg<Ordering: Any>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> { Seems good? Regards, Boqun
On Mon Jun 30, 2025 at 5:24 PM CEST, Boqun Feng wrote: > On Sat, Jun 28, 2025 at 10:00:34AM +0200, Benno Lossin wrote: >> On Sat Jun 28, 2025 at 9:31 AM CEST, Boqun Feng wrote: >> > On Sat, Jun 28, 2025 at 08:12:42AM +0200, Benno Lossin wrote: >> >> On Fri Jun 27, 2025 at 3:53 PM CEST, Boqun Feng wrote: >> >> > As for naming, the reason I choose xchg() and cmpxchg() is because they >> >> > are the name LKMM uses for a long time, to use another name, we have to >> >> > have a very good reason to do so and I don't see a good reason >> >> > that the other names are better, especially, in our memory model, we use >> >> > xchg() and cmpxchg() a lot, and they are different than Rust version >> >> > where you can specify orderings separately. Naming LKMM xchg()/cmpxchg() >> >> > would cause more confusion I believe. >> >> >> >> I'm just not used to the name shortening from the kernel... I think it's >> > >> > I guess it's a bit curse of knowledge from my side... >> > >> >> fine to use them especially since the ordering parameters differ from >> >> std's atomics. >> >> >> >> Can you add aliases for the Rust names? >> >> >> > >> > I can, but I also want to see a real user request ;-) As a bi-model user >> > myself, I generally don't mind the name, as you can see C++ and Rust use >> > different names as well, what I usually do is just "tell me what's the >> > name of the function if I need to do this" ;-) >> >> I think learning Rust in the kernel is different from learning a new >> language. Yes you're learning a specific dialect of Rust, but that's >> what every project does. >> >> You also added aliases for the C versions, so let's also add the Rust >> ones :) >> > > Make senses, so added: > > --- a/rust/kernel/sync/atomic/generic.rs > +++ b/rust/kernel/sync/atomic/generic.rs > @@ -310,7 +310,7 @@ impl<T: AllowAtomic> Atomic<T> > /// assert_eq!(42, x.xchg(52, Acquire)); > /// assert_eq!(52, x.load(Relaxed)); > /// ``` > - #[doc(alias("atomic_xchg", "atomic64_xchg"))] > + #[doc(alias("atomic_xchg", "atomic64_xchg", "swap"))] > #[inline(always)] > pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T { > let v = T::into_repr(v); > @@ -382,6 +382,7 @@ pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T { > "atomic64_cmpxchg", > "atomic_try_cmpxchg", > "atomic64_try_cmpxchg" > + "compare_exchange" > ))] > #[inline(always)] > pub fn cmpxchg<Ordering: Any>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> { > > Seems good? Yeah, thanks! --- Cheers, Benno
On Mon, Jun 30, 2025 at 08:24:13AM -0700, Boqun Feng wrote: [...] > > Make senses, so added: > > --- a/rust/kernel/sync/atomic/generic.rs > +++ b/rust/kernel/sync/atomic/generic.rs > @@ -310,7 +310,7 @@ impl<T: AllowAtomic> Atomic<T> > /// assert_eq!(42, x.xchg(52, Acquire)); > /// assert_eq!(52, x.load(Relaxed)); > /// ``` > - #[doc(alias("atomic_xchg", "atomic64_xchg"))] > + #[doc(alias("atomic_xchg", "atomic64_xchg", "swap"))] > #[inline(always)] > pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T { > let v = T::into_repr(v); > @@ -382,6 +382,7 @@ pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T { > "atomic64_cmpxchg", > "atomic_try_cmpxchg", > "atomic64_try_cmpxchg" Missing a comma here, fixed locally. Regards, Boqun > + "compare_exchange" > ))] > #[inline(always)] > pub fn cmpxchg<Ordering: Any>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> { > > Seems good? > > Regards, > Boqun
"Boqun Feng" <boqun.feng@gmail.com> writes: > xchg() and cmpxchg() are basic operations on atomic. Provide these based > on C APIs. > > Note that cmpxchg() use the similar function signature as > compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means > the operation succeeds and `Err(old)` means the operation fails. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > rust/kernel/sync/atomic/generic.rs | 154 +++++++++++++++++++++++++++++ > 1 file changed, 154 insertions(+) > > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs > index 73c26f9cf6b8..bcdbeea45dd8 100644 > --- a/rust/kernel/sync/atomic/generic.rs > +++ b/rust/kernel/sync/atomic/generic.rs > @@ -256,3 +256,157 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) { > }; > } > } > + > +impl<T: AllowAtomic> Atomic<T> > +where > + T::Repr: AtomicHasXchgOps, > +{ > + /// Atomic exchange. > + /// > + /// # Examples > + /// > + /// ```rust > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; > + /// > + /// let x = Atomic::new(42); > + /// > + /// assert_eq!(42, x.xchg(52, Acquire)); > + /// assert_eq!(52, x.load(Relaxed)); > + /// ``` > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] > + #[inline(always)] > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { > + let v = T::into_repr(v); > + let a = self.as_ptr().cast::<T::Repr>(); > + > + // SAFETY: > + // - For calling the atomic_xchg*() function: > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, Typo: `AllowAtomic`. > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > + // - per the type invariants, the following atomic operation won't cause data races. > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > + // - atomic operations are used here. > + let ret = unsafe { > + match Ordering::TYPE { > + OrderingType::Full => T::Repr::atomic_xchg(a, v), > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), > + } > + }; > + > + T::from_repr(ret) > + } > + > + /// Atomic compare and exchange. > + /// > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > + /// with the `old` value. > + /// > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > + /// failed cmpxchg should be treated as a relaxed read. Rust `core::ptr` functions have this sentence on success ordering for compare_exchange: Using Acquire as success ordering makes the store part of this operation Relaxed, and using Release makes the successful load Relaxed. Does this translate to LKMM cmpxchg operations? If so, I think we should include this sentence. This also applies to `Atomic::xchg`. Best regards, Andreas Hindborg
On Thu, Jun 26, 2025 at 03:12:12PM +0200, Andreas Hindborg wrote: > "Boqun Feng" <boqun.feng@gmail.com> writes: > > > xchg() and cmpxchg() are basic operations on atomic. Provide these based > > on C APIs. > > > > Note that cmpxchg() use the similar function signature as > > compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means > > the operation succeeds and `Err(old)` means the operation fails. > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > --- > > rust/kernel/sync/atomic/generic.rs | 154 +++++++++++++++++++++++++++++ > > 1 file changed, 154 insertions(+) > > > > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs > > index 73c26f9cf6b8..bcdbeea45dd8 100644 > > --- a/rust/kernel/sync/atomic/generic.rs > > +++ b/rust/kernel/sync/atomic/generic.rs > > @@ -256,3 +256,157 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) { > > }; > > } > > } > > + > > +impl<T: AllowAtomic> Atomic<T> > > +where > > + T::Repr: AtomicHasXchgOps, > > +{ > > + /// Atomic exchange. > > + /// > > + /// # Examples > > + /// > > + /// ```rust > > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; > > + /// > > + /// let x = Atomic::new(42); > > + /// > > + /// assert_eq!(42, x.xchg(52, Acquire)); > > + /// assert_eq!(52, x.load(Relaxed)); > > + /// ``` > > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] > > + #[inline(always)] > > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { > > + let v = T::into_repr(v); > > + let a = self.as_ptr().cast::<T::Repr>(); > > + > > + // SAFETY: > > + // - For calling the atomic_xchg*() function: > > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, > > Typo: `AllowAtomic`. > Fixed. > > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > > + // - per the type invariants, the following atomic operation won't cause data races. > > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > > + // - atomic operations are used here. > > + let ret = unsafe { > > + match Ordering::TYPE { > > + OrderingType::Full => T::Repr::atomic_xchg(a, v), > > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), > > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), > > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), > > + } > > + }; > > + > > + T::from_repr(ret) > > + } > > + > > + /// Atomic compare and exchange. > > + /// > > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > > + /// with the `old` value. > > + /// > > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > > + /// failed cmpxchg should be treated as a relaxed read. > > Rust `core::ptr` functions have this sentence on success ordering for > compare_exchange: > > Using Acquire as success ordering makes the store part of this > operation Relaxed, and using Release makes the successful load > Relaxed. > > Does this translate to LKMM cmpxchg operations? If so, I think we should > include this sentence. This also applies to `Atomic::xchg`. > I see this as a different style of documenting, so in my next version, I have the following: //! - [`Acquire`] provides ordering between the load part of the annotated operation and all the //! following memory accesses. //! - [`Release`] provides ordering between all the preceding memory accesses and the store part of //! the annotated operation. in atomic/ordering.rs, I think I can extend it to: //! - [`Acquire`] provides ordering between the load part of the annotated operation and all the //! following memory accesses, and if there is a store part, it has Relaxed ordering. //! - [`Release`] provides ordering between all the preceding memory accesses and the store part of //! the annotated operation, and if there is load part, it has Relaxed ordering This aligns with what we usually describe things in tool/memory-model/. Regards, Boqun > > Best regards, > Andreas Hindborg > >
"Boqun Feng" <boqun.feng@gmail.com> writes: > On Thu, Jun 26, 2025 at 03:12:12PM +0200, Andreas Hindborg wrote: >> "Boqun Feng" <boqun.feng@gmail.com> writes: >> >> > xchg() and cmpxchg() are basic operations on atomic. Provide these based >> > on C APIs. >> > >> > Note that cmpxchg() use the similar function signature as >> > compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means >> > the operation succeeds and `Err(old)` means the operation fails. >> > >> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> >> > --- >> > rust/kernel/sync/atomic/generic.rs | 154 +++++++++++++++++++++++++++++ >> > 1 file changed, 154 insertions(+) >> > >> > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs >> > index 73c26f9cf6b8..bcdbeea45dd8 100644 >> > --- a/rust/kernel/sync/atomic/generic.rs >> > +++ b/rust/kernel/sync/atomic/generic.rs >> > @@ -256,3 +256,157 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) { >> > }; >> > } >> > } >> > + >> > +impl<T: AllowAtomic> Atomic<T> >> > +where >> > + T::Repr: AtomicHasXchgOps, >> > +{ >> > + /// Atomic exchange. >> > + /// >> > + /// # Examples >> > + /// >> > + /// ```rust >> > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; >> > + /// >> > + /// let x = Atomic::new(42); >> > + /// >> > + /// assert_eq!(42, x.xchg(52, Acquire)); >> > + /// assert_eq!(52, x.load(Relaxed)); >> > + /// ``` >> > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] >> > + #[inline(always)] >> > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { >> > + let v = T::into_repr(v); >> > + let a = self.as_ptr().cast::<T::Repr>(); >> > + >> > + // SAFETY: >> > + // - For calling the atomic_xchg*() function: >> > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, >> >> Typo: `AllowAtomic`. >> > > Fixed. > >> > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, >> > + // - per the type invariants, the following atomic operation won't cause data races. >> > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): >> > + // - atomic operations are used here. >> > + let ret = unsafe { >> > + match Ordering::TYPE { >> > + OrderingType::Full => T::Repr::atomic_xchg(a, v), >> > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), >> > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), >> > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), >> > + } >> > + }; >> > + >> > + T::from_repr(ret) >> > + } >> > + >> > + /// Atomic compare and exchange. >> > + /// >> > + /// Compare: The comparison is done via the byte level comparison between the atomic variables >> > + /// with the `old` value. >> > + /// >> > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type >> > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a >> > + /// failed cmpxchg should be treated as a relaxed read. >> >> Rust `core::ptr` functions have this sentence on success ordering for >> compare_exchange: >> >> Using Acquire as success ordering makes the store part of this >> operation Relaxed, and using Release makes the successful load >> Relaxed. >> >> Does this translate to LKMM cmpxchg operations? If so, I think we should >> include this sentence. This also applies to `Atomic::xchg`. >> > > I see this as a different style of documenting, so in my next version, > I have the following: > > //! - [`Acquire`] provides ordering between the load part of the annotated operation and all the > //! following memory accesses. > //! - [`Release`] provides ordering between all the preceding memory accesses and the store part of > //! the annotated operation. > > in atomic/ordering.rs, I think I can extend it to: > > //! - [`Acquire`] provides ordering between the load part of the annotated operation and all the > //! following memory accesses, and if there is a store part, it has Relaxed ordering. > //! - [`Release`] provides ordering between all the preceding memory accesses and the store part of > //! the annotated operation, and if there is load part, it has Relaxed ordering > > This aligns with what we usually describe things in tool/memory-model/. Cool. When you start to go into details of ordering concepts, I feel like something is missing though. For example for this sentence: [`Release`] provides ordering between all the preceding memory accesses and the store part of the annotated operation. I guess this provided ordering is only guaranteed to be observable for threads that read the same location with `Acquire` or stronger ordering? If we start expanding on the orderings, rather than deferring to LKMM, we should include this info. Best regards, Andreas Hindborg
On Mon, Jun 30, 2025 at 12:16:27PM +0200, Andreas Hindborg wrote: > "Boqun Feng" <boqun.feng@gmail.com> writes: > > in atomic/ordering.rs, I think I can extend it to: > > > > //! - [`Acquire`] provides ordering between the load part of the annotated operation and all the > > //! following memory accesses, and if there is a store part, it has Relaxed ordering. > > //! - [`Release`] provides ordering between all the preceding memory accesses and the store part of > > //! the annotated operation, and if there is load part, it has Relaxed ordering > > > > This aligns with what we usually describe things in tool/memory-model/. > > Cool. When you start to go into details of ordering concepts, I feel > like something is missing though. For example for this sentence: > > [`Release`] provides ordering between all the preceding memory > accesses and the store part of the annotated operation. > > I guess this provided ordering is only guaranteed to be observable for > threads that read the same location with `Acquire` or stronger ordering? > > If we start expanding on the orderings, rather than deferring to LKMM, > we should include this info. The problem with the word "ordering" is that it is too general, not specific enough. You need more context to know exactly what the ordering means. For example, ordering store A against store B (which comes later in the code) could mean that the CPU executes A before it executes B. Or it could mean that a different CPU will see the data from A before it sees the data from B. A more explicit description would be helpful. Alan Stern
On Mon, Jun 30, 2025 at 10:51:00AM -0400, Alan Stern wrote: > On Mon, Jun 30, 2025 at 12:16:27PM +0200, Andreas Hindborg wrote: > > "Boqun Feng" <boqun.feng@gmail.com> writes: > > > in atomic/ordering.rs, I think I can extend it to: > > > > > > //! - [`Acquire`] provides ordering between the load part of the annotated operation and all the > > > //! following memory accesses, and if there is a store part, it has Relaxed ordering. > > > //! - [`Release`] provides ordering between all the preceding memory accesses and the store part of > > > //! the annotated operation, and if there is load part, it has Relaxed ordering > > > > > > This aligns with what we usually describe things in tool/memory-model/. > > > > Cool. When you start to go into details of ordering concepts, I feel Well, avoiding going too much into the details is what I wanted for those documentation comments ;-) > > like something is missing though. For example for this sentence: > > > > [`Release`] provides ordering between all the preceding memory > > accesses and the store part of the annotated operation. > > > > I guess this provided ordering is only guaranteed to be observable for > > threads that read the same location with `Acquire` or stronger ordering? > > > > If we start expanding on the orderings, rather than deferring to LKMM, > > we should include this info. I'm not sure I follow you on this one. I'm not trying to expand on orderings, instead I'm trying to resolve your feedback that my previous version didn't mention what ordering the unspecific part of a read-modify-write has (like the store part of an Acquire and load part of the Release). And to me, they are naturally just Relaxed, but I feel that given the feedback I got from you, maybe I should explicitly mention they are Relaxed. It's simply just making things more explicit and I'm still deferring to LKMM about the exact meaning of the ordering. > > The problem with the word "ordering" is that it is too general, not > specific enough. You need more context to know exactly what the > ordering means. > > For example, ordering store A against store B (which comes later in the > code) could mean that the CPU executes A before it executes B. Or it > could mean that a different CPU will see the data from A before it sees > the data from B. > > A more explicit description would be helpful. > Except that the explicit description should be in tools/memory-model/ instead of the comments of a function or the Rust atomic module. Like Documentation/atomic_t.txt vs tools/memory-model/Documentation/explanation.txt. Because one is for people to get a quick idea about what these annotations/suffixes mean, and the other is the more elaborate on their precise meaning. Most of the cases, people won't need to get the subtilly of the memory model to write correct code. Regards, Boqun > Alan Stern
On Wed, 18 Jun 2025 09:49:29 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > xchg() and cmpxchg() are basic operations on atomic. Provide these based > on C APIs. > > Note that cmpxchg() use the similar function signature as > compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means > the operation succeeds and `Err(old)` means the operation fails. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > rust/kernel/sync/atomic/generic.rs | 154 +++++++++++++++++++++++++++++ > 1 file changed, 154 insertions(+) > > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs > index 73c26f9cf6b8..bcdbeea45dd8 100644 > --- a/rust/kernel/sync/atomic/generic.rs > +++ b/rust/kernel/sync/atomic/generic.rs > @@ -256,3 +256,157 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) { > }; > } > } > + > +impl<T: AllowAtomic> Atomic<T> > +where > + T::Repr: AtomicHasXchgOps, > +{ > + /// Atomic exchange. > + /// > + /// # Examples > + /// > + /// ```rust > + /// use kernel::sync::atomic::{Atomic, Acquire, Relaxed}; > + /// > + /// let x = Atomic::new(42); > + /// > + /// assert_eq!(42, x.xchg(52, Acquire)); > + /// assert_eq!(52, x.load(Relaxed)); > + /// ``` > + #[doc(alias("atomic_xchg", "atomic64_xchg"))] > + #[inline(always)] > + pub fn xchg<Ordering: All>(&self, v: T, _: Ordering) -> T { > + let v = T::into_repr(v); > + let a = self.as_ptr().cast::<T::Repr>(); > + > + // SAFETY: > + // - For calling the atomic_xchg*() function: > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllocAtomic`, > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > + // - per the type invariants, the following atomic operation won't cause data races. > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > + // - atomic operations are used here. > + let ret = unsafe { > + match Ordering::TYPE { > + OrderingType::Full => T::Repr::atomic_xchg(a, v), > + OrderingType::Acquire => T::Repr::atomic_xchg_acquire(a, v), > + OrderingType::Release => T::Repr::atomic_xchg_release(a, v), > + OrderingType::Relaxed => T::Repr::atomic_xchg_relaxed(a, v), > + } > + }; > + > + T::from_repr(ret) > + } > + > + /// Atomic compare and exchange. > + /// > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > + /// with the `old` value. > + /// > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > + /// failed cmpxchg should be treated as a relaxed read. > + /// > + /// Returns `Ok(value)` if cmpxchg succeeds, and `value` is guaranteed to be equal to `old`, > + /// otherwise returns `Err(value)`, and `value` is the value of the atomic variable when > + /// cmpxchg was happening. > + /// > + /// # Examples > + /// > + /// ```rust > + /// use kernel::sync::atomic::{Atomic, Full, Relaxed}; > + /// > + /// let x = Atomic::new(42); > + /// > + /// // Checks whether cmpxchg succeeded. > + /// let success = x.cmpxchg(52, 64, Relaxed).is_ok(); > + /// # assert!(!success); > + /// > + /// // Checks whether cmpxchg failed. > + /// let failure = x.cmpxchg(52, 64, Relaxed).is_err(); > + /// # assert!(failure); > + /// > + /// // Uses the old value if failed, probably re-try cmpxchg. > + /// match x.cmpxchg(52, 64, Relaxed) { > + /// Ok(_) => { }, > + /// Err(old) => { > + /// // do something with `old`. > + /// # assert_eq!(old, 42); > + /// } > + /// } > + /// > + /// // Uses the latest value regardlessly, same as atomic_cmpxchg() in C. > + /// let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old); > + /// # assert_eq!(42, latest); > + /// assert_eq!(64, x.load(Relaxed)); > + /// ``` > + #[doc(alias( > + "atomic_cmpxchg", > + "atomic64_cmpxchg", > + "atomic_try_cmpxchg", > + "atomic64_try_cmpxchg" > + ))] > + #[inline(always)] > + pub fn cmpxchg<Ordering: All>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> { > + // Note on code generation: > + // > + // try_cmpxchg() is used to implement cmpxchg(), and if the helper functions are inlined, > + // the compiler is able to figure out that branch is not needed if the users don't care > + // about whether the operation succeeds or not. One exception is on x86, due to commit > + // 44fe84459faf ("locking/atomic: Fix atomic_try_cmpxchg() semantics"), the > + // atomic_try_cmpxchg() on x86 has a branch even if the caller doesn't care about the > + // success of cmpxchg and only wants to use the old value. For example, for code like: > + // > + // let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old); > + // > + // It will still generate code: > + // > + // movl $0x40, %ecx > + // movl $0x34, %eax > + // lock > + // cmpxchgl %ecx, 0x4(%rsp) > + // jne 1f > + // 2: > + // ... > + // 1: movl %eax, %ecx > + // jmp 2b > + // > + // This might be "fixed" by introducing a try_cmpxchg_exclusive() that knows the "*old" > + // location in the C function is always safe to write. > + if self.try_cmpxchg(&mut old, new, o) { > + Ok(old) > + } else { > + Err(old) > + } > + } > + > + /// Atomic compare and exchange and returns whether the operation succeeds. > + /// > + /// "Compare" and "Ordering" part are the same as [`Atomic::cmpxchg()`]. > + /// > + /// Returns `true` means the cmpxchg succeeds otherwise returns `false` with `old` updated to > + /// the value of the atomic variable when cmpxchg was happening. > + #[inline(always)] > + fn try_cmpxchg<Ordering: All>(&self, old: &mut T, new: T, _: Ordering) -> bool { > + let old = (old as *mut T).cast::<T::Repr>(); > + let new = T::into_repr(new); > + let a = self.0.get().cast::<T::Repr>(); > + > + // SAFETY: > + // - For calling the atomic_try_cmpchg*() function: > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllowAtomic`, > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > + // - per the type invariants, the following atomic operation won't cause data races. > + // - `old` is a valid pointer to write because it comes from a mutable reference. > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > + // - atomic operations are used here. > + unsafe { > + match Ordering::TYPE { > + OrderingType::Full => T::Repr::atomic_try_cmpxchg(a, old, new), > + OrderingType::Acquire => T::Repr::atomic_try_cmpxchg_acquire(a, old, new), > + OrderingType::Release => T::Repr::atomic_try_cmpxchg_release(a, old, new), > + OrderingType::Relaxed => T::Repr::atomic_try_cmpxchg_relaxed(a, old, new), > + } > + } Again this function is only using `T::into_repr`, bypassing `T::from_repr` and just use pointer casting. BTW, any reason that this is a separate function, and it couldn't just be in `cmpxchg` function? > + } > +}
On Sat, Jun 21, 2025 at 12:37:53PM +0100, Gary Guo wrote: [...] > > + /// Atomic compare and exchange. > > + /// > > + /// Compare: The comparison is done via the byte level comparison between the atomic variables > > + /// with the `old` value. > > + /// > > + /// Ordering: When succeeds, provides the corresponding ordering as the `Ordering` type > > + /// parameter indicates, and a failed one doesn't provide any ordering, the read part of a > > + /// failed cmpxchg should be treated as a relaxed read. > > + /// > > + /// Returns `Ok(value)` if cmpxchg succeeds, and `value` is guaranteed to be equal to `old`, > > + /// otherwise returns `Err(value)`, and `value` is the value of the atomic variable when > > + /// cmpxchg was happening. > > + /// > > + /// # Examples > > + /// > > + /// ```rust > > + /// use kernel::sync::atomic::{Atomic, Full, Relaxed}; > > + /// > > + /// let x = Atomic::new(42); > > + /// > > + /// // Checks whether cmpxchg succeeded. > > + /// let success = x.cmpxchg(52, 64, Relaxed).is_ok(); > > + /// # assert!(!success); > > + /// > > + /// // Checks whether cmpxchg failed. > > + /// let failure = x.cmpxchg(52, 64, Relaxed).is_err(); > > + /// # assert!(failure); > > + /// > > + /// // Uses the old value if failed, probably re-try cmpxchg. > > + /// match x.cmpxchg(52, 64, Relaxed) { > > + /// Ok(_) => { }, > > + /// Err(old) => { > > + /// // do something with `old`. > > + /// # assert_eq!(old, 42); > > + /// } > > + /// } > > + /// > > + /// // Uses the latest value regardlessly, same as atomic_cmpxchg() in C. > > + /// let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old); > > + /// # assert_eq!(42, latest); > > + /// assert_eq!(64, x.load(Relaxed)); > > + /// ``` > > + #[doc(alias( > > + "atomic_cmpxchg", > > + "atomic64_cmpxchg", > > + "atomic_try_cmpxchg", > > + "atomic64_try_cmpxchg" > > + ))] > > + #[inline(always)] > > + pub fn cmpxchg<Ordering: All>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> { > > + // Note on code generation: > > + // > > + // try_cmpxchg() is used to implement cmpxchg(), and if the helper functions are inlined, > > + // the compiler is able to figure out that branch is not needed if the users don't care > > + // about whether the operation succeeds or not. One exception is on x86, due to commit > > + // 44fe84459faf ("locking/atomic: Fix atomic_try_cmpxchg() semantics"), the > > + // atomic_try_cmpxchg() on x86 has a branch even if the caller doesn't care about the > > + // success of cmpxchg and only wants to use the old value. For example, for code like: > > + // > > + // let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old); > > + // > > + // It will still generate code: > > + // > > + // movl $0x40, %ecx > > + // movl $0x34, %eax > > + // lock > > + // cmpxchgl %ecx, 0x4(%rsp) > > + // jne 1f > > + // 2: > > + // ... > > + // 1: movl %eax, %ecx > > + // jmp 2b > > + // > > + // This might be "fixed" by introducing a try_cmpxchg_exclusive() that knows the "*old" > > + // location in the C function is always safe to write. > > + if self.try_cmpxchg(&mut old, new, o) { > > + Ok(old) > > + } else { > > + Err(old) > > + } > > + } > > + > > + /// Atomic compare and exchange and returns whether the operation succeeds. > > + /// > > + /// "Compare" and "Ordering" part are the same as [`Atomic::cmpxchg()`]. > > + /// > > + /// Returns `true` means the cmpxchg succeeds otherwise returns `false` with `old` updated to > > + /// the value of the atomic variable when cmpxchg was happening. > > + #[inline(always)] > > + fn try_cmpxchg<Ordering: All>(&self, old: &mut T, new: T, _: Ordering) -> bool { > > + let old = (old as *mut T).cast::<T::Repr>(); > > + let new = T::into_repr(new); > > + let a = self.0.get().cast::<T::Repr>(); > > + > > + // SAFETY: > > + // - For calling the atomic_try_cmpchg*() function: > > + // - `self.as_ptr()` is a valid pointer, and per the safety requirement of `AllowAtomic`, > > + // a `*mut T` is a valid `*mut T::Repr`. Therefore `a` is a valid pointer, > > + // - per the type invariants, the following atomic operation won't cause data races. > > + // - `old` is a valid pointer to write because it comes from a mutable reference. > > + // - For extra safety requirement of usage on pointers returned by `self.as_ptr(): > > + // - atomic operations are used here. > > + unsafe { > > + match Ordering::TYPE { > > + OrderingType::Full => T::Repr::atomic_try_cmpxchg(a, old, new), > > + OrderingType::Acquire => T::Repr::atomic_try_cmpxchg_acquire(a, old, new), > > + OrderingType::Release => T::Repr::atomic_try_cmpxchg_release(a, old, new), > > + OrderingType::Relaxed => T::Repr::atomic_try_cmpxchg_relaxed(a, old, new), > > + } > > + } > > Again this function is only using `T::into_repr`, bypassing > `T::from_repr` and just use pointer casting. > > BTW, any reason that this is a separate function, and it couldn't just > be in `cmpxchg` function? > It's a non-public function, I feel it's easier to see that Rust's cmpxchg() is implemented via a try_cmpxchg() that is a wrapper of `atomic_try_cmpxchg*()`. Regards, Boqun > > > + } > > +} >
© 2016 - 2025 Red Hat, Inc.