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

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