[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DAXY0EJHHDWM.1KRSSJLOTCZ8F@kernel.org>
Date: Sat, 28 Jun 2025 08:12:42 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
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 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
Powered by blists - more mailing lists