[PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations

Boqun Feng posted 10 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 3 weeks ago
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)
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Benno Lossin 3 months, 1 week ago
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),
> +            }
> +        }
> +    }
> +}
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 1 week ago
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`.
> 
[...]
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Benno Lossin 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Benno Lossin 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Benno Lossin 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Andreas Hindborg 3 months, 2 weeks ago
"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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 1 week ago
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
> 
>
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Andreas Hindborg 3 months, 1 week ago
"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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Alan Stern 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 1 week ago
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
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Gary Guo 3 months, 2 weeks ago
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?


> +    }
> +}
Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg operations
Posted by Boqun Feng 3 months, 2 weeks ago
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

> 
> > +    }
> > +}
>