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

[Boqun Feng]

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`.
> 
[...]