Message-ID: <aF-aS5FLX7QIiiPa@Mac.home>
Date: Sat, 28 Jun 2025 00:31:23 -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 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

Powered by blists - more mailing lists