[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aF6iXB6wiHcpAKIU@Mac.home>
Date: Fri, 27 Jun 2025 06:53:32 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
lkmm@...ts.linux.dev, linux-arch@...r.kernel.org,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>, Will Deacon <will@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Mark Rutland <mark.rutland@....com>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Viresh Kumar <viresh.kumar@...aro.org>,
Lyude Paul <lyude@...hat.com>, Ingo Molnar <mingo@...nel.org>,
Mitchell Levy <levymitchell0@...il.com>,
"Paul E. McKenney" <paulmck@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v5 05/10] rust: sync: atomic: Add atomic {cmp,}xchg
operations
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`.
>
[...]
Powered by blists - more mailing lists