[PATCH v6 0/9] LKMM generic atomics in Rust

[PATCH v6 0/9] LKMM generic atomics in Rust

[Boqun Feng]

Hi all,

This is the v6 of LKMM atomics in Rust, you can find the previous
versions at:

v5: https://lore.kernel.org/rust-for-linux/20250618164934.19817-1-boqun.feng@gmail.com/
v4: https://lore.kernel.org/rust-for-linux/20250609224615.27061-1-boqun.feng@gmail.com/
v3: https://lore.kernel.org/rust-for-linux/20250421164221.1121805-1-boqun.feng@gmail.com/
v2: https://lore.kernel.org/rust-for-linux/20241101060237.1185533-1-boqun.feng@gmail.com/
v1: https://lore.kernel.org/rust-for-linux/20240612223025.1158537-1-boqun.feng@gmail.com/
wip: https://lore.kernel.org/rust-for-linux/20240322233838.868874-1-boqun.feng@gmail.com/

I dropped the support for atomic pointers for now
(Atomic<{isize,usize}> still exists), because further more work is
needed to ensure the implementation could preserve provenance [1]. And
we already other series depending on atomics [2], so it makes sense to
get some basic support in-tree.

Peter & Ingo, I take it that it's OK for me to send a PR to tip later
this week or early next week if things went well? Of course, any
feedback is welcome!

Changes since v5:

* Replace `as` cast with `ptr.cast()` in atomic/ops.rs per the
  suggestion of Andreas.
* Explicitly document `Acquire` and `Release` ordering definition per
  the suggestion of Peter.
* Rename `All` to `Any` for ordering accept traits per the suggestion of
  Andreas.
* Remove unnecessary fields in `AcquireOrRelaxed` and `ReleaseOrRelaxed`
  per the suggestion of Gary.
* Add round-trip transmutability (thanks Benno) as the safety
  requirement of `AllowAtomic` per discussion with Gary and Benno.
* Add doc alias for xchg() and cmpxchg() per the suggestion of Benno.
* Examples and documentation improvement.
* Applied Reviewed-by tags from Alice and Andreas.

[1]: https://lore.kernel.org/rust-for-linux/aGg4sIORQiG02IoD@Mac.home/
[2]: https://lore.kernel.org/rust-for-linux/20250709-module-params-v3-v16-1-4f926bcccb50@kernel.org/

Regards,
Boqun

Boqun Feng (9):
  rust: Introduce atomic API helpers
  rust: sync: Add basic atomic operation mapping framework
  rust: sync: atomic: Add ordering annotation types
  rust: sync: atomic: Add generic atomics
  rust: sync: atomic: Add atomic {cmp,}xchg operations
  rust: sync: atomic: Add the framework of arithmetic operations
  rust: sync: atomic: Add Atomic<u{32,64}>
  rust: sync: Add memory barriers
  rust: sync: atomic: Add Atomic<{usize,isize}>

 MAINTAINERS                               |    4 +-
 rust/helpers/atomic.c                     | 1040 +++++++++++++++++++++
 rust/helpers/barrier.c                    |   18 +
 rust/helpers/helpers.c                    |    2 +
 rust/kernel/sync.rs                       |    2 +
 rust/kernel/sync/atomic.rs                |  193 ++++
 rust/kernel/sync/atomic/generic.rs        |  567 +++++++++++
 rust/kernel/sync/atomic/ops.rs            |  195 ++++
 rust/kernel/sync/atomic/ordering.rs       |   97 ++
 rust/kernel/sync/barrier.rs               |   65 ++
 scripts/atomic/gen-atomics.sh             |    1 +
 scripts/atomic/gen-rust-atomic-helpers.sh |   67 ++
 12 files changed, 2250 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/atomic.c
 create mode 100644 rust/helpers/barrier.c
 create mode 100644 rust/kernel/sync/atomic.rs
 create mode 100644 rust/kernel/sync/atomic/generic.rs
 create mode 100644 rust/kernel/sync/atomic/ops.rs
 create mode 100644 rust/kernel/sync/atomic/ordering.rs
 create mode 100644 rust/kernel/sync/barrier.rs
 create mode 100755 scripts/atomic/gen-rust-atomic-helpers.sh

-- 
2.39.5 (Apple Git-154)

[PATCH v6 1/9] rust: Introduce atomic API helpers

[Boqun Feng]

In order to support LKMM atomics in Rust, add rust_helper_* for atomic
APIs. These helpers ensure the implementation of LKMM atomics in Rust is
the same as in C. This could save the maintenance burden of having two
similar atomic implementations in asm.

Originally-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/helpers/atomic.c                     | 1040 +++++++++++++++++++++
 rust/helpers/helpers.c                    |    1 +
 scripts/atomic/gen-atomics.sh             |    1 +
 scripts/atomic/gen-rust-atomic-helpers.sh |   67 ++
 4 files changed, 1109 insertions(+)
 create mode 100644 rust/helpers/atomic.c
 create mode 100755 scripts/atomic/gen-rust-atomic-helpers.sh

diff --git a/rust/helpers/atomic.c b/rust/helpers/atomic.c
new file mode 100644
index 000000000000..cf06b7ef9a1c
--- /dev/null
+++ b/rust/helpers/atomic.c
@@ -0,0 +1,1040 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Generated by scripts/atomic/gen-rust-atomic-helpers.sh
+// DO NOT MODIFY THIS FILE DIRECTLY
+
+/*
+ * This file provides helpers for the various atomic functions for Rust.
+ */
+#ifndef _RUST_ATOMIC_API_H
+#define _RUST_ATOMIC_API_H
+
+#include <linux/atomic.h>
+
+// TODO: Remove this after INLINE_HELPERS support is added.
+#ifndef __rust_helper
+#define __rust_helper
+#endif
+
+__rust_helper int
+rust_helper_atomic_read(const atomic_t *v)
+{
+	return atomic_read(v);
+}
+
+__rust_helper int
+rust_helper_atomic_read_acquire(const atomic_t *v)
+{
+	return atomic_read_acquire(v);
+}
+
+__rust_helper void
+rust_helper_atomic_set(atomic_t *v, int i)
+{
+	atomic_set(v, i);
+}
+
+__rust_helper void
+rust_helper_atomic_set_release(atomic_t *v, int i)
+{
+	atomic_set_release(v, i);
+}
+
+__rust_helper void
+rust_helper_atomic_add(int i, atomic_t *v)
+{
+	atomic_add(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_add_return(int i, atomic_t *v)
+{
+	return atomic_add_return(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_add_return_acquire(int i, atomic_t *v)
+{
+	return atomic_add_return_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_add_return_release(int i, atomic_t *v)
+{
+	return atomic_add_return_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_add_return_relaxed(int i, atomic_t *v)
+{
+	return atomic_add_return_relaxed(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_add(int i, atomic_t *v)
+{
+	return atomic_fetch_add(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_add_acquire(int i, atomic_t *v)
+{
+	return atomic_fetch_add_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_add_release(int i, atomic_t *v)
+{
+	return atomic_fetch_add_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_add_relaxed(int i, atomic_t *v)
+{
+	return atomic_fetch_add_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic_sub(int i, atomic_t *v)
+{
+	atomic_sub(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_sub_return(int i, atomic_t *v)
+{
+	return atomic_sub_return(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_sub_return_acquire(int i, atomic_t *v)
+{
+	return atomic_sub_return_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_sub_return_release(int i, atomic_t *v)
+{
+	return atomic_sub_return_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_sub_return_relaxed(int i, atomic_t *v)
+{
+	return atomic_sub_return_relaxed(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_sub(int i, atomic_t *v)
+{
+	return atomic_fetch_sub(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_sub_acquire(int i, atomic_t *v)
+{
+	return atomic_fetch_sub_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_sub_release(int i, atomic_t *v)
+{
+	return atomic_fetch_sub_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_sub_relaxed(int i, atomic_t *v)
+{
+	return atomic_fetch_sub_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic_inc(atomic_t *v)
+{
+	atomic_inc(v);
+}
+
+__rust_helper int
+rust_helper_atomic_inc_return(atomic_t *v)
+{
+	return atomic_inc_return(v);
+}
+
+__rust_helper int
+rust_helper_atomic_inc_return_acquire(atomic_t *v)
+{
+	return atomic_inc_return_acquire(v);
+}
+
+__rust_helper int
+rust_helper_atomic_inc_return_release(atomic_t *v)
+{
+	return atomic_inc_return_release(v);
+}
+
+__rust_helper int
+rust_helper_atomic_inc_return_relaxed(atomic_t *v)
+{
+	return atomic_inc_return_relaxed(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_inc(atomic_t *v)
+{
+	return atomic_fetch_inc(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_inc_acquire(atomic_t *v)
+{
+	return atomic_fetch_inc_acquire(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_inc_release(atomic_t *v)
+{
+	return atomic_fetch_inc_release(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_inc_relaxed(atomic_t *v)
+{
+	return atomic_fetch_inc_relaxed(v);
+}
+
+__rust_helper void
+rust_helper_atomic_dec(atomic_t *v)
+{
+	atomic_dec(v);
+}
+
+__rust_helper int
+rust_helper_atomic_dec_return(atomic_t *v)
+{
+	return atomic_dec_return(v);
+}
+
+__rust_helper int
+rust_helper_atomic_dec_return_acquire(atomic_t *v)
+{
+	return atomic_dec_return_acquire(v);
+}
+
+__rust_helper int
+rust_helper_atomic_dec_return_release(atomic_t *v)
+{
+	return atomic_dec_return_release(v);
+}
+
+__rust_helper int
+rust_helper_atomic_dec_return_relaxed(atomic_t *v)
+{
+	return atomic_dec_return_relaxed(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_dec(atomic_t *v)
+{
+	return atomic_fetch_dec(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_dec_acquire(atomic_t *v)
+{
+	return atomic_fetch_dec_acquire(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_dec_release(atomic_t *v)
+{
+	return atomic_fetch_dec_release(v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_dec_relaxed(atomic_t *v)
+{
+	return atomic_fetch_dec_relaxed(v);
+}
+
+__rust_helper void
+rust_helper_atomic_and(int i, atomic_t *v)
+{
+	atomic_and(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_and(int i, atomic_t *v)
+{
+	return atomic_fetch_and(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_and_acquire(int i, atomic_t *v)
+{
+	return atomic_fetch_and_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_and_release(int i, atomic_t *v)
+{
+	return atomic_fetch_and_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_and_relaxed(int i, atomic_t *v)
+{
+	return atomic_fetch_and_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic_andnot(int i, atomic_t *v)
+{
+	atomic_andnot(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_andnot(int i, atomic_t *v)
+{
+	return atomic_fetch_andnot(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_andnot_acquire(int i, atomic_t *v)
+{
+	return atomic_fetch_andnot_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_andnot_release(int i, atomic_t *v)
+{
+	return atomic_fetch_andnot_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_andnot_relaxed(int i, atomic_t *v)
+{
+	return atomic_fetch_andnot_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic_or(int i, atomic_t *v)
+{
+	atomic_or(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_or(int i, atomic_t *v)
+{
+	return atomic_fetch_or(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_or_acquire(int i, atomic_t *v)
+{
+	return atomic_fetch_or_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_or_release(int i, atomic_t *v)
+{
+	return atomic_fetch_or_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_or_relaxed(int i, atomic_t *v)
+{
+	return atomic_fetch_or_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic_xor(int i, atomic_t *v)
+{
+	atomic_xor(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_xor(int i, atomic_t *v)
+{
+	return atomic_fetch_xor(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_xor_acquire(int i, atomic_t *v)
+{
+	return atomic_fetch_xor_acquire(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_xor_release(int i, atomic_t *v)
+{
+	return atomic_fetch_xor_release(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_xor_relaxed(int i, atomic_t *v)
+{
+	return atomic_fetch_xor_relaxed(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_xchg(atomic_t *v, int new)
+{
+	return atomic_xchg(v, new);
+}
+
+__rust_helper int
+rust_helper_atomic_xchg_acquire(atomic_t *v, int new)
+{
+	return atomic_xchg_acquire(v, new);
+}
+
+__rust_helper int
+rust_helper_atomic_xchg_release(atomic_t *v, int new)
+{
+	return atomic_xchg_release(v, new);
+}
+
+__rust_helper int
+rust_helper_atomic_xchg_relaxed(atomic_t *v, int new)
+{
+	return atomic_xchg_relaxed(v, new);
+}
+
+__rust_helper int
+rust_helper_atomic_cmpxchg(atomic_t *v, int old, int new)
+{
+	return atomic_cmpxchg(v, old, new);
+}
+
+__rust_helper int
+rust_helper_atomic_cmpxchg_acquire(atomic_t *v, int old, int new)
+{
+	return atomic_cmpxchg_acquire(v, old, new);
+}
+
+__rust_helper int
+rust_helper_atomic_cmpxchg_release(atomic_t *v, int old, int new)
+{
+	return atomic_cmpxchg_release(v, old, new);
+}
+
+__rust_helper int
+rust_helper_atomic_cmpxchg_relaxed(atomic_t *v, int old, int new)
+{
+	return atomic_cmpxchg_relaxed(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic_try_cmpxchg(atomic_t *v, int *old, int new)
+{
+	return atomic_try_cmpxchg(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic_try_cmpxchg_acquire(atomic_t *v, int *old, int new)
+{
+	return atomic_try_cmpxchg_acquire(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic_try_cmpxchg_release(atomic_t *v, int *old, int new)
+{
+	return atomic_try_cmpxchg_release(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic_try_cmpxchg_relaxed(atomic_t *v, int *old, int new)
+{
+	return atomic_try_cmpxchg_relaxed(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic_sub_and_test(int i, atomic_t *v)
+{
+	return atomic_sub_and_test(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic_dec_and_test(atomic_t *v)
+{
+	return atomic_dec_and_test(v);
+}
+
+__rust_helper bool
+rust_helper_atomic_inc_and_test(atomic_t *v)
+{
+	return atomic_inc_and_test(v);
+}
+
+__rust_helper bool
+rust_helper_atomic_add_negative(int i, atomic_t *v)
+{
+	return atomic_add_negative(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic_add_negative_acquire(int i, atomic_t *v)
+{
+	return atomic_add_negative_acquire(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic_add_negative_release(int i, atomic_t *v)
+{
+	return atomic_add_negative_release(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic_add_negative_relaxed(int i, atomic_t *v)
+{
+	return atomic_add_negative_relaxed(i, v);
+}
+
+__rust_helper int
+rust_helper_atomic_fetch_add_unless(atomic_t *v, int a, int u)
+{
+	return atomic_fetch_add_unless(v, a, u);
+}
+
+__rust_helper bool
+rust_helper_atomic_add_unless(atomic_t *v, int a, int u)
+{
+	return atomic_add_unless(v, a, u);
+}
+
+__rust_helper bool
+rust_helper_atomic_inc_not_zero(atomic_t *v)
+{
+	return atomic_inc_not_zero(v);
+}
+
+__rust_helper bool
+rust_helper_atomic_inc_unless_negative(atomic_t *v)
+{
+	return atomic_inc_unless_negative(v);
+}
+
+__rust_helper bool
+rust_helper_atomic_dec_unless_positive(atomic_t *v)
+{
+	return atomic_dec_unless_positive(v);
+}
+
+__rust_helper int
+rust_helper_atomic_dec_if_positive(atomic_t *v)
+{
+	return atomic_dec_if_positive(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_read(const atomic64_t *v)
+{
+	return atomic64_read(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_read_acquire(const atomic64_t *v)
+{
+	return atomic64_read_acquire(v);
+}
+
+__rust_helper void
+rust_helper_atomic64_set(atomic64_t *v, s64 i)
+{
+	atomic64_set(v, i);
+}
+
+__rust_helper void
+rust_helper_atomic64_set_release(atomic64_t *v, s64 i)
+{
+	atomic64_set_release(v, i);
+}
+
+__rust_helper void
+rust_helper_atomic64_add(s64 i, atomic64_t *v)
+{
+	atomic64_add(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_add_return(s64 i, atomic64_t *v)
+{
+	return atomic64_add_return(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_add_return_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_add_return_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_add_return_release(s64 i, atomic64_t *v)
+{
+	return atomic64_add_return_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_add_return_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_add_return_relaxed(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_add(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_add(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_add_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_add_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_add_release(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_add_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_add_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_add_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic64_sub(s64 i, atomic64_t *v)
+{
+	atomic64_sub(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_sub_return(s64 i, atomic64_t *v)
+{
+	return atomic64_sub_return(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_sub_return_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_sub_return_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_sub_return_release(s64 i, atomic64_t *v)
+{
+	return atomic64_sub_return_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_sub_return_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_sub_return_relaxed(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_sub(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_sub(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_sub_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_sub_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_sub_release(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_sub_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_sub_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_sub_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic64_inc(atomic64_t *v)
+{
+	atomic64_inc(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_inc_return(atomic64_t *v)
+{
+	return atomic64_inc_return(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_inc_return_acquire(atomic64_t *v)
+{
+	return atomic64_inc_return_acquire(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_inc_return_release(atomic64_t *v)
+{
+	return atomic64_inc_return_release(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_inc_return_relaxed(atomic64_t *v)
+{
+	return atomic64_inc_return_relaxed(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_inc(atomic64_t *v)
+{
+	return atomic64_fetch_inc(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_inc_acquire(atomic64_t *v)
+{
+	return atomic64_fetch_inc_acquire(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_inc_release(atomic64_t *v)
+{
+	return atomic64_fetch_inc_release(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_inc_relaxed(atomic64_t *v)
+{
+	return atomic64_fetch_inc_relaxed(v);
+}
+
+__rust_helper void
+rust_helper_atomic64_dec(atomic64_t *v)
+{
+	atomic64_dec(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_dec_return(atomic64_t *v)
+{
+	return atomic64_dec_return(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_dec_return_acquire(atomic64_t *v)
+{
+	return atomic64_dec_return_acquire(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_dec_return_release(atomic64_t *v)
+{
+	return atomic64_dec_return_release(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_dec_return_relaxed(atomic64_t *v)
+{
+	return atomic64_dec_return_relaxed(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_dec(atomic64_t *v)
+{
+	return atomic64_fetch_dec(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_dec_acquire(atomic64_t *v)
+{
+	return atomic64_fetch_dec_acquire(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_dec_release(atomic64_t *v)
+{
+	return atomic64_fetch_dec_release(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_dec_relaxed(atomic64_t *v)
+{
+	return atomic64_fetch_dec_relaxed(v);
+}
+
+__rust_helper void
+rust_helper_atomic64_and(s64 i, atomic64_t *v)
+{
+	atomic64_and(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_and(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_and(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_and_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_and_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_and_release(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_and_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_and_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_and_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic64_andnot(s64 i, atomic64_t *v)
+{
+	atomic64_andnot(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_andnot(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_andnot(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_andnot_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_andnot_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_andnot_release(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_andnot_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_andnot_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_andnot_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic64_or(s64 i, atomic64_t *v)
+{
+	atomic64_or(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_or(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_or(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_or_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_or_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_or_release(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_or_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_or_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_or_relaxed(i, v);
+}
+
+__rust_helper void
+rust_helper_atomic64_xor(s64 i, atomic64_t *v)
+{
+	atomic64_xor(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_xor(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_xor(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_xor_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_xor_acquire(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_xor_release(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_xor_release(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_xor_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_fetch_xor_relaxed(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_xchg(atomic64_t *v, s64 new)
+{
+	return atomic64_xchg(v, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_xchg_acquire(atomic64_t *v, s64 new)
+{
+	return atomic64_xchg_acquire(v, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_xchg_release(atomic64_t *v, s64 new)
+{
+	return atomic64_xchg_release(v, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_xchg_relaxed(atomic64_t *v, s64 new)
+{
+	return atomic64_xchg_relaxed(v, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_cmpxchg(atomic64_t *v, s64 old, s64 new)
+{
+	return atomic64_cmpxchg(v, old, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_cmpxchg_acquire(atomic64_t *v, s64 old, s64 new)
+{
+	return atomic64_cmpxchg_acquire(v, old, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_cmpxchg_release(atomic64_t *v, s64 old, s64 new)
+{
+	return atomic64_cmpxchg_release(v, old, new);
+}
+
+__rust_helper s64
+rust_helper_atomic64_cmpxchg_relaxed(atomic64_t *v, s64 old, s64 new)
+{
+	return atomic64_cmpxchg_relaxed(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic64_try_cmpxchg(atomic64_t *v, s64 *old, s64 new)
+{
+	return atomic64_try_cmpxchg(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic64_try_cmpxchg_acquire(atomic64_t *v, s64 *old, s64 new)
+{
+	return atomic64_try_cmpxchg_acquire(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic64_try_cmpxchg_release(atomic64_t *v, s64 *old, s64 new)
+{
+	return atomic64_try_cmpxchg_release(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic64_try_cmpxchg_relaxed(atomic64_t *v, s64 *old, s64 new)
+{
+	return atomic64_try_cmpxchg_relaxed(v, old, new);
+}
+
+__rust_helper bool
+rust_helper_atomic64_sub_and_test(s64 i, atomic64_t *v)
+{
+	return atomic64_sub_and_test(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_dec_and_test(atomic64_t *v)
+{
+	return atomic64_dec_and_test(v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_inc_and_test(atomic64_t *v)
+{
+	return atomic64_inc_and_test(v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_add_negative(s64 i, atomic64_t *v)
+{
+	return atomic64_add_negative(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_add_negative_acquire(s64 i, atomic64_t *v)
+{
+	return atomic64_add_negative_acquire(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_add_negative_release(s64 i, atomic64_t *v)
+{
+	return atomic64_add_negative_release(i, v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_add_negative_relaxed(s64 i, atomic64_t *v)
+{
+	return atomic64_add_negative_relaxed(i, v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_fetch_add_unless(atomic64_t *v, s64 a, s64 u)
+{
+	return atomic64_fetch_add_unless(v, a, u);
+}
+
+__rust_helper bool
+rust_helper_atomic64_add_unless(atomic64_t *v, s64 a, s64 u)
+{
+	return atomic64_add_unless(v, a, u);
+}
+
+__rust_helper bool
+rust_helper_atomic64_inc_not_zero(atomic64_t *v)
+{
+	return atomic64_inc_not_zero(v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_inc_unless_negative(atomic64_t *v)
+{
+	return atomic64_inc_unless_negative(v);
+}
+
+__rust_helper bool
+rust_helper_atomic64_dec_unless_positive(atomic64_t *v)
+{
+	return atomic64_dec_unless_positive(v);
+}
+
+__rust_helper s64
+rust_helper_atomic64_dec_if_positive(atomic64_t *v)
+{
+	return atomic64_dec_if_positive(v);
+}
+
+#endif /* _RUST_ATOMIC_API_H */
+// 615a0e0c98b5973a47fe4fa65e92935051ca00ed
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 16fa9bca5949..83e89f6a68fb 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -7,6 +7,7 @@
  * Sorted alphabetically.
  */
 
+#include "atomic.c"
 #include "auxiliary.c"
 #include "blk.c"
 #include "bug.c"
diff --git a/scripts/atomic/gen-atomics.sh b/scripts/atomic/gen-atomics.sh
index 5b98a8307693..02508d0d6fe4 100755
--- a/scripts/atomic/gen-atomics.sh
+++ b/scripts/atomic/gen-atomics.sh
@@ -11,6 +11,7 @@ cat <<EOF |
 gen-atomic-instrumented.sh      linux/atomic/atomic-instrumented.h
 gen-atomic-long.sh              linux/atomic/atomic-long.h
 gen-atomic-fallback.sh          linux/atomic/atomic-arch-fallback.h
+gen-rust-atomic-helpers.sh      ../rust/helpers/atomic.c
 EOF
 while read script header args; do
 	/bin/sh ${ATOMICDIR}/${script} ${ATOMICTBL} ${args} > ${LINUXDIR}/include/${header}
diff --git a/scripts/atomic/gen-rust-atomic-helpers.sh b/scripts/atomic/gen-rust-atomic-helpers.sh
new file mode 100755
index 000000000000..45b1e100ed7c
--- /dev/null
+++ b/scripts/atomic/gen-rust-atomic-helpers.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+ATOMICDIR=$(dirname $0)
+
+. ${ATOMICDIR}/atomic-tbl.sh
+
+#gen_proto_order_variant(meta, pfx, name, sfx, order, atomic, int, arg...)
+gen_proto_order_variant()
+{
+	local meta="$1"; shift
+	local pfx="$1"; shift
+	local name="$1"; shift
+	local sfx="$1"; shift
+	local order="$1"; shift
+	local atomic="$1"; shift
+	local int="$1"; shift
+
+	local atomicname="${atomic}_${pfx}${name}${sfx}${order}"
+
+	local ret="$(gen_ret_type "${meta}" "${int}")"
+	local params="$(gen_params "${int}" "${atomic}" "$@")"
+	local args="$(gen_args "$@")"
+	local retstmt="$(gen_ret_stmt "${meta}")"
+
+cat <<EOF
+__rust_helper ${ret}
+rust_helper_${atomicname}(${params})
+{
+	${retstmt}${atomicname}(${args});
+}
+
+EOF
+}
+
+cat << EOF
+// SPDX-License-Identifier: GPL-2.0
+
+// Generated by $0
+// DO NOT MODIFY THIS FILE DIRECTLY
+
+/*
+ * This file provides helpers for the various atomic functions for Rust.
+ */
+#ifndef _RUST_ATOMIC_API_H
+#define _RUST_ATOMIC_API_H
+
+#include <linux/atomic.h>
+
+// TODO: Remove this after INLINE_HELPERS support is added.
+#ifndef __rust_helper
+#define __rust_helper
+#endif
+
+EOF
+
+grep '^[a-z]' "$1" | while read name meta args; do
+	gen_proto "${meta}" "${name}" "atomic" "int" ${args}
+done
+
+grep '^[a-z]' "$1" | while read name meta args; do
+	gen_proto "${meta}" "${name}" "atomic64" "s64" ${args}
+done
+
+cat <<EOF
+#endif /* _RUST_ATOMIC_API_H */
+EOF
-- 
2.39.5 (Apple Git-154)

[PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Boqun Feng]

Preparation for generic atomic implementation. To unify the
implementation of a generic method over `i32` and `i64`, the C side
atomic methods need to be grouped so that in a generic method, they can
be referred as <type>::<method>, otherwise their parameters and return
value are different between `i32` and `i64`, which would require using
`transmute()` to unify the type into a `T`.

Introduce `AtomicImpl` to represent a basic type in Rust that has the
direct mapping to an atomic implementation from C. This trait is sealed,
and currently only `i32` and `i64` impl this.

Further, different methods are put into different `*Ops` trait groups,
and this is for the future when smaller types like `i8`/`i16` are
supported but only with a limited set of API (e.g. only set(), load(),
xchg() and cmpxchg(), no add() or sub() etc).

While the atomic mod is introduced, documentation is also added for
memory models and data races.

Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect
my responsiblity on the Rust atomic mod.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 MAINTAINERS                    |   4 +-
 rust/kernel/sync.rs            |   1 +
 rust/kernel/sync/atomic.rs     |  19 ++++
 rust/kernel/sync/atomic/ops.rs | 195 +++++++++++++++++++++++++++++++++
 4 files changed, 218 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/sync/atomic.rs
 create mode 100644 rust/kernel/sync/atomic/ops.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 0c1d245bf7b8..5eef524975ca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3894,7 +3894,7 @@ F:	drivers/input/touchscreen/atmel_mxt_ts.c
 ATOMIC INFRASTRUCTURE
 M:	Will Deacon <will@kernel.org>
 M:	Peter Zijlstra <peterz@infradead.org>
-R:	Boqun Feng <boqun.feng@gmail.com>
+M:	Boqun Feng <boqun.feng@gmail.com>
 R:	Mark Rutland <mark.rutland@arm.com>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
@@ -3903,6 +3903,8 @@ F:	arch/*/include/asm/atomic*.h
 F:	include/*/atomic*.h
 F:	include/linux/refcount.h
 F:	scripts/atomic/
+F:	rust/kernel/sync/atomic.rs
+F:	rust/kernel/sync/atomic/
 
 ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER
 M:	Bradley Grove <linuxdrivers@attotech.com>
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 36a719015583..b620027e0641 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -10,6 +10,7 @@
 use pin_init;
 
 mod arc;
+pub mod atomic;
 mod condvar;
 pub mod lock;
 mod locked_by;
diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
new file mode 100644
index 000000000000..c9c7c3617dd5
--- /dev/null
+++ b/rust/kernel/sync/atomic.rs
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Atomic primitives.
+//!
+//! These primitives have the same semantics as their C counterparts: and the precise definitions of
+//! semantics can be found at [`LKMM`]. Note that Linux Kernel Memory (Consistency) Model is the
+//! only model for Rust code in kernel, and Rust's own atomics should be avoided.
+//!
+//! # Data races
+//!
+//! [`LKMM`] atomics have different rules regarding data races:
+//!
+//! - A normal write from C side is treated as an atomic write if
+//!   CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y.
+//! - Mixed-size atomic accesses don't cause data races.
+//!
+//! [`LKMM`]: srctree/tools/memory-model/
+
+pub mod ops;
diff --git a/rust/kernel/sync/atomic/ops.rs b/rust/kernel/sync/atomic/ops.rs
new file mode 100644
index 000000000000..da04dd383962
--- /dev/null
+++ b/rust/kernel/sync/atomic/ops.rs
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Atomic implementations.
+//!
+//! Provides 1:1 mapping of atomic implementations.
+
+use crate::bindings::*;
+use crate::macros::paste;
+
+mod private {
+    /// Sealed trait marker to disable customized impls on atomic implementation traits.
+    pub trait Sealed {}
+}
+
+// `i32` and `i64` are only supported atomic implementations.
+impl private::Sealed for i32 {}
+impl private::Sealed for i64 {}
+
+/// A marker trait for types that implement atomic operations with C side primitives.
+///
+/// This trait is sealed, and only types that have directly mapping to the C side atomics should
+/// impl this:
+///
+/// - `i32` maps to `atomic_t`.
+/// - `i64` maps to `atomic64_t`.
+pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {}
+
+// `atomic_t` implements atomic operations on `i32`.
+impl AtomicImpl for i32 {}
+
+// `atomic64_t` implements atomic operations on `i64`.
+impl AtomicImpl for i64 {}
+
+// This macro generates the function signature with given argument list and return type.
+macro_rules! declare_atomic_method {
+    (
+        $func:ident($($arg:ident : $arg_type:ty),*) $(-> $ret:ty)?
+    ) => {
+        paste!(
+            #[doc = concat!("Atomic ", stringify!($func))]
+            #[doc = "# Safety"]
+            #[doc = "- Any pointer passed to the function has to be a valid pointer"]
+            #[doc = "- Accesses must not cause data races per LKMM:"]
+            #[doc = "  - Atomic read racing with normal read, normal write or atomic write is not data race."]
+            #[doc = "  - Atomic write racing with normal read or normal write is data-race, unless the"]
+            #[doc = "    normal accesses are done at C side and considered as immune to data"]
+            #[doc = "    races, e.g. CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC."]
+            unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)?;
+        );
+    };
+    (
+        $func:ident [$variant:ident $($rest:ident)*]($($arg_sig:tt)*) $(-> $ret:ty)?
+    ) => {
+        paste!(
+            declare_atomic_method!(
+                [< $func _ $variant >]($($arg_sig)*) $(-> $ret)?
+            );
+        );
+
+        declare_atomic_method!(
+            $func [$($rest)*]($($arg_sig)*) $(-> $ret)?
+        );
+    };
+    (
+        $func:ident []($($arg_sig:tt)*) $(-> $ret:ty)?
+    ) => {
+        declare_atomic_method!(
+            $func($($arg_sig)*) $(-> $ret)?
+        );
+    }
+}
+
+// This macro generates the function implementation with given argument list and return type, and it
+// will replace "call(...)" expression with "$ctype _ $func" to call the real C function.
+macro_rules! impl_atomic_method {
+    (
+        ($ctype:ident) $func:ident($($arg:ident: $arg_type:ty),*) $(-> $ret:ty)? {
+            call($($c_arg:expr),*)
+        }
+    ) => {
+        paste!(
+            #[inline(always)]
+            unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)? {
+                // SAFETY: Per function safety requirement, all pointers are valid, and accesses
+                // won't cause data race per LKMM.
+                unsafe { [< $ctype _ $func >]($($c_arg,)*) }
+            }
+        );
+    };
+    (
+        ($ctype:ident) $func:ident[$variant:ident $($rest:ident)*]($($arg_sig:tt)*) $(-> $ret:ty)? {
+            call($($arg:tt)*)
+        }
+    ) => {
+        paste!(
+            impl_atomic_method!(
+                ($ctype) [< $func _ $variant >]($($arg_sig)*) $( -> $ret)? {
+                    call($($arg)*)
+            }
+            );
+        );
+        impl_atomic_method!(
+            ($ctype) $func [$($rest)*]($($arg_sig)*) $( -> $ret)? {
+                call($($arg)*)
+            }
+        );
+    };
+    (
+        ($ctype:ident) $func:ident[]($($arg_sig:tt)*) $( -> $ret:ty)? {
+            call($($arg:tt)*)
+        }
+    ) => {
+        impl_atomic_method!(
+            ($ctype) $func($($arg_sig)*) $(-> $ret)? {
+                call($($arg)*)
+            }
+        );
+    }
+}
+
+// Delcares $ops trait with methods and implements the trait for `i32` and `i64`.
+macro_rules! declare_and_impl_atomic_methods {
+    ($ops:ident ($doc:literal) {
+        $(
+            $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
+                call($($arg:tt)*)
+            }
+        )*
+    }) => {
+        #[doc = $doc]
+        pub trait $ops: AtomicImpl {
+            $(
+                declare_atomic_method!(
+                    $func[$($variant)*]($($arg_sig)*) $(-> $ret)?
+                );
+            )*
+        }
+
+        impl $ops for i32 {
+            $(
+                impl_atomic_method!(
+                    (atomic) $func[$($variant)*]($($arg_sig)*) $(-> $ret)? {
+                        call($($arg)*)
+                    }
+                );
+            )*
+        }
+
+        impl $ops for i64 {
+            $(
+                impl_atomic_method!(
+                    (atomic64) $func[$($variant)*]($($arg_sig)*) $(-> $ret)? {
+                        call($($arg)*)
+                    }
+                );
+            )*
+        }
+    }
+}
+
+declare_and_impl_atomic_methods!(
+    AtomicHasBasicOps ("Basic atomic operations") {
+        read[acquire](ptr: *mut Self) -> Self {
+            call(ptr.cast())
+        }
+
+        set[release](ptr: *mut Self, v: Self) {
+            call(ptr.cast(), v)
+        }
+    }
+);
+
+declare_and_impl_atomic_methods!(
+    AtomicHasXchgOps ("Exchange and compare-and-exchange atomic operations") {
+        xchg[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
+            call(ptr.cast(), v)
+        }
+
+        try_cmpxchg[acquire, release, relaxed](ptr: *mut Self, old: *mut Self, new: Self) -> bool {
+            call(ptr.cast(), old, new)
+        }
+    }
+);
+
+declare_and_impl_atomic_methods!(
+    AtomicHasArithmeticOps ("Atomic arithmetic operations") {
+        add[](ptr: *mut Self, v: Self) {
+            call(v, ptr.cast())
+        }
+
+        fetch_add[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
+            call(v, ptr.cast())
+        }
+    }
+);
-- 
2.39.5 (Apple Git-154)

[PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Boqun Feng]

Preparation for atomic primitives. Instead of a suffix like _acquire, a
method parameter along with the corresponding generic parameter will be
used to specify the ordering of an atomic operations. For example,
atomic load() can be defined as:

	impl<T: ...> Atomic<T> {
	    pub fn load<O: AcquireOrRelaxed>(&self, _o: O) -> T { ... }
	}

and acquire users would do:

	let r = x.load(Acquire);

relaxed users:

	let r = x.load(Relaxed);

doing the following:

	let r = x.load(Release);

will cause a compiler error.

Compared to suffixes, it's easier to tell what ordering variants an
operation has, and it also make it easier to unify the implementation of
all ordering variants in one method via generic. The `TYPE` associate
const is for generic function to pick up the particular implementation
specified by an ordering annotation.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs          |  3 +
 rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)
 create mode 100644 rust/kernel/sync/atomic/ordering.rs

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index c9c7c3617dd5..e80ac049f36b 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -17,3 +17,6 @@
 //! [`LKMM`]: srctree/tools/memory-model/
 
 pub mod ops;
+pub mod ordering;
+
+pub use ordering::{Acquire, Full, Relaxed, Release};
diff --git a/rust/kernel/sync/atomic/ordering.rs b/rust/kernel/sync/atomic/ordering.rs
new file mode 100644
index 000000000000..5fffbaa2fa6d
--- /dev/null
+++ b/rust/kernel/sync/atomic/ordering.rs
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Memory orderings.
+//!
+//! The semantics of these orderings follows the [`LKMM`] definitions and rules.
+//!
+//! - [`Acquire`] provides ordering between the load part of the annotated operation and all the
+//!   following memory accesses, and if there is a store part, the store part has the [`Relaxed`]
+//!   ordering.
+//! - [`Release`] provides ordering between all the preceding memory accesses and the store part of
+//!   the annotated operation, and if there is a load part, the load part has the [`Relaxed`]
+//!   ordering.
+//! - [`Full`] means "fully-ordered", that is:
+//!   - It provides ordering between all the preceding memory accesses and the annotated operation.
+//!   - It provides ordering between the annotated operation and all the following memory accesses.
+//!   - It provides ordering between all the preceding memory accesses and all the fllowing memory
+//!     accesses.
+//!   - All the orderings are the same strength as a full memory barrier (i.e. `smp_mb()`).
+//! - [`Relaxed`] provides no ordering except the dependency orderings. Dependency orderings are
+//!   described in "DEPENDENCY RELATIONS" in [`LKMM`]'s [`explanation`].
+//!
+//! [`LKMM`]: srctree/tools/memory-model/
+//! [`explanation`]: srctree/tools/memory-model/Documentation/explanation.txt
+
+/// The annotation type for relaxed memory ordering.
+pub struct Relaxed;
+
+/// The annotation type for acquire memory ordering.
+pub struct Acquire;
+
+/// The annotation type for release memory ordering.
+pub struct Release;
+
+/// The annotation type for fully-order memory ordering.
+pub struct Full;
+
+/// Describes the exact memory ordering.
+#[doc(hidden)]
+pub enum OrderingType {
+    /// Relaxed ordering.
+    Relaxed,
+    /// Acquire ordering.
+    Acquire,
+    /// Release ordering.
+    Release,
+    /// Fully-ordered.
+    Full,
+}
+
+mod internal {
+    /// Sealed trait, can be only implemented inside atomic mod.
+    pub trait Sealed {}
+
+    impl Sealed for super::Relaxed {}
+    impl Sealed for super::Acquire {}
+    impl Sealed for super::Release {}
+    impl Sealed for super::Full {}
+}
+
+/// The trait bound for annotating operations that support any ordering.
+pub trait Any: internal::Sealed {
+    /// Describes the exact memory ordering.
+    const TYPE: OrderingType;
+}
+
+impl Any for Relaxed {
+    const TYPE: OrderingType = OrderingType::Relaxed;
+}
+
+impl Any for Acquire {
+    const TYPE: OrderingType = OrderingType::Acquire;
+}
+
+impl Any for Release {
+    const TYPE: OrderingType = OrderingType::Release;
+}
+
+impl Any for Full {
+    const TYPE: OrderingType = OrderingType::Full;
+}
+
+/// The trait bound for operations that only support acquire or relaxed ordering.
+pub trait AcquireOrRelaxed: Any {}
+
+impl AcquireOrRelaxed for Acquire {}
+impl AcquireOrRelaxed for Relaxed {}
+
+/// The trait bound for operations that only support release or relaxed ordering.
+pub trait ReleaseOrRelaxed: Any {}
+
+impl ReleaseOrRelaxed for Release {}
+impl ReleaseOrRelaxed for Relaxed {}
+
+/// The trait bound for operations that only support relaxed ordering.
+pub trait RelaxedOnly: AcquireOrRelaxed + ReleaseOrRelaxed + Any {}
+
+impl RelaxedOnly for Relaxed {}
-- 
2.39.5 (Apple Git-154)

[PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

To provide using LKMM atomics for Rust code, a generic `Atomic<T>` is
added, currently `T` needs to be Send + Copy because these are the
straightforward usages and all basic types support this.

Implement `AllowAtomic` for `i32` and `i64`, and so far only basic
operations load() and store() are introduced.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs         |  14 ++
 rust/kernel/sync/atomic/generic.rs | 289 +++++++++++++++++++++++++++++
 2 files changed, 303 insertions(+)
 create mode 100644 rust/kernel/sync/atomic/generic.rs

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index e80ac049f36b..c5193c1c90fe 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -16,7 +16,21 @@
 //!
 //! [`LKMM`]: srctree/tools/memory-model/
 
+pub mod generic;
 pub mod ops;
 pub mod ordering;
 
+pub use generic::Atomic;
 pub use ordering::{Acquire, Full, Relaxed, Release};
+
+// SAFETY: `i32` has the same size and alignment with itself, and is round-trip transmutable to
+// itself.
+unsafe impl generic::AllowAtomic for i32 {
+    type Repr = i32;
+}
+
+// SAFETY: `i64` has the same size and alignment with itself, and is round-trip transmutable to
+// itself.
+unsafe impl generic::AllowAtomic for i64 {
+    type Repr = i64;
+}
diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
new file mode 100644
index 000000000000..e044fe21b128
--- /dev/null
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic atomic primitives.
+
+use super::ops::*;
+use super::ordering::*;
+use crate::build_error;
+use core::cell::UnsafeCell;
+
+/// A generic atomic variable.
+///
+/// `T` must impl [`AllowAtomic`], that is, an [`AtomicImpl`] has to be chosen.
+///
+/// # Examples
+///
+/// A customized type stored in [`Atomic`]:
+///
+/// ```rust
+/// use kernel::sync::atomic::{generic::AllowAtomic, Atomic, Relaxed};
+///
+/// #[derive(Clone, Copy, PartialEq, Eq)]
+/// #[repr(i32)]
+/// enum State {
+///     Uninit = 0,
+///     Working = 1,
+///     Done = 2,
+/// };
+///
+/// // SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip
+/// // transmutable to `i32`.
+/// unsafe impl AllowAtomic for State {
+///     type Repr = i32;
+/// }
+///
+/// let s = Atomic::new(State::Uninit);
+///
+/// assert_eq!(State::Uninit, s.load(Relaxed));
+/// ```
+///
+/// # Guarantees
+///
+/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race,
+/// this is guaranteed by the safety requirement of [`Self::from_ptr()`] and the extra safety
+/// requirement of the usage on pointers returned by [`Self::as_ptr()`].
+#[repr(transparent)]
+pub struct Atomic<T: AllowAtomic>(UnsafeCell<T>);
+
+// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
+unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
+
+/// Types that support basic atomic operations.
+///
+/// # Round-trip transmutability
+///
+/// Implementing [`AllowAtomic`] requires that the type is round-trip transmutable to its
+/// representation:
+///
+/// - Any value of [`Self`] must be sound to [`transmute()`] to a [`Self::Repr`], and this also
+///   means that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`] for reading.
+/// - If a value of [`Self::Repr`] is a result a [`transmute()`] from a [`Self`], it must be
+///   sound to [`transmute()`] the value back to a [`Self`].
+///
+/// This essentially means a valid bit pattern of `T: AllowAtomic` has to be a valid bit pattern
+/// of `T::Repr`. This is needed because [`Atomic<T: AllowAtomic>`] operates on `T::Repr` to
+/// implement atomic operations on `T`.
+///
+/// Note that this is more relaxed than bidirectional transmutability (i.e. [`transmute()`] is
+/// always sound between `T` and `T::Repr`) because of the support for atomic variables over
+/// unit-only enums:
+///
+/// ```
+/// #[repr(i32)]
+/// enum State { Init = 0, Working = 1, Done = 2, }
+/// ```
+///
+/// # Safety
+///
+/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
+/// - [`Self`] and [`Self::Repr`] must have the [round-trip transmutability].
+///
+/// # Limitations
+///
+/// Because C primitives are used to implement the atomic operations, and a C function requires a
+/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
+/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,
+/// u16)` (a tuple with a `MaybeUninit` hole in it) are currently not supported. Note that
+/// technically these types can be supported if some APIs are removed for them and the inner
+/// implementation is tweaked, but the justification of support such a type is not strong enough at
+/// the moment. This should be resolved if there is an implementation for `MaybeUninit<i32>` as
+/// `AtomicImpl`.
+///
+/// [`transmute()`]: core::mem::transmute
+/// [round-trip transmutability]: AllowAtomic#round-trip-transmutability
+pub unsafe trait AllowAtomic: Sized + Send + Copy {
+    /// The backing atomic implementation type.
+    type Repr: AtomicImpl;
+}
+
+#[inline(always)]
+const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
+    // SAFETY: Per the safety requirement of `AllowAtomic`, the transmute operation is sound.
+    unsafe { core::mem::transmute_copy(&v) }
+}
+
+/// # Safety
+///
+/// `r` must be a valid bit pattern of `T`.
+#[inline(always)]
+const unsafe fn from_repr<T: AllowAtomic>(r: T::Repr) -> T {
+    // SAFETY: Per the safety requirement of the function, the transmute operation is sound.
+    unsafe { core::mem::transmute_copy(&r) }
+}
+
+impl<T: AllowAtomic> Atomic<T> {
+    /// Creates a new atomic.
+    pub const fn new(v: T) -> Self {
+        Self(UnsafeCell::new(v))
+    }
+
+    /// Creates a reference to [`Self`] from a pointer.
+    ///
+    /// # Safety
+    ///
+    /// - `ptr` has to be a valid pointer.
+    /// - `ptr` has to be valid for both reads and writes for the whole lifetime `'a`.
+    /// - For the duration of `'a`, other accesses to the object cannot cause data races (defined
+    ///   by [`LKMM`]) against atomic operations on the returned reference. Note that if all other
+    ///   accesses are atomic, then this safety requirement is trivially fulfilled.
+    ///
+    /// [`LKMM`]: srctree/tools/memory-model
+    ///
+    /// # Examples
+    ///
+    /// Using [`Atomic::from_ptr()`] combined with [`Atomic::load()`] or [`Atomic::store()`] can
+    /// achieve the same functionality as `READ_ONCE()`/`smp_load_acquire()` or
+    /// `WRITE_ONCE()`/`smp_store_release()` in C side:
+    ///
+    /// ```rust
+    /// # use kernel::types::Opaque;
+    /// use kernel::sync::atomic::{Atomic, Relaxed, Release};
+    ///
+    /// // Assume there is a C struct `Foo`.
+    /// mod cbindings {
+    ///     #[repr(C)]
+    ///     pub(crate) struct foo { pub(crate) a: i32, pub(crate) b: i32 }
+    /// }
+    ///
+    /// let tmp = Opaque::new(cbindings::foo { a: 1, b: 2});
+    ///
+    /// // struct foo *foo_ptr = ..;
+    /// let foo_ptr = tmp.get();
+    ///
+    /// // SAFETY: `foo_ptr` is a valid pointer, and `.a` is in bounds.
+    /// let foo_a_ptr = unsafe { &raw mut (*foo_ptr).a };
+    ///
+    /// // a = READ_ONCE(foo_ptr->a);
+    /// //
+    /// // SAFETY: `foo_a_ptr` is a valid pointer for read, and all accesses on it is atomic, so no
+    /// // data race.
+    /// let a = unsafe { Atomic::from_ptr(foo_a_ptr) }.load(Relaxed);
+    /// # assert_eq!(a, 1);
+    ///
+    /// // smp_store_release(&foo_ptr->a, 2);
+    /// //
+    /// // SAFETY: `foo_a_ptr` is a valid pointer for write, and all accesses on it is atomic, so
+    /// // no data race.
+    /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
+    /// ```
+    ///
+    /// However, this should be only used when communicating with C side or manipulating a C struct.
+    pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
+    where
+        T: Sync,
+    {
+        // CAST: `T` is transparent to `Atomic<T>`.
+        // SAFETY: Per function safety requirement, `ptr` is a valid pointer and the object will
+        // live long enough. It's safe to return a `&Atomic<T>` because function safety requirement
+        // guarantees other accesses won't cause data races.
+        unsafe { &*ptr.cast::<Self>() }
+    }
+
+    /// Returns a pointer to the underlying atomic variable.
+    ///
+    /// Extra safety requirement on using the return pointer: the operations done via the pointer
+    /// cannot cause data races defined by [`LKMM`].
+    ///
+    /// [`LKMM`]: srctree/tools/memory-model
+    pub const fn as_ptr(&self) -> *mut T {
+        self.0.get()
+    }
+
+    /// Returns a mutable reference to the underlying atomic variable.
+    ///
+    /// This is safe because the mutable reference of the atomic variable guarantees the exclusive
+    /// access.
+    pub fn get_mut(&mut self) -> &mut T {
+        // SAFETY: `self.as_ptr()` is a valid pointer to `T`. `&mut self` guarantees the exclusive
+        // access, so it's safe to reborrow mutably.
+        unsafe { &mut *self.as_ptr() }
+    }
+}
+
+impl<T: AllowAtomic> Atomic<T>
+where
+    T::Repr: AtomicHasBasicOps,
+{
+    /// Loads the value from the atomic variable.
+    ///
+    /// # Examples
+    ///
+    /// Simple usages:
+    ///
+    /// ```rust
+    /// use kernel::sync::atomic::{Atomic, Relaxed};
+    ///
+    /// let x = Atomic::new(42i32);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// let x = Atomic::new(42i64);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    /// ```
+    #[doc(alias("atomic_read", "atomic64_read"))]
+    #[inline(always)]
+    pub fn load<Ordering: AcquireOrRelaxed>(&self, _: Ordering) -> T {
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_read*() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, 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 v = unsafe {
+            match Ordering::TYPE {
+                OrderingType::Relaxed => T::Repr::atomic_read(a),
+                OrderingType::Acquire => T::Repr::atomic_read_acquire(a),
+                _ => build_error!("Wrong ordering"),
+            }
+        };
+
+        // SAFETY: The atomic variable holds a valid `T`, so `v` is a valid bit pattern of `T`,
+        // therefore it's safe to call `from_repr()`.
+        unsafe { from_repr(v) }
+    }
+
+    /// Stores a value to the atomic variable.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::sync::atomic::{Atomic, Relaxed};
+    ///
+    /// let x = Atomic::new(42i32);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// x.store(43, Relaxed);
+    ///
+    /// assert_eq!(43, x.load(Relaxed));
+    /// ```
+    #[doc(alias("atomic_set", "atomic64_set"))]
+    #[inline(always)]
+    pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
+        let v = into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_set*() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, 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.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `v` is a valid bit pattern of `T`, so it's sound to store it in an `Atomic<T>`.
+        unsafe {
+            match Ordering::TYPE {
+                OrderingType::Relaxed => T::Repr::atomic_set(a, v),
+                OrderingType::Release => T::Repr::atomic_set_release(a, v),
+                _ => build_error!("Wrong ordering"),
+            }
+        };
+    }
+}
-- 
2.39.5 (Apple Git-154)

[PATCH v6 5/9] rust: sync: atomic: Add atomic {cmp,}xchg operations

[Boqun Feng]

xchg() and cmpxchg() are basic operations on atomic. Provide these based
on C APIs.

Note that cmpxchg() use the similar function signature as
compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means
the operation succeeds and `Err(old)` means the operation fails.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic/generic.rs | 170 +++++++++++++++++++++++++++++
 1 file changed, 170 insertions(+)

diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
index e044fe21b128..1beb802843ee 100644
--- a/rust/kernel/sync/atomic/generic.rs
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -287,3 +287,173 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
         };
     }
 }
+
+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", "swap"))]
+    #[inline(always)]
+    pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T {
+        let v = into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_xchg*() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, 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.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `v` is a valid bit pattern of `T`, so it's sound to store it in an `Atomic<T>`.
+        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),
+            }
+        };
+
+        // SAFETY: The atomic variable holds a valid `T`, so `ret` is a valid bit pattern of `T`,
+        // therefore it's safe to call `from_repr()`.
+        unsafe { 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.
+    ///
+    /// Returns `Ok(value)` if cmpxchg succeeds, and `value` is guaranteed to be equal to `old`,
+    /// otherwise returns `Err(value)`, and `value` is the value of the atomic variable when
+    /// cmpxchg was happening.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::sync::atomic::{Atomic, Full, Relaxed};
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// // Checks whether cmpxchg succeeded.
+    /// let success = x.cmpxchg(52, 64, Relaxed).is_ok();
+    /// # assert!(!success);
+    ///
+    /// // Checks whether cmpxchg failed.
+    /// let failure = x.cmpxchg(52, 64, Relaxed).is_err();
+    /// # assert!(failure);
+    ///
+    /// // Uses the old value if failed, probably re-try cmpxchg.
+    /// match x.cmpxchg(52, 64, Relaxed) {
+    ///     Ok(_) => { },
+    ///     Err(old) => {
+    ///         // do something with `old`.
+    ///         # assert_eq!(old, 42);
+    ///     }
+    /// }
+    ///
+    /// // Uses the latest value regardlessly, same as atomic_cmpxchg() in C.
+    /// let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old);
+    /// # assert_eq!(42, latest);
+    /// assert_eq!(64, x.load(Relaxed));
+    /// ```
+    #[doc(alias(
+        "atomic_cmpxchg",
+        "atomic64_cmpxchg",
+        "atomic_try_cmpxchg",
+        "atomic64_try_cmpxchg",
+        "compare_exchange"
+    ))]
+    #[inline(always)]
+    pub fn cmpxchg<Ordering: Any>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> {
+        // Note on code generation:
+        //
+        // try_cmpxchg() is used to implement cmpxchg(), and if the helper functions are inlined,
+        // the compiler is able to figure out that branch is not needed if the users don't care
+        // about whether the operation succeeds or not. One exception is on x86, due to commit
+        // 44fe84459faf ("locking/atomic: Fix atomic_try_cmpxchg() semantics"), the
+        // atomic_try_cmpxchg() on x86 has a branch even if the caller doesn't care about the
+        // success of cmpxchg and only wants to use the old value. For example, for code like:
+        //
+        //     let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old);
+        //
+        // It will still generate code:
+        //
+        //     movl    $0x40, %ecx
+        //     movl    $0x34, %eax
+        //     lock
+        //     cmpxchgl        %ecx, 0x4(%rsp)
+        //     jne     1f
+        //     2:
+        //     ...
+        //     1:  movl    %eax, %ecx
+        //     jmp 2b
+        //
+        // This might be "fixed" by introducing a try_cmpxchg_exclusive() that knows the "*old"
+        // location in the C function is always safe to write.
+        if self.try_cmpxchg(&mut old, new, o) {
+            Ok(old)
+        } else {
+            Err(old)
+        }
+    }
+
+    /// Atomic compare and exchange and returns whether the operation succeeds.
+    ///
+    /// "Compare" and "Ordering" part are the same as [`Atomic::cmpxchg()`].
+    ///
+    /// Returns `true` means the cmpxchg succeeds otherwise returns `false` with `old` updated to
+    /// the value of the atomic variable when cmpxchg was happening.
+    #[inline(always)]
+    fn try_cmpxchg<Ordering: Any>(&self, old: &mut T, new: T, _: Ordering) -> bool {
+        let mut old_tmp = into_repr(*old);
+        let oldp = &raw mut old_tmp;
+        let new = into_repr(new);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.0.get().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_try_cmpxchg*() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - `oldp` is a valid pointer for the function.
+        //   - Per the type guarantees, 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.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `new` is a valid bit pattern of `T`, so it's sound to store it in an `Atomic<T>`.
+        let ret = unsafe {
+            match Ordering::TYPE {
+                OrderingType::Full => T::Repr::atomic_try_cmpxchg(a, oldp, new),
+                OrderingType::Acquire => T::Repr::atomic_try_cmpxchg_acquire(a, oldp, new),
+                OrderingType::Release => T::Repr::atomic_try_cmpxchg_release(a, oldp, new),
+                OrderingType::Relaxed => T::Repr::atomic_try_cmpxchg_relaxed(a, oldp, new),
+            }
+        };
+
+        // SAFETY: The atomic variable holds a valid `T`, so `old_tmp` is a valid bit pattern of
+        // `T`, therefore it's safe to call `from_repr()`.
+        *old = unsafe { from_repr(old_tmp) };
+
+        ret
+    }
+}
-- 
2.39.5 (Apple Git-154)

[PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Boqun Feng]

One important set of atomic operations is the arithmetic operations,
i.e. add(), sub(), fetch_add(), add_return(), etc. However it may not
make senses for all the types that `AllowAtomic` to have arithmetic
operations, for example a `Foo(u32)` may not have a reasonable add() or
sub(), plus subword types (`u8` and `u16`) currently don't have
atomic arithmetic operations even on C side and might not have them in
the future in Rust (because they are usually suboptimal on a few
architecures). Therefore add a subtrait of `AllowAtomic` describing
which types have and can do atomic arithemtic operations.

Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
`i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.

Only add() and fetch_add() are added. The rest will be added in the
future.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs         |  18 +++++
 rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index c5193c1c90fe..26f66cccd4e0 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -29,8 +29,26 @@ unsafe impl generic::AllowAtomic for i32 {
     type Repr = i32;
 }
 
+// SAFETY: `i32` is always sound to transmute back to itself.
+unsafe impl generic::AllowAtomicArithmetic for i32 {
+    type Delta = i32;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d
+    }
+}
+
 // SAFETY: `i64` has the same size and alignment with itself, and is round-trip transmutable to
 // itself.
 unsafe impl generic::AllowAtomic for i64 {
     type Repr = i64;
 }
+
+// SAFETY: `i64` is always sound to transmute back to itself.
+unsafe impl generic::AllowAtomicArithmetic for i64 {
+    type Delta = i64;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d
+    }
+}
diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
index 1beb802843ee..412a2c811c3d 100644
--- a/rust/kernel/sync/atomic/generic.rs
+++ b/rust/kernel/sync/atomic/generic.rs
@@ -111,6 +111,20 @@ const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
     unsafe { core::mem::transmute_copy(&r) }
 }
 
+/// Atomics that allows arithmetic operations with an integer type.
+///
+/// # Safety
+///
+/// Implementers must guarantee [`Self::Repr`] can always soundly [`transmute()`] to [`Self`] after
+/// arithmetic operations.
+pub unsafe trait AllowAtomicArithmetic: AllowAtomic {
+    /// The delta types for arithmetic operations.
+    type Delta;
+
+    /// Converts [`Self::Delta`] into the representation of the atomic type.
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr;
+}
+
 impl<T: AllowAtomic> Atomic<T> {
     /// Creates a new atomic.
     pub const fn new(v: T) -> Self {
@@ -457,3 +471,97 @@ fn try_cmpxchg<Ordering: Any>(&self, old: &mut T, new: T, _: Ordering) -> bool {
         ret
     }
 }
+
+impl<T: AllowAtomicArithmetic> Atomic<T>
+where
+    T::Repr: AtomicHasArithmeticOps,
+{
+    /// Atomic add.
+    ///
+    /// The addition is a wrapping addition.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::sync::atomic::{Atomic, Relaxed};
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// x.add(12, Relaxed);
+    ///
+    /// assert_eq!(54, x.load(Relaxed));
+    /// ```
+    #[inline(always)]
+    pub fn add<Ordering: RelaxedOnly>(&self, v: T::Delta, _: Ordering) {
+        let v = T::delta_into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_add() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, 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.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `T: AllowAtomicArithmetic` guarantees the arithmetic operation result is sound to
+        //     stored in an `Atomic<T>`.
+        unsafe {
+            T::Repr::atomic_add(a, v);
+        }
+    }
+
+    /// Atomic fetch and add.
+    ///
+    /// The addition is a wrapping addition.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::sync::atomic::{Atomic, Acquire, Full, Relaxed};
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// assert_eq!(54, { x.fetch_add(12, Acquire); x.load(Relaxed) });
+    ///
+    /// let x = Atomic::new(42);
+    ///
+    /// assert_eq!(42, x.load(Relaxed));
+    ///
+    /// assert_eq!(54, { x.fetch_add(12, Full); x.load(Relaxed) } );
+    /// ```
+    #[inline(always)]
+    pub fn fetch_add<Ordering: Any>(&self, v: T::Delta, _: Ordering) -> T {
+        let v = T::delta_into_repr(v);
+        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
+        // valid pointer of `T::Repr`.
+        let a = self.as_ptr().cast::<T::Repr>();
+
+        // SAFETY:
+        // - For calling the atomic_fetch_add*() function:
+        //   - `a` is a valid pointer for the function per the CAST justification above.
+        //   - Per the type guarantees, 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.
+        // - For the bit validity of `Atomic<T>`:
+        //   - `T: AllowAtomicArithmetic` guarantees the arithmetic operation result is sound to
+        //     stored in an `Atomic<T>`.
+        let ret = unsafe {
+            match Ordering::TYPE {
+                OrderingType::Full => T::Repr::atomic_fetch_add(a, v),
+                OrderingType::Acquire => T::Repr::atomic_fetch_add_acquire(a, v),
+                OrderingType::Release => T::Repr::atomic_fetch_add_release(a, v),
+                OrderingType::Relaxed => T::Repr::atomic_fetch_add_relaxed(a, v),
+            }
+        };
+
+        // SAFETY: Per safety requirement of `AllowAtomicArithmetic`, `ret` is a valid bit pattern
+        // of `T`.
+        unsafe { from_repr(ret) }
+    }
+}
-- 
2.39.5 (Apple Git-154)

[PATCH v6 7/9] rust: sync: atomic: Add Atomic<u{32,64}>

[Boqun Feng]

Add generic atomic support for basic unsigned types that have an
`AtomicImpl` with the same size and alignment.

Unit tests are added including Atomic<i32> and Atomic<i64>.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs | 99 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index 26f66cccd4e0..e676bc7d9275 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -52,3 +52,102 @@ fn delta_into_repr(d: Self::Delta) -> Self::Repr {
         d
     }
 }
+
+// SAFETY: `u32` and `i32` has the same size and alignment, and `u32` is round-trip transmutable to
+// `i32`.
+unsafe impl generic::AllowAtomic for u32 {
+    type Repr = i32;
+}
+
+// SAFETY: `i32` is always sound to transmute back to `u32`.
+unsafe impl generic::AllowAtomicArithmetic for u32 {
+    type Delta = u32;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d as Self::Repr
+    }
+}
+
+// SAFETY: `u64` and `i64` has the same size and alignment, and `u64` is round-trip transmutable to
+// `i64`.
+unsafe impl generic::AllowAtomic for u64 {
+    type Repr = i64;
+}
+
+// SAFETY: `i64` is always sound to transmute back to `u64`.
+unsafe impl generic::AllowAtomicArithmetic for u64 {
+    type Delta = u64;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d as Self::Repr
+    }
+}
+
+use crate::macros::kunit_tests;
+
+#[kunit_tests(rust_atomics)]
+mod tests {
+    use super::*;
+
+    // Call $fn($val) with each $type of $val.
+    macro_rules! for_each_type {
+        ($val:literal in [$($type:ty),*] $fn:expr) => {
+            $({
+                let v: $type = $val;
+
+                $fn(v);
+            })*
+        }
+    }
+
+    #[test]
+    fn atomic_basic_tests() {
+        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+            let x = Atomic::new(v);
+
+            assert_eq!(v, x.load(Relaxed));
+        });
+    }
+
+    #[test]
+    fn atomic_xchg_tests() {
+        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+            let x = Atomic::new(v);
+
+            let old = v;
+            let new = v + 1;
+
+            assert_eq!(old, x.xchg(new, Full));
+            assert_eq!(new, x.load(Relaxed));
+        });
+    }
+
+    #[test]
+    fn atomic_cmpxchg_tests() {
+        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+            let x = Atomic::new(v);
+
+            let old = v;
+            let new = v + 1;
+
+            assert_eq!(Err(old), x.cmpxchg(new, new, Full));
+            assert_eq!(old, x.load(Relaxed));
+            assert_eq!(Ok(old), x.cmpxchg(old, new, Relaxed));
+            assert_eq!(new, x.load(Relaxed));
+        });
+    }
+
+    #[test]
+    fn atomic_arithmetic_tests() {
+        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+            let x = Atomic::new(v);
+
+            assert_eq!(v, x.fetch_add(12, Full));
+            assert_eq!(v + 12, x.load(Relaxed));
+
+            x.add(13, Relaxed);
+
+            assert_eq!(v + 25, x.load(Relaxed));
+        });
+    }
+}
-- 
2.39.5 (Apple Git-154)

[PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

Memory barriers are building blocks for concurrent code, hence provide
a minimal set of them.

The compiler barrier, barrier(), is implemented in inline asm instead of
using core::sync::atomic::compiler_fence() because memory models are
different: kernel's atomics are implemented in inline asm therefore the
compiler barrier should be implemented in inline asm as well. Also it's
currently only public to the kernel crate until there's a reasonable
driver usage.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/helpers/barrier.c      | 18 ++++++++++
 rust/helpers/helpers.c      |  1 +
 rust/kernel/sync.rs         |  1 +
 rust/kernel/sync/barrier.rs | 65 +++++++++++++++++++++++++++++++++++++
 4 files changed, 85 insertions(+)
 create mode 100644 rust/helpers/barrier.c
 create mode 100644 rust/kernel/sync/barrier.rs

diff --git a/rust/helpers/barrier.c b/rust/helpers/barrier.c
new file mode 100644
index 000000000000..cdf28ce8e511
--- /dev/null
+++ b/rust/helpers/barrier.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <asm/barrier.h>
+
+void rust_helper_smp_mb(void)
+{
+	smp_mb();
+}
+
+void rust_helper_smp_wmb(void)
+{
+	smp_wmb();
+}
+
+void rust_helper_smp_rmb(void)
+{
+	smp_rmb();
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 83e89f6a68fb..8ddfc8f84e87 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -9,6 +9,7 @@
 
 #include "atomic.c"
 #include "auxiliary.c"
+#include "barrier.c"
 #include "blk.c"
 #include "bug.c"
 #include "build_assert.c"
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index b620027e0641..c7c0e552bafe 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -11,6 +11,7 @@
 
 mod arc;
 pub mod atomic;
+pub mod barrier;
 mod condvar;
 pub mod lock;
 mod locked_by;
diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
new file mode 100644
index 000000000000..df4015221503
--- /dev/null
+++ b/rust/kernel/sync/barrier.rs
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Memory barriers.
+//!
+//! These primitives have the same semantics as their C counterparts: and the precise definitions
+//! of semantics can be found at [`LKMM`].
+//!
+//! [`LKMM`]: srctree/tools/memory-model/
+
+/// A compiler barrier.
+///
+/// A barrier that prevents compiler from reordering memory accesses across the barrier.
+pub(crate) fn barrier() {
+    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
+    // it suffices as a compiler barrier.
+    //
+    // SAFETY: An empty asm block should be safe.
+    unsafe {
+        core::arch::asm!("");
+    }
+}
+
+/// A full memory barrier.
+///
+/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
+pub fn smp_mb() {
+    if cfg!(CONFIG_SMP) {
+        // SAFETY: `smp_mb()` is safe to call.
+        unsafe {
+            bindings::smp_mb();
+        }
+    } else {
+        barrier();
+    }
+}
+
+/// A write-write memory barrier.
+///
+/// A barrier that prevents compiler and CPU from reordering memory write accesses across the
+/// barrier.
+pub fn smp_wmb() {
+    if cfg!(CONFIG_SMP) {
+        // SAFETY: `smp_wmb()` is safe to call.
+        unsafe {
+            bindings::smp_wmb();
+        }
+    } else {
+        barrier();
+    }
+}
+
+/// A read-read memory barrier.
+///
+/// A barrier that prevents compiler and CPU from reordering memory read accesses across the
+/// barrier.
+pub fn smp_rmb() {
+    if cfg!(CONFIG_SMP) {
+        // SAFETY: `smp_rmb()` is safe to call.
+        unsafe {
+            bindings::smp_rmb();
+        }
+    } else {
+        barrier();
+    }
+}
-- 
2.39.5 (Apple Git-154)

[PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Boqun Feng]

Add generic atomic support for `usize` and `isize`. Note that instead of
mapping directly to `atomic_long_t`, the represention type
(`AllowAtomic::Repr`) is selected based on CONFIG_64BIT. This reduces
the necessity of creating `atomic_long_*` helpers, which could save
the binary size of kernel if inline helpers are not available.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/atomic.rs | 48 ++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index e676bc7d9275..e1e40757d7b5 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -53,6 +53,26 @@ fn delta_into_repr(d: Self::Delta) -> Self::Repr {
     }
 }
 
+// SAFETY: For 32bit kernel, `isize` has the same size and alignment with `i32` and is round-trip
+// transmutable to it, for 64bit kernel `isize` has the same size and alignment with `i64` and is
+// round-trip transmutable to it.
+unsafe impl generic::AllowAtomic for isize {
+    #[cfg(not(CONFIG_64BIT))]
+    type Repr = i32;
+    #[cfg(CONFIG_64BIT)]
+    type Repr = i64;
+}
+
+// SAFETY: `isize` is always sound to transmute back from `i32` or `i64` when their sizes are the
+// same.
+unsafe impl generic::AllowAtomicArithmetic for isize {
+    type Delta = Self;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d as Self::Repr
+    }
+}
+
 // SAFETY: `u32` and `i32` has the same size and alignment, and `u32` is round-trip transmutable to
 // `i32`.
 unsafe impl generic::AllowAtomic for u32 {
@@ -83,6 +103,26 @@ fn delta_into_repr(d: Self::Delta) -> Self::Repr {
     }
 }
 
+// SAFETY: For 32bit kernel, `usize` has the same size and alignment with `i32` and is round-trip
+// transmutable to it, for 64bit kernel `usize` has the same size and alignment with `i64` and is
+// round-trip transmutable to it.
+unsafe impl generic::AllowAtomic for usize {
+    #[cfg(not(CONFIG_64BIT))]
+    type Repr = i32;
+    #[cfg(CONFIG_64BIT)]
+    type Repr = i64;
+}
+
+// SAFETY: `usize` is always sound to transmute back from `i32` or `i64` when their sizes are the
+// same.
+unsafe impl generic::AllowAtomicArithmetic for usize {
+    type Delta = Self;
+
+    fn delta_into_repr(d: Self::Delta) -> Self::Repr {
+        d as Self::Repr
+    }
+}
+
 use crate::macros::kunit_tests;
 
 #[kunit_tests(rust_atomics)]
@@ -102,7 +142,7 @@ macro_rules! for_each_type {
 
     #[test]
     fn atomic_basic_tests() {
-        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+        for_each_type!(42 in [i32, i64, u32, u64, isize, usize] |v| {
             let x = Atomic::new(v);
 
             assert_eq!(v, x.load(Relaxed));
@@ -111,7 +151,7 @@ fn atomic_basic_tests() {
 
     #[test]
     fn atomic_xchg_tests() {
-        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+        for_each_type!(42 in [i32, i64, u32, u64, isize, usize] |v| {
             let x = Atomic::new(v);
 
             let old = v;
@@ -124,7 +164,7 @@ fn atomic_xchg_tests() {
 
     #[test]
     fn atomic_cmpxchg_tests() {
-        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+        for_each_type!(42 in [i32, i64, u32, u64, isize, usize] |v| {
             let x = Atomic::new(v);
 
             let old = v;
@@ -139,7 +179,7 @@ fn atomic_cmpxchg_tests() {
 
     #[test]
     fn atomic_arithmetic_tests() {
-        for_each_type!(42 in [i32, i64, u32, u64] |v| {
+        for_each_type!(42 in [i32, i64, u32, u64, isize, usize] |v| {
             let x = Atomic::new(v);
 
             assert_eq!(v, x.fetch_add(12, Full));
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> Preparation for generic atomic implementation. To unify the
> implementation of a generic method over `i32` and `i64`, the C side
> atomic methods need to be grouped so that in a generic method, they can
> be referred as <type>::<method>, otherwise their parameters and return
> value are different between `i32` and `i64`, which would require using
> `transmute()` to unify the type into a `T`.
>
> Introduce `AtomicImpl` to represent a basic type in Rust that has the
> direct mapping to an atomic implementation from C. This trait is sealed,
> and currently only `i32` and `i64` impl this.
>
> Further, different methods are put into different `*Ops` trait groups,
> and this is for the future when smaller types like `i8`/`i16` are
> supported but only with a limited set of API (e.g. only set(), load(),
> xchg() and cmpxchg(), no add() or sub() etc).
>
> While the atomic mod is introduced, documentation is also added for
> memory models and data races.
>
> Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect
> my responsiblity on the Rust atomic mod.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Overall this looks good from a functionality view. I have some cosmetic
comments for the macros below and a possibly bigger concern regarding
safety comments. But I think this is good enough for now, so:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> diff --git a/rust/kernel/sync/atomic/ops.rs b/rust/kernel/sync/atomic/ops.rs
> new file mode 100644
> index 000000000000..da04dd383962
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/ops.rs
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Atomic implementations.
> +//!
> +//! Provides 1:1 mapping of atomic implementations.
> +
> +use crate::bindings::*;

We shouldn't import all bindings, just use `bindings::` below.

> +// This macro generates the function signature with given argument list and return type.
> +macro_rules! declare_atomic_method {
> +    (
> +        $func:ident($($arg:ident : $arg_type:ty),*) $(-> $ret:ty)?
> +    ) => {
> +        paste!(
> +            #[doc = concat!("Atomic ", stringify!($func))]
> +            #[doc = "# Safety"]
> +            #[doc = "- Any pointer passed to the function has to be a valid pointer"]
> +            #[doc = "- Accesses must not cause data races per LKMM:"]
> +            #[doc = "  - Atomic read racing with normal read, normal write or atomic write is not data race."]

s/not/not a/

> +            #[doc = "  - Atomic write racing with normal read or normal write is data-race, unless the"]

s/data-race/a data race/

> +            #[doc = "    normal accesses are done at C side and considered as immune to data"]

    #[doc = "    normal access is done from the C side and considered immune to data"]

> +            #[doc = "    races, e.g. CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC."]

Missing '`'.


Also why aren't you using `///` instead of `#[doc =`? The only part
where you need interpolation is the first one.

> +            unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)?;
> +        );
> +    };

> +declare_and_impl_atomic_methods!(
> +    AtomicHasBasicOps ("Basic atomic operations") {
> +        read[acquire](ptr: *mut Self) -> Self {
> +            call(ptr.cast())
> +        }
> +
> +        set[release](ptr: *mut Self, v: Self) {
> +            call(ptr.cast(), v)
> +        }
> +    }

I think this would look a bit better:

    /// Basic atomic operations.
    pub trait AtomicHasBasicOps {
        unsafe fn read[acquire](ptr: *mut Self) -> Self {
            bindings::#call(ptr.cast())
        }

        unsafe fn set[release](ptr: *mut Self, v: Self) {
            bindings::#call(ptr.cast(), v)
        }
    }

And then we could also put the safety comments inline:

    /// Basic atomic operations.
    pub trait AtomicHasBasicOps {
        /// Atomic read
        ///
        /// # Safety
        /// - Any pointer passed to the function has to be a valid pointer
        /// - Accesses must not cause data races per LKMM:
        ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
        ///   - Atomic write racing with normal read or normal write is a data race, unless the
        ///     normal access is done from the C side and considered immune to data races, e.g.
        ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
        unsafe fn read[acquire](ptr: *mut Self) -> Self {
            // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
            // cause data race per LKMM.
            unsafe { bindings::#call(ptr.cast()) }
        }

        /// Atomic read
        ///
        /// # Safety
        /// - Any pointer passed to the function has to be a valid pointer
        /// - Accesses must not cause data races per LKMM:
        ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
        ///   - Atomic write racing with normal read or normal write is a data race, unless the
        ///     normal access is done from the C side and considered immune to data races, e.g.
        ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
        unsafe fn set[release](ptr: *mut Self, v: Self) {
            // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
            // cause data race per LKMM.
            unsafe { bindings::#call(ptr.cast(), v) }
        }
    }

I'm not sure if this is worth it, but for reading the definitions of
these operations directly in the code this is going to be a lot more
readable. I don't think it's too bad to duplicate it.

I'm also not fully satisfied with the safety comment on
`bindings::#call`...

---
Cheers,
Benno

> +);
> +
> +declare_and_impl_atomic_methods!(
> +    AtomicHasXchgOps ("Exchange and compare-and-exchange atomic operations") {
> +        xchg[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> +            call(ptr.cast(), v)
> +        }
> +
> +        try_cmpxchg[acquire, release, relaxed](ptr: *mut Self, old: *mut Self, new: Self) -> bool {
> +            call(ptr.cast(), old, new)
> +        }
> +    }
> +);
> +
> +declare_and_impl_atomic_methods!(
> +    AtomicHasArithmeticOps ("Atomic arithmetic operations") {
> +        add[](ptr: *mut Self, v: Self) {
> +            call(v, ptr.cast())
> +        }
> +
> +        fetch_add[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> +            call(v, ptr.cast())
> +        }
> +    }
> +);

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> Preparation for atomic primitives. Instead of a suffix like _acquire, a
> method parameter along with the corresponding generic parameter will be
> used to specify the ordering of an atomic operations. For example,
> atomic load() can be defined as:
>
> 	impl<T: ...> Atomic<T> {
> 	    pub fn load<O: AcquireOrRelaxed>(&self, _o: O) -> T { ... }
> 	}
>
> and acquire users would do:
>
> 	let r = x.load(Acquire);
>
> relaxed users:
>
> 	let r = x.load(Relaxed);
>
> doing the following:
>
> 	let r = x.load(Release);
>
> will cause a compiler error.
>
> Compared to suffixes, it's easier to tell what ordering variants an
> operation has, and it also make it easier to unify the implementation of
> all ordering variants in one method via generic. The `TYPE` associate
> const is for generic function to pick up the particular implementation
> specified by an ordering annotation.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

One naming comment below, with that fixed:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> ---
>  rust/kernel/sync/atomic.rs          |  3 +
>  rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 rust/kernel/sync/atomic/ordering.rs

> +/// The trait bound for annotating operations that support any ordering.
> +pub trait Any: internal::Sealed {

I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
should require people to write `ordering::Any` because otherwise it's
pretty confusing.

---
Cheers,
Benno

> +    /// Describes the exact memory ordering.
> +    const TYPE: OrderingType;
> +}

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> Preparation for atomic primitives. Instead of a suffix like _acquire, a
>> method parameter along with the corresponding generic parameter will be
>> used to specify the ordering of an atomic operations. For example,
>> atomic load() can be defined as:
>>
>> 	impl<T: ...> Atomic<T> {
>> 	    pub fn load<O: AcquireOrRelaxed>(&self, _o: O) -> T { ... }
>> 	}
>>
>> and acquire users would do:
>>
>> 	let r = x.load(Acquire);
>>
>> relaxed users:
>>
>> 	let r = x.load(Relaxed);
>>
>> doing the following:
>>
>> 	let r = x.load(Release);
>>
>> will cause a compiler error.
>>
>> Compared to suffixes, it's easier to tell what ordering variants an
>> operation has, and it also make it easier to unify the implementation of
>> all ordering variants in one method via generic. The `TYPE` associate
>> const is for generic function to pick up the particular implementation
>> specified by an ordering annotation.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>
> One naming comment below, with that fixed:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
>> ---
>>  rust/kernel/sync/atomic.rs          |  3 +
>>  rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
>>  2 files changed, 100 insertions(+)
>>  create mode 100644 rust/kernel/sync/atomic/ordering.rs
>
>> +/// The trait bound for annotating operations that support any ordering.
>> +pub trait Any: internal::Sealed {
>
> I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
> should require people to write `ordering::Any` because otherwise it's
> pretty confusing.

I agree with this observation.


Best regards,
Andreas Hindborg

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Boqun Feng]

On Thu, Jul 10, 2025 at 02:00:59PM +0200, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
> 
> > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> >> Preparation for atomic primitives. Instead of a suffix like _acquire, a
> >> method parameter along with the corresponding generic parameter will be
> >> used to specify the ordering of an atomic operations. For example,
> >> atomic load() can be defined as:
> >>
> >> 	impl<T: ...> Atomic<T> {
> >> 	    pub fn load<O: AcquireOrRelaxed>(&self, _o: O) -> T { ... }
> >> 	}
> >>
> >> and acquire users would do:
> >>
> >> 	let r = x.load(Acquire);
> >>
> >> relaxed users:
> >>
> >> 	let r = x.load(Relaxed);
> >>
> >> doing the following:
> >>
> >> 	let r = x.load(Release);
> >>
> >> will cause a compiler error.
> >>
> >> Compared to suffixes, it's easier to tell what ordering variants an
> >> operation has, and it also make it easier to unify the implementation of
> >> all ordering variants in one method via generic. The `TYPE` associate
> >> const is for generic function to pick up the particular implementation
> >> specified by an ordering annotation.
> >>
> >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >
> > One naming comment below, with that fixed:
> >
> > Reviewed-by: Benno Lossin <lossin@kernel.org>
> >
> >> ---
> >>  rust/kernel/sync/atomic.rs          |  3 +
> >>  rust/kernel/sync/atomic/ordering.rs | 97 +++++++++++++++++++++++++++++
> >>  2 files changed, 100 insertions(+)
> >>  create mode 100644 rust/kernel/sync/atomic/ordering.rs
> >
> >> +/// The trait bound for annotating operations that support any ordering.
> >> +pub trait Any: internal::Sealed {
> >
> > I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
> > should require people to write `ordering::Any` because otherwise it's
> > pretty confusing.
> 
> I agree with this observation.
> 

I'm OK to do the change, but let me show my arguments ;-)

* First, we are using a language that supports namespaces,
  so I feel it's a bit unnecessary to use a different name just because
  it conflicts with `core::any::Any`. Doing so kinda undermines the
  namespace concepts. And we may have other `Any`s in the future, are we
  sure at the moment we should keyword `Any`?

* Another thing is that this trait won't be used very often outside
  definition of functions that having ordering variants, currently the
  only users are all inside atomic/generic.rs.

I probably choose the `ordering::Any` approach if you guys insist.

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 
> 

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Benno Lossin]

On Thu Jul 10, 2025 at 4:42 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 02:00:59PM +0200, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>> > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> >> +/// The trait bound for annotating operations that support any ordering.
>> >> +pub trait Any: internal::Sealed {
>> >
>> > I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
>> > should require people to write `ordering::Any` because otherwise it's
>> > pretty confusing.
>> 
>> I agree with this observation.
>> 
>
> I'm OK to do the change, but let me show my arguments ;-)
>
> * First, we are using a language that supports namespaces,
>   so I feel it's a bit unnecessary to use a different name just because
>   it conflicts with `core::any::Any`. Doing so kinda undermines the
>   namespace concepts. And we may have other `Any`s in the future, are we
>   sure at the moment we should keyword `Any`?

I don't think `Any` is a good name for something this specific anyways.
If it were something private, then sure use `Any`, but since this is
public, I don't think `Any` is a good name.

> * Another thing is that this trait won't be used very often outside
>   definition of functions that having ordering variants, currently the
>   only users are all inside atomic/generic.rs.

I don't think this is a good argument to keep a bad name.

> I probably choose the `ordering::Any` approach if you guys insist.

I don't think we have a lint for that, so I'd prefer if we avoid that...

Someone is going to just `use ...::ordering::Any` and then have a
function `fn<T: Any>(_: T)` in their code and that will be very
confusing.

---
Cheers,
Benno

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Boqun Feng]

On Thu, Jul 10, 2025 at 01:04:38PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > Preparation for generic atomic implementation. To unify the
> > implementation of a generic method over `i32` and `i64`, the C side
> > atomic methods need to be grouped so that in a generic method, they can
> > be referred as <type>::<method>, otherwise their parameters and return
> > value are different between `i32` and `i64`, which would require using
> > `transmute()` to unify the type into a `T`.
> >
> > Introduce `AtomicImpl` to represent a basic type in Rust that has the
> > direct mapping to an atomic implementation from C. This trait is sealed,
> > and currently only `i32` and `i64` impl this.
> >
> > Further, different methods are put into different `*Ops` trait groups,
> > and this is for the future when smaller types like `i8`/`i16` are
> > supported but only with a limited set of API (e.g. only set(), load(),
> > xchg() and cmpxchg(), no add() or sub() etc).
> >
> > While the atomic mod is introduced, documentation is also added for
> > memory models and data races.
> >
> > Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect
> > my responsiblity on the Rust atomic mod.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Overall this looks good from a functionality view. I have some cosmetic
> comments for the macros below and a possibly bigger concern regarding
> safety comments. But I think this is good enough for now, so:
> 
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> 

Thanks!

> > diff --git a/rust/kernel/sync/atomic/ops.rs b/rust/kernel/sync/atomic/ops.rs
> > new file mode 100644
> > index 000000000000..da04dd383962
> > --- /dev/null
> > +++ b/rust/kernel/sync/atomic/ops.rs
> > @@ -0,0 +1,195 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Atomic implementations.
> > +//!
> > +//! Provides 1:1 mapping of atomic implementations.
> > +
> > +use crate::bindings::*;
> 
> We shouldn't import all bindings, just use `bindings::` below.
> 

Make sense!

> > +// This macro generates the function signature with given argument list and return type.
> > +macro_rules! declare_atomic_method {
> > +    (
> > +        $func:ident($($arg:ident : $arg_type:ty),*) $(-> $ret:ty)?
> > +    ) => {
> > +        paste!(
> > +            #[doc = concat!("Atomic ", stringify!($func))]
> > +            #[doc = "# Safety"]
> > +            #[doc = "- Any pointer passed to the function has to be a valid pointer"]
> > +            #[doc = "- Accesses must not cause data races per LKMM:"]
> > +            #[doc = "  - Atomic read racing with normal read, normal write or atomic write is not data race."]
> 
> s/not/not a/
> 
> > +            #[doc = "  - Atomic write racing with normal read or normal write is data-race, unless the"]
> 
> s/data-race/a data race/
> 
> > +            #[doc = "    normal accesses are done at C side and considered as immune to data"]
> 
>     #[doc = "    normal access is done from the C side and considered immune to data"]
> 
> > +            #[doc = "    races, e.g. CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC."]
> 
> Missing '`'.
> 

Fixed.

> 
> Also why aren't you using `///` instead of `#[doc =`? The only part
> where you need interpolation is the first one.
> 

I think at a certain point I was not using `paste!()` and then using
`///` wouldn't generate them into rustdoc, but with `paste!()` your
suggestion makes sense, thanks!

> > +            unsafe fn [< atomic_ $func >]($($arg: $arg_type,)*) $(-> $ret)?;
> > +        );
> > +    };
> 
> > +declare_and_impl_atomic_methods!(
> > +    AtomicHasBasicOps ("Basic atomic operations") {
> > +        read[acquire](ptr: *mut Self) -> Self {
> > +            call(ptr.cast())
> > +        }
> > +
> > +        set[release](ptr: *mut Self, v: Self) {
> > +            call(ptr.cast(), v)
> > +        }
> > +    }
> 
> I think this would look a bit better:
> 
>     /// Basic atomic operations.
>     pub trait AtomicHasBasicOps {
>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
>             bindings::#call(ptr.cast())
>         }
> 
>         unsafe fn set[release](ptr: *mut Self, v: Self) {
>             bindings::#call(ptr.cast(), v)
>         }
>     }
> 

Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
hard-coded:

macro_rules! declare_and_impl_atomic_methods {
    (#[doc = $doc:expr] pub trait $ops:ident {
        $(
            unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
                bindings::#call($($arg:tt)*)
            }
        )*
    }) => {

It shouldn't be very hard to make use of the actual visibility or
unsafe, but we currently don't have other visibility or safe function,
so it's simple to keep it as it is.

> And then we could also put the safety comments inline:
> 
>     /// Basic atomic operations.
>     pub trait AtomicHasBasicOps {
>         /// Atomic read
>         ///
>         /// # Safety
>         /// - Any pointer passed to the function has to be a valid pointer
>         /// - Accesses must not cause data races per LKMM:
>         ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>         ///   - Atomic write racing with normal read or normal write is a data race, unless the
>         ///     normal access is done from the C side and considered immune to data races, e.g.
>         ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
>             // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>             // cause data race per LKMM.
>             unsafe { bindings::#call(ptr.cast()) }
>         }
> 
>         /// Atomic read

Copy-pasta ;-)

>         ///
>         /// # Safety
>         /// - Any pointer passed to the function has to be a valid pointer
>         /// - Accesses must not cause data races per LKMM:
>         ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>         ///   - Atomic write racing with normal read or normal write is a data race, unless the
>         ///     normal access is done from the C side and considered immune to data races, e.g.
>         ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>         unsafe fn set[release](ptr: *mut Self, v: Self) {
>             // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>             // cause data race per LKMM.
>             unsafe { bindings::#call(ptr.cast(), v) }
>         }
>     }
> 
> I'm not sure if this is worth it, but for reading the definitions of
> these operations directly in the code this is going to be a lot more
> readable. I don't think it's too bad to duplicate it.
> 
> I'm also not fully satisfied with the safety comment on
> `bindings::#call`...
> 

Based on the above, I'm not going to do the change (i.e. duplicating
the safe comments and improve them), and I would make an issue tracking
it, and we can revisit it when we have time. Sounds good?

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > +);
> > +
> > +declare_and_impl_atomic_methods!(
> > +    AtomicHasXchgOps ("Exchange and compare-and-exchange atomic operations") {
> > +        xchg[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> > +            call(ptr.cast(), v)
> > +        }
> > +
> > +        try_cmpxchg[acquire, release, relaxed](ptr: *mut Self, old: *mut Self, new: Self) -> bool {
> > +            call(ptr.cast(), old, new)
> > +        }
> > +    }
> > +);
> > +
> > +declare_and_impl_atomic_methods!(
> > +    AtomicHasArithmeticOps ("Atomic arithmetic operations") {
> > +        add[](ptr: *mut Self, v: Self) {
> > +            call(v, ptr.cast())
> > +        }
> > +
> > +        fetch_add[acquire, release, relaxed](ptr: *mut Self, v: Self) -> Self {
> > +            call(v, ptr.cast())
> > +        }
> > +    }
> > +);

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Benno Lossin]

On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 01:04:38PM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> > +declare_and_impl_atomic_methods!(
>> > +    AtomicHasBasicOps ("Basic atomic operations") {
>> > +        read[acquire](ptr: *mut Self) -> Self {
>> > +            call(ptr.cast())
>> > +        }
>> > +
>> > +        set[release](ptr: *mut Self, v: Self) {
>> > +            call(ptr.cast(), v)
>> > +        }
>> > +    }
>> 
>> I think this would look a bit better:
>> 
>>     /// Basic atomic operations.
>>     pub trait AtomicHasBasicOps {
>>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
>>             bindings::#call(ptr.cast())
>>         }
>> 
>>         unsafe fn set[release](ptr: *mut Self, v: Self) {
>>             bindings::#call(ptr.cast(), v)
>>         }
>>     }
>> 
>
> Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
> hard-coded:
>
> macro_rules! declare_and_impl_atomic_methods {
>     (#[doc = $doc:expr] pub trait $ops:ident {

You should allow any kind of attribute (and multiple), that makes it
much simpler.

>         $(
>             unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
>                 bindings::#call($($arg:tt)*)
>             }
>         )*
>     }) => {
>
> It shouldn't be very hard to make use of the actual visibility or
> unsafe, but we currently don't have other visibility or safe function,
> so it's simple to keep it as it is.

Yeah I also meant hardcoding them.

>> And then we could also put the safety comments inline:
>> 
>>     /// Basic atomic operations.
>>     pub trait AtomicHasBasicOps {
>>         /// Atomic read
>>         ///
>>         /// # Safety
>>         /// - Any pointer passed to the function has to be a valid pointer
>>         /// - Accesses must not cause data races per LKMM:
>>         ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>>         ///   - Atomic write racing with normal read or normal write is a data race, unless the
>>         ///     normal access is done from the C side and considered immune to data races, e.g.
>>         ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
>>             // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>>             // cause data race per LKMM.
>>             unsafe { bindings::#call(ptr.cast()) }
>>         }
>> 
>>         /// Atomic read
>
> Copy-pasta ;-)
>
>>         ///
>>         /// # Safety
>>         /// - Any pointer passed to the function has to be a valid pointer
>>         /// - Accesses must not cause data races per LKMM:
>>         ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>>         ///   - Atomic write racing with normal read or normal write is a data race, unless the
>>         ///     normal access is done from the C side and considered immune to data races, e.g.
>>         ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>>         unsafe fn set[release](ptr: *mut Self, v: Self) {
>>             // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>>             // cause data race per LKMM.
>>             unsafe { bindings::#call(ptr.cast(), v) }
>>         }
>>     }
>> 
>> I'm not sure if this is worth it, but for reading the definitions of
>> these operations directly in the code this is going to be a lot more
>> readable. I don't think it's too bad to duplicate it.
>> 
>> I'm also not fully satisfied with the safety comment on
>> `bindings::#call`...
>> 
>
> Based on the above, I'm not going to do the change (i.e. duplicating
> the safe comments and improve them), and I would make an issue tracking
> it, and we can revisit it when we have time. Sounds good?

Sure, I feel like some kind of method duplication macro might be much
better here, like:

    multi_functions! {
        pub trait AtomicHasBasicOps {
            /// Atomic read
            ///
            /// # Safety
            /// - Any pointer passed to the function has to be a valid pointer
            /// - Accesses must not cause data races per LKMM:
            ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
            ///   - Atomic write racing with normal read or normal write is a data race, unless the
            ///     normal access is done from the C side and considered immune to data races, e.g.
            ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
            unsafe fn [<read, read_acquire>](ptr: *mut Self) -> Self;

            // ...
        }
    }

And then also allow it on impls. I don't really like the idea of
duplicating and thus hiding the safety docs... But I also see that just
copy pasting them everywhere isn't really a good solution either...

---
Cheers,
Benno

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Boqun Feng]

On Thu, Jul 10, 2025 at 05:05:25PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 4:42 PM CEST, Boqun Feng wrote:
> > On Thu, Jul 10, 2025 at 02:00:59PM +0200, Andreas Hindborg wrote:
> >> "Benno Lossin" <lossin@kernel.org> writes:
> >> > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> >> >> +/// The trait bound for annotating operations that support any ordering.
> >> >> +pub trait Any: internal::Sealed {
> >> >
> >> > I don't like the name `Any`, how about `AnyOrdering`? Otherwise we
> >> > should require people to write `ordering::Any` because otherwise it's
> >> > pretty confusing.
> >> 
> >> I agree with this observation.
> >> 
> >
> > I'm OK to do the change, but let me show my arguments ;-)
> >
> > * First, we are using a language that supports namespaces,
> >   so I feel it's a bit unnecessary to use a different name just because
> >   it conflicts with `core::any::Any`. Doing so kinda undermines the
> >   namespace concepts. And we may have other `Any`s in the future, are we
> >   sure at the moment we should keyword `Any`?
> 
> I don't think `Any` is a good name for something this specific anyways.

Well, that's the point of namespace: providing contexts for a name, and
the contexts can be very specific. I'm sure we both have used "any" in
English to refer something specific ;-)

> If it were something private, then sure use `Any`, but since this is
> public, I don't think `Any` is a good name.
> 

This essentially means we keyword `Any` as a public trait name, then we
should document it somewhere, along with other names we want to keyword.

Regards,
Boqun

> > * Another thing is that this trait won't be used very often outside
> >   definition of functions that having ordering variants, currently the
> >   only users are all inside atomic/generic.rs.
> 
> I don't think this is a good argument to keep a bad name.
> 
> > I probably choose the `ordering::Any` approach if you guys insist.
> 
> I don't think we have a lint for that, so I'd prefer if we avoid that...
> 
> Someone is going to just `use ...::ordering::Any` and then have a
> function `fn<T: Any>(_: T)` in their code and that will be very
> confusing.
> 
> ---
> Cheers,
> Benno

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Boqun Feng]

On Thu, Jul 10, 2025 at 05:46:56PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> > On Thu, Jul 10, 2025 at 01:04:38PM +0200, Benno Lossin wrote:
> >> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> >> > +declare_and_impl_atomic_methods!(
> >> > +    AtomicHasBasicOps ("Basic atomic operations") {
> >> > +        read[acquire](ptr: *mut Self) -> Self {
> >> > +            call(ptr.cast())
> >> > +        }
> >> > +
> >> > +        set[release](ptr: *mut Self, v: Self) {
> >> > +            call(ptr.cast(), v)
> >> > +        }
> >> > +    }
> >> 
> >> I think this would look a bit better:
> >> 
> >>     /// Basic atomic operations.
> >>     pub trait AtomicHasBasicOps {
> >>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
> >>             bindings::#call(ptr.cast())
> >>         }
> >> 
> >>         unsafe fn set[release](ptr: *mut Self, v: Self) {
> >>             bindings::#call(ptr.cast(), v)
> >>         }
> >>     }
> >> 
> >
> > Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
> > hard-coded:
> >
> > macro_rules! declare_and_impl_atomic_methods {
> >     (#[doc = $doc:expr] pub trait $ops:ident {
> 
> You should allow any kind of attribute (and multiple), that makes it
> much simpler.
> 

I didn't know I could do that, updated:

    ($(#[$attr:meta])* pub trait $ops:ident {
        $(
            unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
                bindings::#call($($arg:tt)*)
            }
        )*
    }) => {
        $(#[$attr])*

Thanks!

> >         $(
> >             unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
> >                 bindings::#call($($arg:tt)*)
> >             }
> >         )*
> >     }) => {
> >
> > It shouldn't be very hard to make use of the actual visibility or
> > unsafe, but we currently don't have other visibility or safe function,
> > so it's simple to keep it as it is.
[..]
> >> I'm not sure if this is worth it, but for reading the definitions of
> >> these operations directly in the code this is going to be a lot more
> >> readable. I don't think it's too bad to duplicate it.
> >> 
> >> I'm also not fully satisfied with the safety comment on
> >> `bindings::#call`...
> >> 
> >
> > Based on the above, I'm not going to do the change (i.e. duplicating
> > the safe comments and improve them), and I would make an issue tracking
> > it, and we can revisit it when we have time. Sounds good?
> 
> Sure, I feel like some kind of method duplication macro might be much
> better here, like:
> 
>     multi_functions! {
>         pub trait AtomicHasBasicOps {
>             /// Atomic read
>             ///
>             /// # Safety
>             /// - Any pointer passed to the function has to be a valid pointer
>             /// - Accesses must not cause data races per LKMM:
>             ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>             ///   - Atomic write racing with normal read or normal write is a data race, unless the
>             ///     normal access is done from the C side and considered immune to data races, e.g.
>             ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>             unsafe fn [<read, read_acquire>](ptr: *mut Self) -> Self;
> 
>             // ...
>         }
>     }
> 
> And then also allow it on impls. I don't really like the idea of
> duplicating and thus hiding the safety docs... But I also see that just

At least the rustdoc has safety section for each function. ;-)

> copy pasting them everywhere isn't really a good solution either...
> 

Yeah, perhaps there is no immediate resolution, but open to any
suggestion.

Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Miguel Ojeda]

On Thu, Jul 10, 2025 at 5:05 PM Benno Lossin <lossin@kernel.org> wrote:
>
> I don't think we have a lint for that, so I'd prefer if we avoid that...
>
> Someone is going to just `use ...::ordering::Any` and then have a
> function `fn<T: Any>(_: T)` in their code and that will be very
> confusing.

I guess there could be a lint that detects a given item being `use`d
which we could use in some cases like this.

I took a quick look, and I see `enum_glob_use`, but that seems global
without a way to filter, and it doesn't cover direct `use`s.

Then there is `wildcard_imports`, and that seems fairly usable (it has
an allowed list), but of course doesn't cover non-wildcard ones.

Cheers,
Miguel

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Miguel Ojeda]

On Thu, Jul 10, 2025 at 8:32 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I guess there could be a lint that detects a given item being `use`d
> which we could use in some cases like this.

Filled: https://github.com/rust-lang/rust-clippy/issues/15244.

Cheers,
Miguel

Re: [PATCH v6 3/9] rust: sync: atomic: Add ordering annotation types

[Benno Lossin]

On Thu Jul 10, 2025 at 5:57 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 05:05:25PM +0200, Benno Lossin wrote:
>> If it were something private, then sure use `Any`, but since this is
>> public, I don't think `Any` is a good name.
>> 
>
> This essentially means we keyword `Any` as a public trait name, then we
> should document it somewhere, along with other names we want to keyword.

Then let's restrict `Any`.

---
Cheers,
Benno

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Benno Lossin]

On Thu Jul 10, 2025 at 6:16 PM CEST, Boqun Feng wrote:
> At least the rustdoc has safety section for each function. ;-)

I don't usually use rustdoc to read function safety sections. Instead I
jump to their definition and read the code.

---
Cheers,
Benno

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Boqun Feng]

On Thu, Jul 10, 2025 at 09:21:17PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 6:16 PM CEST, Boqun Feng wrote:
> > At least the rustdoc has safety section for each function. ;-)
> 
> I don't usually use rustdoc to read function safety sections. Instead I
> jump to their definition and read the code.
> 

Understood. It's probalby the first time we use macros to generate a few
unsafe functions, so this is something new.

But let me ask you a question, as a programmer yourself, when you run
into a code base, and see something repeat in a similar pattern for 10+
times, what's your instinct? You would try to combine the them together,
right? That's why duplication seems not compelling to me. But surely, we
don't need to draw conclusion right now, however that's my opinion.
 
Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> new file mode 100644
> index 000000000000..e044fe21b128
> --- /dev/null
> +++ b/rust/kernel/sync/atomic/generic.rs
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic atomic primitives.
> +
> +use super::ops::*;
> +use super::ordering::*;
> +use crate::build_error;
> +use core::cell::UnsafeCell;
> +
> +/// A generic atomic variable.

How about we copy upstream rust on this:

    A memory location which can be safely modified from multiple threads.

> +///
> +/// `T` must impl [`AllowAtomic`], that is, an [`AtomicImpl`] has to be chosen.

s/impl/implement/

I don't really think this sentence is that valuable... I think you could
mention several things before this:

* compatibility with LKMM (or that it is implemented through LKMM
  atomics)
* "what is an atomic"
* how big (relative to `T`) is this type? what about alignment?

> +///
> +/// # Examples
> +///
> +/// A customized type stored in [`Atomic`]:
> +///
> +/// ```rust
> +/// use kernel::sync::atomic::{generic::AllowAtomic, Atomic, Relaxed};
> +///
> +/// #[derive(Clone, Copy, PartialEq, Eq)]
> +/// #[repr(i32)]
> +/// enum State {
> +///     Uninit = 0,
> +///     Working = 1,
> +///     Done = 2,
> +/// };
> +///
> +/// // SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip
> +/// // transmutable to `i32`.
> +/// unsafe impl AllowAtomic for State {
> +///     type Repr = i32;
> +/// }
> +///
> +/// let s = Atomic::new(State::Uninit);
> +///
> +/// assert_eq!(State::Uninit, s.load(Relaxed));
> +/// ```

This example doesn't really seem like a good idea on this type... Maybe
on `AllowAtomic` instead? This type should just have examples of how to
use `Atomic<T>`.

> +///
> +/// # Guarantees
> +///
> +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race,
> +/// this is guaranteed by the safety requirement of [`Self::from_ptr()`] and the extra safety
> +/// requirement of the usage on pointers returned by [`Self::as_ptr()`].

I'd rather think we turn this into an invariant that says any operations
on `self.0` through a shared reference is atomic.

> +#[repr(transparent)]
> +pub struct Atomic<T: AllowAtomic>(UnsafeCell<T>);
> +
> +// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> +unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
> +
> +/// Types that support basic atomic operations.
> +///
> +/// # Round-trip transmutability

This can stay here for the moment, but it should probably be somewhere
more central.

> +///
> +/// Implementing [`AllowAtomic`] requires that the type is round-trip transmutable to its
> +/// representation:

I would remove this sentence and just define round-trip transmutability
in a standalone fashion.

> +///
> +/// - Any value of [`Self`] must be sound to [`transmute()`] to a [`Self::Repr`], and this also
> +///   means that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`] for reading.
> +/// - If a value of [`Self::Repr`] is a result a [`transmute()`] from a [`Self`], it must be
> +///   sound to [`transmute()`] the value back to a [`Self`].

This seems a bit verbose. How about this:

    `T` is round-trip transmutable to `U` if and only if all of these properties hold:
    * Any valid bit pattern for `T` is also a valid bit pattern for `U`.
    * Transmuting a value of type `T` to `U` and then to `T` again yields a value that is in all aspects
      equivalent to the original value.

> +///
> +/// This essentially means a valid bit pattern of `T: AllowAtomic` has to be a valid bit pattern
> +/// of `T::Repr`. This is needed because [`Atomic<T: AllowAtomic>`] operates on `T::Repr` to
> +/// implement atomic operations on `T`.
> +///
> +/// Note that this is more relaxed than bidirectional transmutability (i.e. [`transmute()`] is
> +/// always sound between `T` and `T::Repr`) because of the support for atomic variables over

s/between `T` and `T::Repr`/from `T` to `T::Repr` and back/

> +/// unit-only enums:

What are "unit-only" enums? Do you mean enums that don't have associated
data?

> +///
> +/// ```
> +/// #[repr(i32)]
> +/// enum State { Init = 0, Working = 1, Done = 2, }
> +/// ```
> +///
> +/// # Safety
> +///
> +/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
> +/// - [`Self`] and [`Self::Repr`] must have the [round-trip transmutability].

s/have the/be/
s/transmutability/transmutable/

> +///
> +/// # Limitations
> +///
> +/// Because C primitives are used to implement the atomic operations, and a C function requires a
> +/// valid object of a type to operate on (i.e. no `MaybeUninit<_>`), hence at the Rust <-> C
> +/// surface, only types with no uninitialized bits can be passed. As a result, types like `(u8,
> +/// u16)` (a tuple with a `MaybeUninit` hole in it) are currently not supported. Note that
> +/// technically these types can be supported if some APIs are removed for them and the inner
> +/// implementation is tweaked, but the justification of support such a type is not strong enough at
> +/// the moment. This should be resolved if there is an implementation for `MaybeUninit<i32>` as
> +/// `AtomicImpl`.
> +///
> +/// [`transmute()`]: core::mem::transmute
> +/// [round-trip transmutability]: AllowAtomic#round-trip-transmutability
> +pub unsafe trait AllowAtomic: Sized + Send + Copy {
> +    /// The backing atomic implementation type.
> +    type Repr: AtomicImpl;
> +}
> +
> +#[inline(always)]
> +const fn into_repr<T: AllowAtomic>(v: T) -> T::Repr {
> +    // SAFETY: Per the safety requirement of `AllowAtomic`, the transmute operation is sound.
> +    unsafe { core::mem::transmute_copy(&v) }
> +}
> +
> +/// # Safety
> +///
> +/// `r` must be a valid bit pattern of `T`.
> +#[inline(always)]
> +const unsafe fn from_repr<T: AllowAtomic>(r: T::Repr) -> T {
> +    // SAFETY: Per the safety requirement of the function, the transmute operation is sound.
> +    unsafe { core::mem::transmute_copy(&r) }
> +}
> +
> +impl<T: AllowAtomic> Atomic<T> {
> +    /// Creates a new atomic.

s/atomic/atomic `T`/

> +    pub const fn new(v: T) -> Self {
> +        Self(UnsafeCell::new(v))
> +    }
> +
> +    /// Creates a reference to [`Self`] from a pointer.

s/[`Self`]/an atomic `T`/

> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` has to be a valid pointer.

s/has to be a/is/
s/pointer//

> +    /// - `ptr` has to be valid for both reads and writes for the whole lifetime `'a`.

s/has to be/is/
s/both//
s/the whole lifetime//

> +    /// - For the duration of `'a`, other accesses to the object cannot cause data races (defined

s/the object/`*ptr`/
s/cannot/must not/

> +    ///   by [`LKMM`]) against atomic operations on the returned reference. Note that if all other
> +    ///   accesses are atomic, then this safety requirement is trivially fulfilled.
> +    ///
> +    /// [`LKMM`]: srctree/tools/memory-model
> +    ///
> +    /// # Examples
> +    ///
> +    /// Using [`Atomic::from_ptr()`] combined with [`Atomic::load()`] or [`Atomic::store()`] can
> +    /// achieve the same functionality as `READ_ONCE()`/`smp_load_acquire()` or
> +    /// `WRITE_ONCE()`/`smp_store_release()` in C side:
> +    ///
> +    /// ```rust

`rust` is the default, so you can just omit it.

> +    /// # use kernel::types::Opaque;
> +    /// use kernel::sync::atomic::{Atomic, Relaxed, Release};
> +    ///
> +    /// // Assume there is a C struct `Foo`.

s/F/f/

> +    /// mod cbindings {
> +    ///     #[repr(C)]
> +    ///     pub(crate) struct foo { pub(crate) a: i32, pub(crate) b: i32 }

Why not format this normally?

> +    /// }
> +    ///
> +    /// let tmp = Opaque::new(cbindings::foo { a: 1, b: 2});

Missing space before `}`.

> +    ///
> +    /// // struct foo *foo_ptr = ..;
> +    /// let foo_ptr = tmp.get();
> +    ///
> +    /// // SAFETY: `foo_ptr` is a valid pointer, and `.a` is in bounds.
> +    /// let foo_a_ptr = unsafe { &raw mut (*foo_ptr).a };
> +    ///
> +    /// // a = READ_ONCE(foo_ptr->a);
> +    /// //
> +    /// // SAFETY: `foo_a_ptr` is a valid pointer for read, and all accesses on it is atomic, so no
> +    /// // data race.
> +    /// let a = unsafe { Atomic::from_ptr(foo_a_ptr) }.load(Relaxed);
> +    /// # assert_eq!(a, 1);
> +    ///
> +    /// // smp_store_release(&foo_ptr->a, 2);
> +    /// //
> +    /// // SAFETY: `foo_a_ptr` is a valid pointer for write, and all accesses on it is atomic, so
> +    /// // no data race.
> +    /// unsafe { Atomic::from_ptr(foo_a_ptr) }.store(2, Release);
> +    /// ```
> +    ///
> +    /// However, this should be only used when communicating with C side or manipulating a C struct.
> +    pub unsafe fn from_ptr<'a>(ptr: *mut T) -> &'a Self
> +    where
> +        T: Sync,
> +    {
> +        // CAST: `T` is transparent to `Atomic<T>`.
> +        // SAFETY: Per function safety requirement, `ptr` is a valid pointer and the object will
> +        // live long enough. It's safe to return a `&Atomic<T>` because function safety requirement
> +        // guarantees other accesses won't cause data races.
> +        unsafe { &*ptr.cast::<Self>() }
> +    }
> +
> +    /// Returns a pointer to the underlying atomic variable.
> +    ///
> +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
> +    /// cannot cause data races defined by [`LKMM`].

I don't think this is correct. I could create an atomic and then share
it with the C side via this function, since I have exclusive access, the
writes to this pointer don't need to be atomic.

We also don't document additional postconditions like this... If you
really would have to do it like this (which you shouldn't given the
example above), you would have to make this function `unsafe`, otherwise
there is no way to ensure that people adhere to it (since it isn't part
of the safety docs).

> +    ///
> +    /// [`LKMM`]: srctree/tools/memory-model
> +    pub const fn as_ptr(&self) -> *mut T {
> +        self.0.get()
> +    }
> +
> +    /// Returns a mutable reference to the underlying atomic variable.
> +    ///
> +    /// This is safe because the mutable reference of the atomic variable guarantees the exclusive
> +    /// access.
> +    pub fn get_mut(&mut self) -> &mut T {
> +        // SAFETY: `self.as_ptr()` is a valid pointer to `T`. `&mut self` guarantees the exclusive
> +        // access, so it's safe to reborrow mutably.
> +        unsafe { &mut *self.as_ptr() }
> +    }
> +}
> +
> +impl<T: AllowAtomic> Atomic<T>
> +where
> +    T::Repr: AtomicHasBasicOps,
> +{
> +    /// Loads the value from the atomic variable.

s/variable/`T`/

> +    ///
> +    /// # Examples
> +    ///
> +    /// Simple usages:

I would remove this.

> +    ///
> +    /// ```rust
> +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> +    ///
> +    /// let x = Atomic::new(42i32);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    ///
> +    /// let x = Atomic::new(42i64);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    /// ```
> +    #[doc(alias("atomic_read", "atomic64_read"))]
> +    #[inline(always)]
> +    pub fn load<Ordering: AcquireOrRelaxed>(&self, _: Ordering) -> T {
> +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
> +        // valid pointer of `T::Repr`.

Well not exactly... The cast is fine due to the round-trip
transmutability, but you're not allowed to write arbitrary values to it.
Only values that are transmutable to `T`. So it is valid for reads and
valid for writes of values transmutable to `T`.

> +        let a = self.as_ptr().cast::<T::Repr>();
> +
> +        // SAFETY:
> +        // - For calling the atomic_read*() function:
> +        //   - `a` is a valid pointer for the function per the CAST justification above.
> +        //   - Per the type guarantees, the following atomic operation won't cause data races.

Which type guarantees? `Self`?

> +        // - For extra safety requirement of usage on pointers returned by `self.as_ptr()`:
> +        //   - Atomic operations are used here.
> +        let v = unsafe {
> +            match Ordering::TYPE {
> +                OrderingType::Relaxed => T::Repr::atomic_read(a),
> +                OrderingType::Acquire => T::Repr::atomic_read_acquire(a),
> +                _ => build_error!("Wrong ordering"),
> +            }
> +        };
> +
> +        // SAFETY: The atomic variable holds a valid `T`, so `v` is a valid bit pattern of `T`,
> +        // therefore it's safe to call `from_repr()`.

    // SAFETY: `v` comes from reading `a` which was derived from `self.as_ptr()` which points at a
    // valid `T`.

> +        unsafe { from_repr(v) }
> +    }
> +
> +    /// Stores a value to the atomic variable.

s/variable/`T`/

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```rust
> +    /// use kernel::sync::atomic::{Atomic, Relaxed};
> +    ///
> +    /// let x = Atomic::new(42i32);
> +    ///
> +    /// assert_eq!(42, x.load(Relaxed));
> +    ///
> +    /// x.store(43, Relaxed);
> +    ///
> +    /// assert_eq!(43, x.load(Relaxed));
> +    /// ```
> +    #[doc(alias("atomic_set", "atomic64_set"))]
> +    #[inline(always)]
> +    pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
> +        let v = into_repr(v);
> +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
> +        // valid pointer of `T::Repr`.

Ditto.

> +        let a = self.as_ptr().cast::<T::Repr>();
> +
> +        // SAFETY:
> +        // - For calling the atomic_set*() function:
> +        //   - `a` is a valid pointer for the function per the CAST justification above.
> +        //   - Per the type guarantees, 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.
> +        // - For the bit validity of `Atomic<T>`:
> +        //   - `v` is a valid bit pattern of `T`, so it's sound to store it in an `Atomic<T>`.

Ditto.

---
Cheers,
Benno

> +        unsafe {
> +            match Ordering::TYPE {
> +                OrderingType::Relaxed => T::Repr::atomic_set(a, v),
> +                OrderingType::Release => T::Repr::atomic_set_release(a, v),
> +                _ => build_error!("Wrong ordering"),
> +            }
> +        };
> +    }
> +}

Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

[Benno Lossin]

On Thu Jul 10, 2025 at 10:29 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 09:21:17PM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 6:16 PM CEST, Boqun Feng wrote:
>> > At least the rustdoc has safety section for each function. ;-)
>> 
>> I don't usually use rustdoc to read function safety sections. Instead I
>> jump to their definition and read the code.
>> 
>
> Understood. It's probalby the first time we use macros to generate a few
> unsafe functions, so this is something new.
>
> But let me ask you a question, as a programmer yourself, when you run
> into a code base, and see something repeat in a similar pattern for 10+
> times, what's your instinct? You would try to combine the them together,
> right? That's why duplication seems not compelling to me. But surely, we
> don't need to draw conclusion right now, however that's my opinion.

It all depends, if the definition never changes for a long time, I don't
mind the duplication. It's probably more effort to write macros to then
have less overall code.

It also makes it harder to read especially wrt safety docs. So if you
think you will have to make constant tweaks & additions to these, then
we should go a macro-heavy route. But if not, then I think making it
more readable is the way to go.

---
Cheers,
Benno

Re: [PATCH v6 5/9] rust: sync: atomic: Add atomic {cmp,}xchg operations

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> xchg() and cmpxchg() are basic operations on atomic. Provide these based
> on C APIs.
>
> Note that cmpxchg() use the similar function signature as
> compare_exchange() in Rust std: returning a `Result`, `Ok(old)` means
> the operation succeeds and `Err(old)` means the operation fails.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/sync/atomic/generic.rs | 170 +++++++++++++++++++++++++++++
>  1 file changed, 170 insertions(+)
>
> diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> index e044fe21b128..1beb802843ee 100644
> --- a/rust/kernel/sync/atomic/generic.rs
> +++ b/rust/kernel/sync/atomic/generic.rs
> @@ -287,3 +287,173 @@ pub fn store<Ordering: ReleaseOrRelaxed>(&self, v: T, _: Ordering) {
>          };
>      }
>  }
> +
> +impl<T: AllowAtomic> Atomic<T>
> +where
> +    T::Repr: AtomicHasXchgOps,
> +{
> +    /// Atomic exchange.

Please also give a longer sentence describing the operation.

> +    ///
> +    /// # 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", "swap"))]
> +    #[inline(always)]
> +    pub fn xchg<Ordering: Any>(&self, v: T, _: Ordering) -> T {
> +        let v = into_repr(v);
> +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is also a
> +        // valid pointer of `T::Repr`.

Ditto as the last patch (I'm not going to mention the others).

> +        let a = self.as_ptr().cast::<T::Repr>();
> +
> +        // SAFETY:
> +        // - For calling the atomic_xchg*() function:
> +        //   - `a` is a valid pointer for the function per the CAST justification above.
> +        //   - Per the type guarantees, 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.
> +        // - For the bit validity of `Atomic<T>`:
> +        //   - `v` is a valid bit pattern of `T`, so it's sound to store it in an `Atomic<T>`.
> +        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),
> +            }
> +        };
> +
> +        // SAFETY: The atomic variable holds a valid `T`, so `ret` is a valid bit pattern of `T`,
> +        // therefore it's safe to call `from_repr()`.
> +        unsafe { from_repr(ret) }
> +    }
> +
> +    /// Atomic compare and exchange.

Also longer description for this function.

> +    ///
> +    /// 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.

Why did you chose to say "should be treated" can't you say it is a
relaxed read? What would the difference be between those two be?

> +    ///
> +    /// Returns `Ok(value)` if cmpxchg succeeds, and `value` is guaranteed to be equal to `old`,
> +    /// otherwise returns `Err(value)`, and `value` is the value of the atomic variable when
> +    /// cmpxchg was happening.

s/cmpxchg was happening/`cmpxchg` was executed/

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```rust
> +    /// use kernel::sync::atomic::{Atomic, Full, Relaxed};
> +    ///
> +    /// let x = Atomic::new(42);
> +    ///
> +    /// // Checks whether cmpxchg succeeded.
> +    /// let success = x.cmpxchg(52, 64, Relaxed).is_ok();
> +    /// # assert!(!success);
> +    ///
> +    /// // Checks whether cmpxchg failed.
> +    /// let failure = x.cmpxchg(52, 64, Relaxed).is_err();
> +    /// # assert!(failure);
> +    ///
> +    /// // Uses the old value if failed, probably re-try cmpxchg.
> +    /// match x.cmpxchg(52, 64, Relaxed) {
> +    ///     Ok(_) => { },
> +    ///     Err(old) => {
> +    ///         // do something with `old`.
> +    ///         # assert_eq!(old, 42);
> +    ///     }
> +    /// }
> +    ///
> +    /// // Uses the latest value regardlessly, same as atomic_cmpxchg() in C.
> +    /// let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old);
> +    /// # assert_eq!(42, latest);
> +    /// assert_eq!(64, x.load(Relaxed));
> +    /// ```
> +    #[doc(alias(
> +        "atomic_cmpxchg",
> +        "atomic64_cmpxchg",
> +        "atomic_try_cmpxchg",
> +        "atomic64_try_cmpxchg",
> +        "compare_exchange"
> +    ))]
> +    #[inline(always)]
> +    pub fn cmpxchg<Ordering: Any>(&self, mut old: T, new: T, o: Ordering) -> Result<T, T> {
> +        // Note on code generation:
> +        //
> +        // try_cmpxchg() is used to implement cmpxchg(), and if the helper functions are inlined,
> +        // the compiler is able to figure out that branch is not needed if the users don't care
> +        // about whether the operation succeeds or not. One exception is on x86, due to commit
> +        // 44fe84459faf ("locking/atomic: Fix atomic_try_cmpxchg() semantics"), the
> +        // atomic_try_cmpxchg() on x86 has a branch even if the caller doesn't care about the
> +        // success of cmpxchg and only wants to use the old value. For example, for code like:
> +        //
> +        //     let latest = x.cmpxchg(42, 64, Full).unwrap_or_else(|old| old);
> +        //
> +        // It will still generate code:
> +        //
> +        //     movl    $0x40, %ecx
> +        //     movl    $0x34, %eax
> +        //     lock
> +        //     cmpxchgl        %ecx, 0x4(%rsp)
> +        //     jne     1f
> +        //     2:
> +        //     ...
> +        //     1:  movl    %eax, %ecx
> +        //     jmp 2b
> +        //
> +        // This might be "fixed" by introducing a try_cmpxchg_exclusive() that knows the "*old"
> +        // location in the C function is always safe to write.

Oh wow the mentioned commit was an interesting read...

---
Cheers,
Benno

> +        if self.try_cmpxchg(&mut old, new, o) {
> +            Ok(old)
> +        } else {
> +            Err(old)
> +        }
> +    }

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> One important set of atomic operations is the arithmetic operations,
> i.e. add(), sub(), fetch_add(), add_return(), etc. However it may not
> make senses for all the types that `AllowAtomic` to have arithmetic
> operations, for example a `Foo(u32)` may not have a reasonable add() or
> sub(), plus subword types (`u8` and `u16`) currently don't have
> atomic arithmetic operations even on C side and might not have them in
> the future in Rust (because they are usually suboptimal on a few
> architecures). Therefore add a subtrait of `AllowAtomic` describing
> which types have and can do atomic arithemtic operations.
>
> Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
> using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
> `i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.
>
> Only add() and fetch_add() are added. The rest will be added in the
> future.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/sync/atomic.rs         |  18 +++++
>  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)

I think it's better to name this trait `AtomicAdd` and make it generic:

    pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
        fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
    }

`sub` and `fetch_sub` can be added using a similar trait.

The generic allows you to implement it multiple times with different
meanings, for example:

    pub struct Nanos(u64);
    pub struct Micros(u64);
    pub struct Millis(u64);

    impl AllowAtomic for Nanos {
        type Repr = i64;
    }

    impl AtomicAdd<Millis> for Nanos {
        fn rhs_into_repr(rhs: Millis) -> i64 {
            transmute(rhs.0 * 1000_000)
        }
    }

    impl AtomicAdd<Micros> for Nanos {
        fn rhs_into_repr(rhs: Micros) -> i64 {
            transmute(rhs.0 * 1000)
        }
    }

    impl AtomicAdd<Nanos> for Nanos {
        fn rhs_into_repr(rhs: Nanos) -> i64 {
            transmute(rhs.0)
        }
    }

For the safety requirement on the `AtomicAdd` trait, we might just
require bi-directional transmutability... Or can you imagine a case
where that is not guaranteed, but a weaker form is?

---
Cheers,
Benno

Re: [PATCH v6 7/9] rust: sync: atomic: Add Atomic<u{32,64}>

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> Add generic atomic support for basic unsigned types that have an
> `AtomicImpl` with the same size and alignment.
>
> Unit tests are added including Atomic<i32> and Atomic<i64>.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/sync/atomic.rs | 99 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
> new file mode 100644
> index 000000000000..df4015221503
> --- /dev/null
> +++ b/rust/kernel/sync/barrier.rs
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Memory barriers.
> +//!
> +//! These primitives have the same semantics as their C counterparts: and the precise definitions
> +//! of semantics can be found at [`LKMM`].
> +//!
> +//! [`LKMM`]: srctree/tools/memory-model/
> +
> +/// A compiler barrier.
> +///
> +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
> +pub(crate) fn barrier() {
> +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
> +    // it suffices as a compiler barrier.

I don't know about this, but it also isn't my area of expertise... I
think I heard Ralf talk about this at Rust Week, but I don't remember...

> +    //
> +    // SAFETY: An empty asm block should be safe.

    // SAFETY: An empty asm block.

> +    unsafe {
> +        core::arch::asm!("");
> +    }

    unsafe { core::arch::asm!("") };

> +}
> +
> +/// A full memory barrier.
> +///
> +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> +pub fn smp_mb() {
> +    if cfg!(CONFIG_SMP) {
> +        // SAFETY: `smp_mb()` is safe to call.
> +        unsafe {
> +            bindings::smp_mb();

Does this really work? How does the Rust compiler know this is a memory
barrier?

---
Cheers,
Benno

> +        }
> +    } else {
> +        barrier();
> +    }
> +}

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Benno Lossin]

On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> Add generic atomic support for `usize` and `isize`. Note that instead of
> mapping directly to `atomic_long_t`, the represention type
> (`AllowAtomic::Repr`) is selected based on CONFIG_64BIT. This reduces
> the necessity of creating `atomic_long_*` helpers, which could save
> the binary size of kernel if inline helpers are not available.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Benno Lossin <lossin@kernel.org>

> ---
>  rust/kernel/sync/atomic.rs | 48 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
> index e676bc7d9275..e1e40757d7b5 100644
> --- a/rust/kernel/sync/atomic.rs
> +++ b/rust/kernel/sync/atomic.rs
> @@ -53,6 +53,26 @@ fn delta_into_repr(d: Self::Delta) -> Self::Repr {
>      }
>  }
>  
> +// SAFETY: For 32bit kernel, `isize` has the same size and alignment with `i32` and is round-trip
> +// transmutable to it, for 64bit kernel `isize` has the same size and alignment with `i64` and is
> +// round-trip transmutable to it.
> +unsafe impl generic::AllowAtomic for isize {
> +    #[cfg(not(CONFIG_64BIT))]
> +    type Repr = i32;
> +    #[cfg(CONFIG_64BIT)]
> +    type Repr = i64;

Do we have a static assert with these cfgs that `isize` has the same
size as these?

If not, then it would probably make sense to add them now.

---
Cheers,
Benno

> +}

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
[...]
> > +
> > +    /// Returns a pointer to the underlying atomic variable.
> > +    ///
> > +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
> > +    /// cannot cause data races defined by [`LKMM`].
> 
> I don't think this is correct. I could create an atomic and then share
> it with the C side via this function, since I have exclusive access, the
> writes to this pointer don't need to be atomic.
> 

that's why it says "the operations done via the pointer cannot cause
data races .." instead of saying "it must be atomic".

> We also don't document additional postconditions like this... If you

Please see how Rust std document their `as_ptr()`:

	https://doc.rust-lang.org/std/sync/atomic/struct.AtomicI32.html#method.as_ptr

It mentions that "Doing non-atomic reads and writes on the resulting
integer can be a data race." (although the document is a bit out of
date, since non-atomic read and atomic read are no longer data race now,
see [1])

I think we can use the similar document structure here: providing more
safety requirement on the returning pointers, and...

> really would have to do it like this (which you shouldn't given the
> example above), you would have to make this function `unsafe`, otherwise
> there is no way to ensure that people adhere to it (since it isn't part
> of the safety docs).
> 

...since dereferencing pointers is always `unsafe`, users need to avoid
data races anyway, hence this is just additional information that helps
reasoning.

Regards,
Boqun

> > +    ///
> > +    /// [`LKMM`]: srctree/tools/memory-model
> > +    pub const fn as_ptr(&self) -> *mut T {
> > +        self.0.get()
> > +    }
[...]

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
[...]
> > +}
> > +
> > +/// A full memory barrier.
> > +///
> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> > +pub fn smp_mb() {
> > +    if cfg!(CONFIG_SMP) {
> > +        // SAFETY: `smp_mb()` is safe to call.
> > +        unsafe {
> > +            bindings::smp_mb();
> 
> Does this really work? How does the Rust compiler know this is a memory
> barrier?
> 

- Without INLINE_HELPER, this is an FFI call, it's safe to assume that
  Rust compiler would treat it as a compiler barrier and in smp_mb() a
  real memory barrier instruction will be executed. 

- With INLINE_HELPER, this will be inlined as an asm block with "memory"
  as clobber, and LLVM will know it's a compiler memory barrier, and the
  real memory barrier instruction guarantees it's a memory barrier at
  CPU reordering level as well.

Think about this, SpinLock and Mutex need memory barriers for critical
section, if this doesn't work, then SpinLock and Mutex don't work
either, then we have a bigger problem ;-)

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > +        }
> > +    } else {
> > +        barrier();
> > +    }
> > +}

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Benno Lossin]

On Fri Jul 11, 2025 at 3:22 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
> [...]
>> > +
>> > +    /// Returns a pointer to the underlying atomic variable.
>> > +    ///
>> > +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
>> > +    /// cannot cause data races defined by [`LKMM`].
>> 
>> I don't think this is correct. I could create an atomic and then share
>> it with the C side via this function, since I have exclusive access, the
>> writes to this pointer don't need to be atomic.
>> 
>
> that's why it says "the operations done via the pointer cannot cause
> data races .." instead of saying "it must be atomic".

Ah right I misread... But then the safety requirement is redundant? Data
races are already UB...

>> We also don't document additional postconditions like this... If you
>
> Please see how Rust std document their `as_ptr()`:
>
> 	https://doc.rust-lang.org/std/sync/atomic/struct.AtomicI32.html#method.as_ptr
>
> It mentions that "Doing non-atomic reads and writes on the resulting
> integer can be a data race." (although the document is a bit out of
> date, since non-atomic read and atomic read are no longer data race now,
> see [1])

That's very different from the comment you wrote though. It's not an
additional safety requirement, but rather a note to users of the API
that they should be careful with the returned pointer.

> I think we can use the similar document structure here: providing more
> safety requirement on the returning pointers, and...
>
>> really would have to do it like this (which you shouldn't given the
>> example above), you would have to make this function `unsafe`, otherwise
>> there is no way to ensure that people adhere to it (since it isn't part
>> of the safety docs).
>> 
>
> ...since dereferencing pointers is always `unsafe`, users need to avoid
> data races anyway, hence this is just additional information that helps
> reasoning.

I disagree.

As mentioned above, data races are already forbidden for raw pointers.
We should indeed add a note that says that non-atomic operations might
result in data races. But that's very different from adding an
additional safety requirement for using the pointer.

And I don't think that we can add additional safety requirements to
dereferencing a raw pointer without an additional `unsafe` block.

---
Cheers,
Benno

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Miguel Ojeda]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On Fri, Jul 11, 2025 at 11:00 AM Benno Lossin <lossin@kernel.org> wrote:
>
> Do we have a static assert with these cfgs that `isize` has the same
> size as these?
>
> If not, then it would probably make sense to add them now.

Yeah, according to e.g. Matthew et al., we may end up with 128-bit
pointers in the kernel fairly soon (e.g. a decade):

    https://lwn.net/Articles/908026/

I rescued part of what I wrote in the old `mod assumptions` which I
never got to send back then -- most of the `static_asserts` are
redundant now that we define directly the types in the `ffi` crate (I
mean, we could still assert that `size_of::<c_char>() == 1` and so on,
but they are essentially a tautology now), so I adapted the comments.
Please see below (draft).

Cheers,
Miguel

[-- Attachment #2: 0001-rust-ffi-assert-sizes-and-clarify-128-bit-situation.patch --]
[-- Type: text/x-patch, Size: 2026 bytes --]

From afd58f3808bd41cfb92bf1acdf2a19081a439ca3 Mon Sep 17 00:00:00 2001
From: Miguel Ojeda <ojeda@kernel.org>
Date: Fri, 11 Jul 2025 15:27:27 +0200
Subject: [PATCH] rust: ffi: assert sizes and clarify 128-bit situation

Link: https://lwn.net/Articles/908026/
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/ffi.rs | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/rust/ffi.rs b/rust/ffi.rs
index d60aad792af4..bbda56c28ca8 100644
--- a/rust/ffi.rs
+++ b/rust/ffi.rs
@@ -38,11 +38,43 @@ macro_rules! alias {
 
     // In the kernel, `intptr_t` is defined to be `long` in all platforms, so we can map the type to
     // `isize`.
+    //
+    // It is likely that the assumption that `long` is the size of a CPU register/pointer will stay
+    // when support for 128-bit architectures is added, thus these will still mapped to `{i,u}size`.
     c_long = isize;
     c_ulong = usize;
 
+    // Since `long` will likely be 128-bit for 128-bit architectures, `long long` will likely be
+    // increased. Thus these may happen to be either 64-bit or 128-bit in the future, and thus new
+    // code should avoid relying on them being 64-bit.
     c_longlong = i64;
     c_ulonglong = u64;
 }
 
+// Thus, `long` may be 32-bit, 64-bit or 128-bit.
+const _: () = {
+    assert!(
+        core::mem::size_of::<c_long>()
+            == if cfg!(CONFIG_128BIT) {
+                core::mem::size_of::<u128>()
+            } else if cfg!(CONFIG_64BIT) {
+                core::mem::size_of::<u64>()
+            } else {
+                core::mem::size_of::<u32>()
+            }
+    );
+};
+
+// And `long long` may be 64-bit or 128-bit.
+const _: () = {
+    assert!(
+        core::mem::size_of::<c_longlong>()
+            == if cfg!(CONFIG_64BIT) {
+                core::mem::size_of::<u64>()
+            } else {
+                core::mem::size_of::<u128>()
+            }
+    );
+};
+
 pub use core::ffi::c_void;

base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
-- 
2.50.1

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

On Fri, Jul 11, 2025 at 03:34:47PM +0200, Benno Lossin wrote:
> On Fri Jul 11, 2025 at 3:22 PM CEST, Boqun Feng wrote:
> > On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
> > [...]
> >> > +
> >> > +    /// Returns a pointer to the underlying atomic variable.
> >> > +    ///
> >> > +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
> >> > +    /// cannot cause data races defined by [`LKMM`].
> >> 
> >> I don't think this is correct. I could create an atomic and then share
> >> it with the C side via this function, since I have exclusive access, the
> >> writes to this pointer don't need to be atomic.
> >> 
> >
> > that's why it says "the operations done via the pointer cannot cause
> > data races .." instead of saying "it must be atomic".
> 
> Ah right I misread... But then the safety requirement is redundant? Data
> races are already UB...
> 
> >> We also don't document additional postconditions like this... If you
> >
> > Please see how Rust std document their `as_ptr()`:
> >
> > 	https://doc.rust-lang.org/std/sync/atomic/struct.AtomicI32.html#method.as_ptr
> >
> > It mentions that "Doing non-atomic reads and writes on the resulting
> > integer can be a data race." (although the document is a bit out of
> > date, since non-atomic read and atomic read are no longer data race now,
> > see [1])
> 
> That's very different from the comment you wrote though. It's not an
> additional safety requirement, but rather a note to users of the API
> that they should be careful with the returned pointer.
> 
> > I think we can use the similar document structure here: providing more
> > safety requirement on the returning pointers, and...
> >
> >> really would have to do it like this (which you shouldn't given the
> >> example above), you would have to make this function `unsafe`, otherwise
> >> there is no way to ensure that people adhere to it (since it isn't part
> >> of the safety docs).
> >> 
> >
> > ...since dereferencing pointers is always `unsafe`, users need to avoid
> > data races anyway, hence this is just additional information that helps
> > reasoning.
> 
> I disagree.
> 
> As mentioned above, data races are already forbidden for raw pointers.
> We should indeed add a note that says that non-atomic operations might
> result in data races. But that's very different from adding an
> additional safety requirement for using the pointer.
> 
> And I don't think that we can add additional safety requirements to
> dereferencing a raw pointer without an additional `unsafe` block.
> 

So all your disagreement is about the "extra safety requirement" part?
How about I drop that:

    /// Returns a pointer to the underlying atomic `T`.
    pub const fn as_ptr(&self) -> *mut T {
        self.0.get()
    }

? I tried to add something additional information:

/// Note that non-atomic reads and writes via the returned pointer may
/// cause data races if racing with atomic reads and writes per [LKMM].

but that seems redundant, because as you said, data races are UB anyway.

Thoughts?

Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> > new file mode 100644
> > index 000000000000..e044fe21b128
> > --- /dev/null
> > +++ b/rust/kernel/sync/atomic/generic.rs
> > @@ -0,0 +1,289 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Generic atomic primitives.
> > +
> > +use super::ops::*;
> > +use super::ordering::*;
> > +use crate::build_error;
> > +use core::cell::UnsafeCell;
> > +
> > +/// A generic atomic variable.
[...]
> 
> > +///
> > +/// # Guarantees
> > +///
> > +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race,
> > +/// this is guaranteed by the safety requirement of [`Self::from_ptr()`] and the extra safety
> > +/// requirement of the usage on pointers returned by [`Self::as_ptr()`].
> 
> I'd rather think we turn this into an invariant that says any operations
> on `self.0` through a shared reference is atomic.
> 
[...]
> > +/// unit-only enums:
> 
> What are "unit-only" enums? Do you mean enums that don't have associated
> data?
> 

Yes, I used the term mentioned at:

	https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.unit-only

> > +///
> > +/// ```
> > +/// #[repr(i32)]
> > +/// enum State { Init = 0, Working = 1, Done = 2, }
> > +/// ```
> > +///
> > +/// # Safety
> > +///
> > +/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
> > +/// - [`Self`] and [`Self::Repr`] must have the [round-trip transmutability].
[...]
> > +        let a = self.as_ptr().cast::<T::Repr>();
> > +
> > +        // SAFETY:
> > +        // - For calling the atomic_read*() function:
> > +        //   - `a` is a valid pointer for the function per the CAST justification above.
> > +        //   - Per the type guarantees, the following atomic operation won't cause data races.
> 
> Which type guarantees? `Self`?
> 

The above "# Guarantees" of `Atomic<T>`, but yeah I think it should be
"# Invariants".

> > +        // - For extra safety requirement of usage on pointers returned by `self.as_ptr()`:
> > +        //   - Atomic operations are used here.
> > +        let v = unsafe {
> > +            match Ordering::TYPE {
> > +                OrderingType::Relaxed => T::Repr::atomic_read(a),
> > +                OrderingType::Acquire => T::Repr::atomic_read_acquire(a),
> > +                _ => build_error!("Wrong ordering"),
> > +            }
> > +        };
> > +
> > +        // SAFETY: The atomic variable holds a valid `T`, so `v` is a valid bit pattern of `T`,
> > +        // therefore it's safe to call `from_repr()`.
[...]

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Boqun Feng]

On Fri, Jul 11, 2025 at 03:45:32PM +0200, Miguel Ojeda wrote:
> On Fri, Jul 11, 2025 at 11:00 AM Benno Lossin <lossin@kernel.org> wrote:
> >
> > Do we have a static assert with these cfgs that `isize` has the same
> > size as these?
> >
> > If not, then it would probably make sense to add them now.
> 
> Yeah, according to e.g. Matthew et al., we may end up with 128-bit
> pointers in the kernel fairly soon (e.g. a decade):
> 
>     https://lwn.net/Articles/908026/
> 
> I rescued part of what I wrote in the old `mod assumptions` which I
> never got to send back then -- most of the `static_asserts` are
> redundant now that we define directly the types in the `ffi` crate (I
> mean, we could still assert that `size_of::<c_char>() == 1` and so on,
> but they are essentially a tautology now), so I adapted the comments.
> Please see below (draft).
> 

Thanks Miguel.

Maybe we can do even better: having a type alias mapping to the exact
i{32,64,128} based on kernel configs? Like

(in kernel/lib.rs or ffi.rs)

// Want to buy a better name ;-)
#[cfg(CONFIG_128BIT)]
type isize_mapping = i128;
#[cfg(CONFIG_64BIT)]
type isize_mapping = i64;
#[cfg(not(any(CONFIG_128BIT, CONFIG_64BIT)))]
type isize_mapping = i32;

similar for usize.

Thoughts?

Regards,
Boqun

> Cheers,
> Miguel

> From afd58f3808bd41cfb92bf1acdf2a19081a439ca3 Mon Sep 17 00:00:00 2001
> From: Miguel Ojeda <ojeda@kernel.org>
> Date: Fri, 11 Jul 2025 15:27:27 +0200
> Subject: [PATCH] rust: ffi: assert sizes and clarify 128-bit situation
> 
> Link: https://lwn.net/Articles/908026/
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  rust/ffi.rs | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/rust/ffi.rs b/rust/ffi.rs
> index d60aad792af4..bbda56c28ca8 100644
> --- a/rust/ffi.rs
> +++ b/rust/ffi.rs
> @@ -38,11 +38,43 @@ macro_rules! alias {
>  
>      // In the kernel, `intptr_t` is defined to be `long` in all platforms, so we can map the type to
>      // `isize`.
> +    //
> +    // It is likely that the assumption that `long` is the size of a CPU register/pointer will stay
> +    // when support for 128-bit architectures is added, thus these will still mapped to `{i,u}size`.
>      c_long = isize;
>      c_ulong = usize;
>  
> +    // Since `long` will likely be 128-bit for 128-bit architectures, `long long` will likely be
> +    // increased. Thus these may happen to be either 64-bit or 128-bit in the future, and thus new
> +    // code should avoid relying on them being 64-bit.
>      c_longlong = i64;
>      c_ulonglong = u64;
>  }
>  
> +// Thus, `long` may be 32-bit, 64-bit or 128-bit.
> +const _: () = {
> +    assert!(
> +        core::mem::size_of::<c_long>()
> +            == if cfg!(CONFIG_128BIT) {
> +                core::mem::size_of::<u128>()
> +            } else if cfg!(CONFIG_64BIT) {
> +                core::mem::size_of::<u64>()
> +            } else {
> +                core::mem::size_of::<u32>()
> +            }
> +    );
> +};
> +
> +// And `long long` may be 64-bit or 128-bit.
> +const _: () = {
> +    assert!(
> +        core::mem::size_of::<c_longlong>()
> +            == if cfg!(CONFIG_64BIT) {
> +                core::mem::size_of::<u64>()
> +            } else {
> +                core::mem::size_of::<u128>()
> +            }
> +    );
> +};
> +
>  pub use core::ffi::c_void;
> 
> base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
> -- 
> 2.50.1
> 

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Boqun Feng]

On Fri, Jul 11, 2025 at 10:53:45AM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > One important set of atomic operations is the arithmetic operations,
> > i.e. add(), sub(), fetch_add(), add_return(), etc. However it may not
> > make senses for all the types that `AllowAtomic` to have arithmetic
> > operations, for example a `Foo(u32)` may not have a reasonable add() or
> > sub(), plus subword types (`u8` and `u16`) currently don't have
> > atomic arithmetic operations even on C side and might not have them in
> > the future in Rust (because they are usually suboptimal on a few
> > architecures). Therefore add a subtrait of `AllowAtomic` describing
> > which types have and can do atomic arithemtic operations.
> >
> > Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
> > using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
> > `i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.
> >
> > Only add() and fetch_add() are added. The rest will be added in the
> > future.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/sync/atomic.rs         |  18 +++++
> >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
> >  2 files changed, 126 insertions(+)
> 
> I think it's better to name this trait `AtomicAdd` and make it generic:
> 
>     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>     }
> 
> `sub` and `fetch_sub` can be added using a similar trait.
> 

Seems a good idea, I will see what I can do. Thanks!

> The generic allows you to implement it multiple times with different
> meanings, for example:
> 
>     pub struct Nanos(u64);
>     pub struct Micros(u64);
>     pub struct Millis(u64);
> 
>     impl AllowAtomic for Nanos {
>         type Repr = i64;
>     }
> 
>     impl AtomicAdd<Millis> for Nanos {
>         fn rhs_into_repr(rhs: Millis) -> i64 {
>             transmute(rhs.0 * 1000_000)

We probably want to use `as` in real code?

>         }
>     }
> 
>     impl AtomicAdd<Micros> for Nanos {
>         fn rhs_into_repr(rhs: Micros) -> i64 {
>             transmute(rhs.0 * 1000)
>         }
>     }
> 
>     impl AtomicAdd<Nanos> for Nanos {
>         fn rhs_into_repr(rhs: Nanos) -> i64 {
>             transmute(rhs.0)
>         }
>     }
> 
> For the safety requirement on the `AtomicAdd` trait, we might just
> require bi-directional transmutability... Or can you imagine a case
> where that is not guaranteed, but a weaker form is?

I have a case that I don't think it's that useful, but it's similar to
the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
`i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
is not always sound. Maybe we could add a "TODO" in the safety section
of `AtomicAdd`, and revisit this later? Like:

/// (in # Safety)
/// TODO: The safety requirement may be tightened to bi-directional
/// transmutability. 

And maybe also add the `Even` example there?

Thoughts?

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Miguel Ojeda]

On Fri, Jul 11, 2025 at 4:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Thanks Miguel.
>
> Maybe we can do even better: having a type alias mapping to the exact
> i{32,64,128} based on kernel configs? Like
>
> (in kernel/lib.rs or ffi.rs)
>
> // Want to buy a better name ;-)
> #[cfg(CONFIG_128BIT)]
> type isize_mapping = i128;
> #[cfg(CONFIG_64BIT)]
> type isize_mapping = i64;
> #[cfg(not(any(CONFIG_128BIT, CONFIG_64BIT)))]
> type isize_mapping = i32;
>
> similar for usize.
>
> Thoughts?

Yeah, I wondered about that too, but I don't know how common it will
be (so you may want to keep it local anyway), and I wasn't sure what
to call it either, because e.g. something like `isize_mapping` sounds
like we are talking about `c_long`.

What we want is a Rust fixed-width integer of the same size of `isize`
-- so I think you should try to pick a word that evokes a bit that
part. Something like `fixed_isize` or words like `underlying` or
`repr` perhaps?

Cheers,
Miguel

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Boqun Feng]

On Fri, Jul 11, 2025 at 04:40:15PM +0200, Miguel Ojeda wrote:
> On Fri, Jul 11, 2025 at 4:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Thanks Miguel.
> >
> > Maybe we can do even better: having a type alias mapping to the exact
> > i{32,64,128} based on kernel configs? Like
> >
> > (in kernel/lib.rs or ffi.rs)
> >
> > // Want to buy a better name ;-)
> > #[cfg(CONFIG_128BIT)]
> > type isize_mapping = i128;
> > #[cfg(CONFIG_64BIT)]
> > type isize_mapping = i64;
> > #[cfg(not(any(CONFIG_128BIT, CONFIG_64BIT)))]
> > type isize_mapping = i32;
> >
> > similar for usize.
> >
> > Thoughts?
> 
> Yeah, I wondered about that too, but I don't know how common it will
> be (so you may want to keep it local anyway), and I wasn't sure what

Sounds good, I will put it locally.

> to call it either, because e.g. something like `isize_mapping` sounds
> like we are talking about `c_long`.
> 
> What we want is a Rust fixed-width integer of the same size of `isize`
> -- so I think you should try to pick a word that evokes a bit that
> part. Something like `fixed_isize` or words like `underlying` or
> `repr` perhaps?
> 

I end up calling it `isize_atomic_repr`, and create the diff as below:

------------>8
diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
index 7ff87b2b49d6..bb0d3d49e3f7 100644
--- a/rust/kernel/sync/atomic.rs
+++ b/rust/kernel/sync/atomic.rs
@@ -53,14 +53,21 @@ fn delta_into_repr(d: Self::Delta) -> Self::Repr {
     }
 }

+#[allow(non_camel_case_types)]
+#[cfg(not(CONFIG_64BIT))]
+type isize_atomic_repr = i32;
+#[allow(non_camel_case_types)]
+#[cfg(CONFIG_64BIT)]
+type isize_atomic_repr = i64;
+
+crate::static_assert!(core::mem::size_of::<isize>() == core::mem::size_of::<isize_atomic_repr>());
+crate::static_assert!(core::mem::align_of::<isize>() == core::mem::align_of::<isize_atomic_repr>());
+
 // SAFETY: For 32bit kernel, `isize` has the same size and alignment with `i32` and is round-trip
 // transmutable to it, for 64bit kernel `isize` has the same size and alignment with `i64` and is
 // round-trip transmutable to it.
 unsafe impl generic::AllowAtomic for isize {
-    #[cfg(not(CONFIG_64BIT))]
-    type Repr = i32;
-    #[cfg(CONFIG_64BIT)]
-    type Repr = i64;
+    type Repr = isize_atomic_repr;
 }

 // SAFETY: `isize` is always sound to transmute back from `i32` or `i64` when their sizes are the


Seems good?

Regards,
Boqun

> Cheers,
> Miguel

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Boqun Feng]

On Fri, Jul 11, 2025 at 07:39:15AM -0700, Boqun Feng wrote:
[...]
> > > ---
> > >  rust/kernel/sync/atomic.rs         |  18 +++++
> > >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
> > >  2 files changed, 126 insertions(+)
> > 
> > I think it's better to name this trait `AtomicAdd` and make it generic:
> > 
> >     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
> >         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
> >     }
> > 
> > `sub` and `fetch_sub` can be added using a similar trait.
> > 
> 
> Seems a good idea, I will see what I can do. Thanks!
> 
> > The generic allows you to implement it multiple times with different
> > meanings, for example:
> > 
> >     pub struct Nanos(u64);
> >     pub struct Micros(u64);
> >     pub struct Millis(u64);
> > 
> >     impl AllowAtomic for Nanos {
> >         type Repr = i64;
> >     }
> > 
> >     impl AtomicAdd<Millis> for Nanos {
> >         fn rhs_into_repr(rhs: Millis) -> i64 {
> >             transmute(rhs.0 * 1000_000)
> 
> We probably want to use `as` in real code?
> 
> >         }
> >     }
> > 
> >     impl AtomicAdd<Micros> for Nanos {
> >         fn rhs_into_repr(rhs: Micros) -> i64 {
> >             transmute(rhs.0 * 1000)
> >         }
> >     }
> > 
> >     impl AtomicAdd<Nanos> for Nanos {
> >         fn rhs_into_repr(rhs: Nanos) -> i64 {
> >             transmute(rhs.0)
> >         }
> >     }
> > 
> > For the safety requirement on the `AtomicAdd` trait, we might just
> > require bi-directional transmutability... Or can you imagine a case
> > where that is not guaranteed, but a weaker form is?
> 
> I have a case that I don't think it's that useful, but it's similar to
> the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a

Oh I mis-read, it's not similar to `Micros` or `Millis`, but still,
`Even<i32>` itself should have the point.

> `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
> is not always sound. Maybe we could add a "TODO" in the safety section
> of `AtomicAdd`, and revisit this later? Like:
> 
> /// (in # Safety)
> /// TODO: The safety requirement may be tightened to bi-directional
> /// transmutability. 
> 
> And maybe also add the `Even` example there?
> 
> Thoughts?
> 

Or since we are going to use fine-grained traits, it's actually easy to
define the safety requirement of `AtomicAdd` (instead of
`AllowAtomicArithmetic) now:

    /// # Safety
    ///
    /// For a `T: AtomicAdd<Rhs>`, the addition result of a valid bit
    /// pattern of `T` and a `T::Repr` convert from `rhs_into_repr()` should
    /// be a valid bit pattern of `T`.
    pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
       fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
    }

Thoughts?

Regards,
Boqun

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
> > new file mode 100644
> > index 000000000000..df4015221503
> > --- /dev/null
> > +++ b/rust/kernel/sync/barrier.rs
> > @@ -0,0 +1,65 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Memory barriers.
> > +//!
> > +//! These primitives have the same semantics as their C counterparts: and the precise definitions
> > +//! of semantics can be found at [`LKMM`].
> > +//!
> > +//! [`LKMM`]: srctree/tools/memory-model/
> > +
> > +/// A compiler barrier.
> > +///
> > +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
> > +pub(crate) fn barrier() {
> > +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
> > +    // it suffices as a compiler barrier.
> 
> I don't know about this, but it also isn't my area of expertise... I
> think I heard Ralf talk about this at Rust Week, but I don't remember...
> 

Easy, let's Cc Ralf ;-)

Ralf, I believe the question here is:

In kernel C, we define a compiler barrier (barrier()), which is
implemented as:

# define barrier() __asm__ __volatile__("": : :"memory")

Now we want to have a Rust version, and I think an empty `asm!()` should
be enough as an equivalent as a barrier() in C, because an empty
`asm!()` in Rust implies "memory" as the clobber:

	https://godbolt.org/z/3z3fnWYjs

?

I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
barrier() and other barriers work for all memory accesses not just
atomics, so the problem "So, if your program contains no atomic
accesses, but some atomic fences, those fences do nothing." doesn't
exist for us. And our barrier() is strictly weaker than other barriers.

And based on my understanding of the consensus on Rust vs LKMM, "do
whatever kernel C does and rely on whatever kernel C relies" is the
general suggestion, so I think an empty `asm!()` works here. Of course
if in practice, we find an issue, I'm happy to look for solutions ;-)

Thoughts?

[1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347

Regards,
Boqun

> > +    //
> > +    // SAFETY: An empty asm block should be safe.
> 
>     // SAFETY: An empty asm block.
> 
> > +    unsafe {
> > +        core::arch::asm!("");
> > +    }
> 
>     unsafe { core::arch::asm!("") };
> 
> > +}
> > +
> > +/// A full memory barrier.
> > +///
> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> > +pub fn smp_mb() {
> > +    if cfg!(CONFIG_SMP) {
> > +        // SAFETY: `smp_mb()` is safe to call.
> > +        unsafe {
> > +            bindings::smp_mb();
> 
> Does this really work? How does the Rust compiler know this is a memory
> barrier?
> 
> ---
> Cheers,
> Benno
> 
> > +        }
> > +    } else {
> > +        barrier();
> > +    }
> > +}

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Benno Lossin]

On Fri Jul 11, 2025 at 3:51 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 03:34:47PM +0200, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 3:22 PM CEST, Boqun Feng wrote:
>> > On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
>> > [...]
>> >> > +
>> >> > +    /// Returns a pointer to the underlying atomic variable.
>> >> > +    ///
>> >> > +    /// Extra safety requirement on using the return pointer: the operations done via the pointer
>> >> > +    /// cannot cause data races defined by [`LKMM`].
>> >> 
>> >> I don't think this is correct. I could create an atomic and then share
>> >> it with the C side via this function, since I have exclusive access, the
>> >> writes to this pointer don't need to be atomic.
>> >> 
>> >
>> > that's why it says "the operations done via the pointer cannot cause
>> > data races .." instead of saying "it must be atomic".
>> 
>> Ah right I misread... But then the safety requirement is redundant? Data
>> races are already UB...
>> 
>> >> We also don't document additional postconditions like this... If you
>> >
>> > Please see how Rust std document their `as_ptr()`:
>> >
>> > 	https://doc.rust-lang.org/std/sync/atomic/struct.AtomicI32.html#method.as_ptr
>> >
>> > It mentions that "Doing non-atomic reads and writes on the resulting
>> > integer can be a data race." (although the document is a bit out of
>> > date, since non-atomic read and atomic read are no longer data race now,
>> > see [1])
>> 
>> That's very different from the comment you wrote though. It's not an
>> additional safety requirement, but rather a note to users of the API
>> that they should be careful with the returned pointer.
>> 
>> > I think we can use the similar document structure here: providing more
>> > safety requirement on the returning pointers, and...
>> >
>> >> really would have to do it like this (which you shouldn't given the
>> >> example above), you would have to make this function `unsafe`, otherwise
>> >> there is no way to ensure that people adhere to it (since it isn't part
>> >> of the safety docs).
>> >> 
>> >
>> > ...since dereferencing pointers is always `unsafe`, users need to avoid
>> > data races anyway, hence this is just additional information that helps
>> > reasoning.
>> 
>> I disagree.
>> 
>> As mentioned above, data races are already forbidden for raw pointers.
>> We should indeed add a note that says that non-atomic operations might
>> result in data races. But that's very different from adding an
>> additional safety requirement for using the pointer.
>> 
>> And I don't think that we can add additional safety requirements to
>> dereferencing a raw pointer without an additional `unsafe` block.
>> 
>
> So all your disagreement is about the "extra safety requirement" part?
> How about I drop that:
>
>     /// Returns a pointer to the underlying atomic `T`.
>     pub const fn as_ptr(&self) -> *mut T {
>         self.0.get()
>     }

Yes that's what I had in mind.

> ? I tried to add something additional information:
>
> /// Note that non-atomic reads and writes via the returned pointer may
> /// cause data races if racing with atomic reads and writes per [LKMM].
>
> but that seems redundant, because as you said, data races are UB anyway.

Yeah... I don't think the stdlib docs are too useful on this function:

    Doing non-atomic reads and writes on the resulting integer can be a data
    race. This method is mostly useful for FFI, where the function signature
    may use *mut i32 instead of &AtomicI32.
    
    Returning an *mut pointer from a shared reference to this atomic is safe
    because the atomic types work with interior mutability. All
    modifications of an atomic change the value through a shared reference,
    and can do so safely as long as they use atomic operations. Any use of
    the returned raw pointer requires an unsafe block and still has to
    uphold the same restriction: operations on it must be atomic.

You can mention the use of this function for FFI. People might then be
discouraged from using it for other things where it doesn't make sense.

---
Cheers,
Benno

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Miguel Ojeda]

On Fri, Jul 11, 2025 at 5:47 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Seems good?

Yeah, thanks!

I will send the other one about 128-bit independently.

Cheers,
Miguel

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Benno Lossin]

On Fri Jul 11, 2025 at 3:58 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
>> > new file mode 100644
>> > index 000000000000..e044fe21b128
>> > --- /dev/null
>> > +++ b/rust/kernel/sync/atomic/generic.rs
>> > @@ -0,0 +1,289 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +//! Generic atomic primitives.
>> > +
>> > +use super::ops::*;
>> > +use super::ordering::*;
>> > +use crate::build_error;
>> > +use core::cell::UnsafeCell;
>> > +
>> > +/// A generic atomic variable.
> [...]
>> 
>> > +///
>> > +/// # Guarantees
>> > +///
>> > +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race,
>> > +/// this is guaranteed by the safety requirement of [`Self::from_ptr()`] and the extra safety
>> > +/// requirement of the usage on pointers returned by [`Self::as_ptr()`].
>> 
>> I'd rather think we turn this into an invariant that says any operations
>> on `self.0` through a shared reference is atomic.
>> 
> [...]
>> > +/// unit-only enums:
>> 
>> What are "unit-only" enums? Do you mean enums that don't have associated
>> data?
>> 
>
> Yes, I used the term mentioned at:
>
> 	https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.unit-only

Ahhh, never saw that before :)

>> > +///
>> > +/// ```
>> > +/// #[repr(i32)]
>> > +/// enum State { Init = 0, Working = 1, Done = 2, }
>> > +/// ```
>> > +///
>> > +/// # Safety
>> > +///
>> > +/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
>> > +/// - [`Self`] and [`Self::Repr`] must have the [round-trip transmutability].
> [...]
>> > +        let a = self.as_ptr().cast::<T::Repr>();
>> > +
>> > +        // SAFETY:
>> > +        // - For calling the atomic_read*() function:
>> > +        //   - `a` is a valid pointer for the function per the CAST justification above.
>> > +        //   - Per the type guarantees, the following atomic operation won't cause data races.
>> 
>> Which type guarantees? `Self`?
>> 
>
> The above "# Guarantees" of `Atomic<T>`, but yeah I think it should be
> "# Invariants".

Yeah and if we use invariants/guarantees always mention the type that
they are of and don't assume the reader will "know" :)

---
Cheers,
Benno

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Benno Lossin]

On Fri Jul 11, 2025 at 4:39 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 10:53:45AM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> > One important set of atomic operations is the arithmetic operations,
>> > i.e. add(), sub(), fetch_add(), add_return(), etc. However it may not
>> > make senses for all the types that `AllowAtomic` to have arithmetic
>> > operations, for example a `Foo(u32)` may not have a reasonable add() or
>> > sub(), plus subword types (`u8` and `u16`) currently don't have
>> > atomic arithmetic operations even on C side and might not have them in
>> > the future in Rust (because they are usually suboptimal on a few
>> > architecures). Therefore add a subtrait of `AllowAtomic` describing
>> > which types have and can do atomic arithemtic operations.
>> >
>> > Trait `AllowAtomicArithmetic` has an associate type `Delta` instead of
>> > using `AllowAllowAtomic::Repr` because, a `Bar(u32)` (whose `Repr` is
>> > `i32`) may not wants an `add(&self, i32)`, but an `add(&self, u32)`.
>> >
>> > Only add() and fetch_add() are added. The rest will be added in the
>> > future.
>> >
>> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> > ---
>> >  rust/kernel/sync/atomic.rs         |  18 +++++
>> >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
>> >  2 files changed, 126 insertions(+)
>> 
>> I think it's better to name this trait `AtomicAdd` and make it generic:
>> 
>>     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>>         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>>     }
>> 
>> `sub` and `fetch_sub` can be added using a similar trait.
>> 
>
> Seems a good idea, I will see what I can do. Thanks!
>
>> The generic allows you to implement it multiple times with different
>> meanings, for example:
>> 
>>     pub struct Nanos(u64);
>>     pub struct Micros(u64);
>>     pub struct Millis(u64);
>> 
>>     impl AllowAtomic for Nanos {
>>         type Repr = i64;

By the way, I find this a bit unfortunate... I think it would be nice to
be able to use `u64` and `u32` as reprs too.

Maybe we can add an additional trait `AtomicRepr` that gets implemented
by all integer types and then we can use that in the `Repr` instead.

This should definitely be a future patch series though.

>>     }
>> 
>>     impl AtomicAdd<Millis> for Nanos {
>>         fn rhs_into_repr(rhs: Millis) -> i64 {
>>             transmute(rhs.0 * 1000_000)
>
> We probably want to use `as` in real code?

I thought that `as` would panic on over/underflow... But it doesn't and
indeed just converts between the two same-sized types.

By the way, should we ask for `Repr` to always be of the same size as
`Self` when implementing `AllowAtomic`?

That might already be implied from the round-trip transmutability:
* `Self` can't have a smaller size, because transmuting `Self` into
  `Repr` would result in uninit bytes.
* `Repr` can't have a smaller size, because then transmuting a `Repr`
  (that was once a `Self`) back into `Self` will result in uninit bytes

We probably should mention this in the docs somewhere?

>>         }
>>     }
>> 
>>     impl AtomicAdd<Micros> for Nanos {
>>         fn rhs_into_repr(rhs: Micros) -> i64 {
>>             transmute(rhs.0 * 1000)
>>         }
>>     }
>> 
>>     impl AtomicAdd<Nanos> for Nanos {
>>         fn rhs_into_repr(rhs: Nanos) -> i64 {
>>             transmute(rhs.0)
>>         }
>>     }
>> 
>> For the safety requirement on the `AtomicAdd` trait, we might just
>> require bi-directional transmutability... Or can you imagine a case
>> where that is not guaranteed, but a weaker form is?
>
> I have a case that I don't think it's that useful, but it's similar to
> the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
> `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
> is not always sound. Maybe we could add a "TODO" in the safety section
> of `AtomicAdd`, and revisit this later? Like:
>
> /// (in # Safety)
> /// TODO: The safety requirement may be tightened to bi-directional
> /// transmutability. 
>
> And maybe also add the `Even` example there?

Ahh that's interesting... I don't think the comment in the tightening
direction makes sense, either we start out with bi-directional
transmutability, or we don't do it at all.

I think an `Even` example is motivation enough to have it. So let's not
tighten it. But I think we should improve the safety requirement:

    /// The valid bit patterns of `Self` must be a superset of the bit patterns reachable through
    /// addition on any values of type [`Self::Repr`] obtained by transmuting values of type `Self`.

or
    
    /// Adding any two values of type [`Self::Repr`] obtained through transmuting values of type `Self`
    /// must yield a value with a bit pattern also valid for `Self`.

I feel like the second one sounds better.

Also is overflowing an atomic variable UB in LKMM? Because if it is,
then `struct MultipleOf<const M: u64>(u64)` is also something that would
be supported. Otherwise only powers of two would be supported.

---
Cheers,
Benno

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Benno Lossin]

On Fri Jul 11, 2025 at 3:32 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> [...]
>> > +}
>> > +
>> > +/// A full memory barrier.
>> > +///
>> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
>> > +pub fn smp_mb() {
>> > +    if cfg!(CONFIG_SMP) {
>> > +        // SAFETY: `smp_mb()` is safe to call.
>> > +        unsafe {
>> > +            bindings::smp_mb();
>> 
>> Does this really work? How does the Rust compiler know this is a memory
>> barrier?
>> 
>
> - Without INLINE_HELPER, this is an FFI call, it's safe to assume that
>   Rust compiler would treat it as a compiler barrier and in smp_mb() a
>   real memory barrier instruction will be executed. 
>
> - With INLINE_HELPER, this will be inlined as an asm block with "memory"
>   as clobber, and LLVM will know it's a compiler memory barrier, and the
>   real memory barrier instruction guarantees it's a memory barrier at
>   CPU reordering level as well.
>
> Think about this, SpinLock and Mutex need memory barriers for critical
> section, if this doesn't work, then SpinLock and Mutex don't work
> either, then we have a bigger problem ;-)

By "this not working" I meant that he barrier would be too strong :)

So essentially without INLINE_HELPER, all barriers in this file are the
same, but with it, we get less strict ones?

---
Cheers,
Benno

Re: [PATCH v6 9/9] rust: sync: atomic: Add Atomic<{usize,isize}>

[Benno Lossin]

On Fri Jul 11, 2025 at 3:45 PM CEST, Miguel Ojeda wrote:
> On Fri, Jul 11, 2025 at 11:00 AM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Do we have a static assert with these cfgs that `isize` has the same
>> size as these?
>>
>> If not, then it would probably make sense to add them now.
>
> Yeah, according to e.g. Matthew et al., we may end up with 128-bit
> pointers in the kernel fairly soon (e.g. a decade):
>
>     https://lwn.net/Articles/908026/
>
> I rescued part of what I wrote in the old `mod assumptions` which I
> never got to send back then -- most of the `static_asserts` are
> redundant now that we define directly the types in the `ffi` crate (I
> mean, we could still assert that `size_of::<c_char>() == 1` and so on,
> but they are essentially a tautology now), so I adapted the comments.
> Please see below (draft).

Ahhh yes the `mod assumptions` was the thing I remembered! I think it's
still useful as a file where we can put other global assumptions as
well.

---
Cheers,
Benno

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Benno Lossin]

On Fri Jul 11, 2025 at 7:41 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 07:39:15AM -0700, Boqun Feng wrote:
> [...]
>> > > ---
>> > >  rust/kernel/sync/atomic.rs         |  18 +++++
>> > >  rust/kernel/sync/atomic/generic.rs | 108 +++++++++++++++++++++++++++++
>> > >  2 files changed, 126 insertions(+)
>> > 
>> > I think it's better to name this trait `AtomicAdd` and make it generic:
>> > 
>> >     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>> >         fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>> >     }
>> > 
>> > `sub` and `fetch_sub` can be added using a similar trait.
>> > 
>> 
>> Seems a good idea, I will see what I can do. Thanks!
>> 
>> > The generic allows you to implement it multiple times with different
>> > meanings, for example:
>> > 
>> >     pub struct Nanos(u64);
>> >     pub struct Micros(u64);
>> >     pub struct Millis(u64);
>> > 
>> >     impl AllowAtomic for Nanos {
>> >         type Repr = i64;
>> >     }
>> > 
>> >     impl AtomicAdd<Millis> for Nanos {
>> >         fn rhs_into_repr(rhs: Millis) -> i64 {
>> >             transmute(rhs.0 * 1000_000)
>> 
>> We probably want to use `as` in real code?
>> 
>> >         }
>> >     }
>> > 
>> >     impl AtomicAdd<Micros> for Nanos {
>> >         fn rhs_into_repr(rhs: Micros) -> i64 {
>> >             transmute(rhs.0 * 1000)
>> >         }
>> >     }
>> > 
>> >     impl AtomicAdd<Nanos> for Nanos {
>> >         fn rhs_into_repr(rhs: Nanos) -> i64 {
>> >             transmute(rhs.0)
>> >         }
>> >     }
>> > 
>> > For the safety requirement on the `AtomicAdd` trait, we might just
>> > require bi-directional transmutability... Or can you imagine a case
>> > where that is not guaranteed, but a weaker form is?
>> 
>> I have a case that I don't think it's that useful, but it's similar to
>> the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
>
> Oh I mis-read, it's not similar to `Micros` or `Millis`, but still,
> `Even<i32>` itself should have the point.
>
>> `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
>> is not always sound. Maybe we could add a "TODO" in the safety section
>> of `AtomicAdd`, and revisit this later? Like:
>> 
>> /// (in # Safety)
>> /// TODO: The safety requirement may be tightened to bi-directional
>> /// transmutability. 
>> 
>> And maybe also add the `Even` example there?
>> 
>> Thoughts?
>> 
>
> Or since we are going to use fine-grained traits, it's actually easy to
> define the safety requirement of `AtomicAdd` (instead of
> `AllowAtomicArithmetic) now:
>
>     /// # Safety
>     ///
>     /// For a `T: AtomicAdd<Rhs>`, the addition result of a valid bit
>     /// pattern of `T` and a `T::Repr` convert from `rhs_into_repr()` should
>     /// be a valid bit pattern of `T`.

Let's combine our two safety requirement ideas (I forgot about my Rhs
change).

---
Cheers,
Benno

>     pub unsafe trait AtomicAdd<Rhs = Self>: AllowAtomic {
>        fn rhs_into_repr(rhs: Rhs) -> Self::Repr;
>     }
>
> Thoughts?
>
> Regards,
> Boqun

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

On Fri, Jul 11, 2025 at 08:57:27PM +0200, Benno Lossin wrote:
> On Fri Jul 11, 2025 at 3:32 PM CEST, Boqun Feng wrote:
> > On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> > [...]
> >> > +}
> >> > +
> >> > +/// A full memory barrier.
> >> > +///
> >> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> >> > +pub fn smp_mb() {
> >> > +    if cfg!(CONFIG_SMP) {
> >> > +        // SAFETY: `smp_mb()` is safe to call.
> >> > +        unsafe {
> >> > +            bindings::smp_mb();
> >> 
> >> Does this really work? How does the Rust compiler know this is a memory
> >> barrier?
> >> 
> >
> > - Without INLINE_HELPER, this is an FFI call, it's safe to assume that
> >   Rust compiler would treat it as a compiler barrier and in smp_mb() a
> >   real memory barrier instruction will be executed. 
> >
> > - With INLINE_HELPER, this will be inlined as an asm block with "memory"
> >   as clobber, and LLVM will know it's a compiler memory barrier, and the
> >   real memory barrier instruction guarantees it's a memory barrier at
> >   CPU reordering level as well.
> >
> > Think about this, SpinLock and Mutex need memory barriers for critical
> > section, if this doesn't work, then SpinLock and Mutex don't work
> > either, then we have a bigger problem ;-)
> 
> By "this not working" I meant that he barrier would be too strong :)
> 
> So essentially without INLINE_HELPER, all barriers in this file are the
> same, but with it, we get less strict ones?

Not the same, each barrier function may generate a different hardware
instruction ;-)

I would say for a Rust function (e.g. smp_mb()), the difference between
with and without INLINE_HELPER is:

- with INLINE_HELPER enabled, they behave exactly like a C function
  calling a C smp_mb().

- without INLINE_HELPER enabled, they behave like a C function calling 
  a function that never inlined:

  void outofline_smp_mb(void)
  {
    smp_mb();
  }

  It might be stronger than the "with INLINE_HELPER" case but both are
  correct regarding memory ordering.

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Boqun Feng]

On Fri, Jul 11, 2025 at 08:55:42PM +0200, Benno Lossin wrote:
[...]
> >> The generic allows you to implement it multiple times with different
> >> meanings, for example:
> >> 
> >>     pub struct Nanos(u64);
> >>     pub struct Micros(u64);
> >>     pub struct Millis(u64);
> >> 
> >>     impl AllowAtomic for Nanos {
> >>         type Repr = i64;
> 
> By the way, I find this a bit unfortunate... I think it would be nice to
> be able to use `u64` and `u32` as reprs too.
> 

I don't think that's necessary, because actually a MaybeUninit<i32> and 
MaybeUninit<i64> would cover all the cases, and even with `u64` and
`u32` being reprs, you still need to trasmute somewhere for non integer
types. But I'm also open to support them, let's discuss this later
separately ;-)

> Maybe we can add an additional trait `AtomicRepr` that gets implemented
> by all integer types and then we can use that in the `Repr` instead.
> 
> This should definitely be a future patch series though.
> 
> >>     }
> >> 
> >>     impl AtomicAdd<Millis> for Nanos {
> >>         fn rhs_into_repr(rhs: Millis) -> i64 {
> >>             transmute(rhs.0 * 1000_000)
> >
> > We probably want to use `as` in real code?
> 
> I thought that `as` would panic on over/underflow... But it doesn't and
> indeed just converts between the two same-sized types.
> 
> By the way, should we ask for `Repr` to always be of the same size as
> `Self` when implementing `AllowAtomic`?
> 
> That might already be implied from the round-trip transmutability:
> * `Self` can't have a smaller size, because transmuting `Self` into
>   `Repr` would result in uninit bytes.
> * `Repr` can't have a smaller size, because then transmuting a `Repr`
>   (that was once a `Self`) back into `Self` will result in uninit bytes
> 
> We probably should mention this in the docs somewhere?
> 

We have it already as the first safety requirement of `AllowAtomic`:

/// # Safety
///
/// - [`Self`] must have the same size and alignment as [`Self::Repr`].

Actually at the beginning, I missed the round-trip transmutablity
(thanks to you and Gary for bring that up), that's only safe requirement
I thought I needed ;-)

> >>         }
> >>     }
> >> 
> >>     impl AtomicAdd<Micros> for Nanos {
> >>         fn rhs_into_repr(rhs: Micros) -> i64 {
> >>             transmute(rhs.0 * 1000)
> >>         }
> >>     }
> >> 
> >>     impl AtomicAdd<Nanos> for Nanos {
> >>         fn rhs_into_repr(rhs: Nanos) -> i64 {
> >>             transmute(rhs.0)
> >>         }
> >>     }
> >> 
> >> For the safety requirement on the `AtomicAdd` trait, we might just
> >> require bi-directional transmutability... Or can you imagine a case
> >> where that is not guaranteed, but a weaker form is?
> >
> > I have a case that I don't think it's that useful, but it's similar to
> > the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
> > `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
> > is not always sound. Maybe we could add a "TODO" in the safety section
> > of `AtomicAdd`, and revisit this later? Like:
> >
> > /// (in # Safety)
> > /// TODO: The safety requirement may be tightened to bi-directional
> > /// transmutability. 
> >
> > And maybe also add the `Even` example there?
> 
> Ahh that's interesting... I don't think the comment in the tightening
> direction makes sense, either we start out with bi-directional
> transmutability, or we don't do it at all.
> 
> I think an `Even` example is motivation enough to have it. So let's not
> tighten it. But I think we should improve the safety requirement:
> 
>     /// The valid bit patterns of `Self` must be a superset of the bit patterns reachable through
>     /// addition on any values of type [`Self::Repr`] obtained by transmuting values of type `Self`.
> 
> or
>     
>     /// Adding any two values of type [`Self::Repr`] obtained through transmuting values of type `Self`
>     /// must yield a value with a bit pattern also valid for `Self`.
> 
> I feel like the second one sounds better.
> 

Me too! Let's use it then. Combining with your `AtomicAdd<Rhs>`
proposal:

    /// # Safety
    ///
    /// Adding any:
    /// - one being the value of [`Self::Repr`] obtained through transmuting value of type `Self`,
    /// - the other being the value of [`Self::Delta`] obtained through conversion of `rhs_into_delta()`,
    /// must yield a value with a bit pattern also valid for `Self`.
    pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
        type Delta = Self::Repr;
        fn rhs_into_delta(rhs: Rhs) -> Delta;
    }

Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
because of when pointer support is added, atomic addition is between
a `*mut ()` and a `isize`, not two `*mut()`.

> Also is overflowing an atomic variable UB in LKMM? Because if it is,

No, all atomic arithmetic operations are wrapping, I did add a comment
in Atomic::add() and Atomic::fetch_add() saying that. This also aligns
with Rust std atomic behaviors.

> then `struct MultipleOf<const M: u64>(u64)` is also something that would
> be supported. Otherwise only powers of two would be supported.

Yeah, seems we can only support PowerOfTwo<integer>.

(but technically you can detect overflow for those value-returning
atomics, but let's think about that later if there is a user)

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Benno Lossin]

On Fri Jul 11, 2025 at 9:51 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 08:55:42PM +0200, Benno Lossin wrote:
> [...]
>> >> The generic allows you to implement it multiple times with different
>> >> meanings, for example:
>> >> 
>> >>     pub struct Nanos(u64);
>> >>     pub struct Micros(u64);
>> >>     pub struct Millis(u64);
>> >> 
>> >>     impl AllowAtomic for Nanos {
>> >>         type Repr = i64;
>> 
>> By the way, I find this a bit unfortunate... I think it would be nice to
>> be able to use `u64` and `u32` as reprs too.
>> 
>
> I don't think that's necessary, because actually a MaybeUninit<i32> and 
> MaybeUninit<i64> would cover all the cases, and even with `u64` and
> `u32` being reprs, you still need to trasmute somewhere for non integer
> types. But I'm also open to support them, let's discuss this later
> separately ;-)

I think it just looks weird for me to build a type that contains a `u64`
and then not being able to choose that as the repr...

>> Maybe we can add an additional trait `AtomicRepr` that gets implemented
>> by all integer types and then we can use that in the `Repr` instead.
>> 
>> This should definitely be a future patch series though.
>> 
>> >>     }
>> >> 
>> >>     impl AtomicAdd<Millis> for Nanos {
>> >>         fn rhs_into_repr(rhs: Millis) -> i64 {
>> >>             transmute(rhs.0 * 1000_000)
>> >
>> > We probably want to use `as` in real code?
>> 
>> I thought that `as` would panic on over/underflow... But it doesn't and
>> indeed just converts between the two same-sized types.
>> 
>> By the way, should we ask for `Repr` to always be of the same size as
>> `Self` when implementing `AllowAtomic`?
>> 
>> That might already be implied from the round-trip transmutability:
>> * `Self` can't have a smaller size, because transmuting `Self` into
>>   `Repr` would result in uninit bytes.
>> * `Repr` can't have a smaller size, because then transmuting a `Repr`
>>   (that was once a `Self`) back into `Self` will result in uninit bytes
>> 
>> We probably should mention this in the docs somewhere?
>> 
>
> We have it already as the first safety requirement of `AllowAtomic`:
>
> /// # Safety
> ///
> /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
>
> Actually at the beginning, I missed the round-trip transmutablity
> (thanks to you and Gary for bring that up), that's only safe requirement
> I thought I needed ;-)

So technically we only need round-trip transmutablity & same alignment
(as size is implied as shown above), but I think it's much more
understandable if we keep it.

>> >>         }
>> >>     }
>> >> 
>> >>     impl AtomicAdd<Micros> for Nanos {
>> >>         fn rhs_into_repr(rhs: Micros) -> i64 {
>> >>             transmute(rhs.0 * 1000)
>> >>         }
>> >>     }
>> >> 
>> >>     impl AtomicAdd<Nanos> for Nanos {
>> >>         fn rhs_into_repr(rhs: Nanos) -> i64 {
>> >>             transmute(rhs.0)
>> >>         }
>> >>     }
>> >> 
>> >> For the safety requirement on the `AtomicAdd` trait, we might just
>> >> require bi-directional transmutability... Or can you imagine a case
>> >> where that is not guaranteed, but a weaker form is?
>> >
>> > I have a case that I don't think it's that useful, but it's similar to
>> > the `Micros` and `Millis` above, an `Even<T>` where `Even<i32>` is a
>> > `i32` but it's always an even number ;-) So transmute<i32, Even<i32>>()
>> > is not always sound. Maybe we could add a "TODO" in the safety section
>> > of `AtomicAdd`, and revisit this later? Like:
>> >
>> > /// (in # Safety)
>> > /// TODO: The safety requirement may be tightened to bi-directional
>> > /// transmutability. 
>> >
>> > And maybe also add the `Even` example there?
>> 
>> Ahh that's interesting... I don't think the comment in the tightening
>> direction makes sense, either we start out with bi-directional
>> transmutability, or we don't do it at all.
>> 
>> I think an `Even` example is motivation enough to have it. So let's not
>> tighten it. But I think we should improve the safety requirement:
>> 
>>     /// The valid bit patterns of `Self` must be a superset of the bit patterns reachable through
>>     /// addition on any values of type [`Self::Repr`] obtained by transmuting values of type `Self`.
>> 
>> or
>>     
>>     /// Adding any two values of type [`Self::Repr`] obtained through transmuting values of type `Self`
>>     /// must yield a value with a bit pattern also valid for `Self`.
>> 
>> I feel like the second one sounds better.
>> 
>
> Me too! Let's use it then. Combining with your `AtomicAdd<Rhs>`
> proposal:
>
>     /// # Safety
>     ///
>     /// Adding any:
>     /// - one being the value of [`Self::Repr`] obtained through transmuting value of type `Self`,
>     /// - the other being the value of [`Self::Delta`] obtained through conversion of `rhs_into_delta()`,
>     /// must yield a value with a bit pattern also valid for `Self`.

I think this will render wrongly in markdown & we shouldn't use a list,
so how about:

    /// Adding any value of type [`Self::Delta`] obtained by [`Self::rhs_into_delta`] to any value of
    /// type [`Self::Repr`] obtained through transmuting a value of type `Self` to must yield a value
    /// with a bit pattern also valid for `Self`.

My only gripe with this is that "Adding" isn't really well-defined...

>     pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
>         type Delta = Self::Repr;
>         fn rhs_into_delta(rhs: Rhs) -> Delta;
>     }
>
> Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
> because of when pointer support is added, atomic addition is between
> a `*mut ()` and a `isize`, not two `*mut()`.

Makes sense, but we don't have default associated types yet :(

>> Also is overflowing an atomic variable UB in LKMM? Because if it is,
>
> No, all atomic arithmetic operations are wrapping, I did add a comment
> in Atomic::add() and Atomic::fetch_add() saying that. This also aligns
> with Rust std atomic behaviors.

Apparently I didn't read your docs very well :)

>> then `struct MultipleOf<const M: u64>(u64)` is also something that would
>> be supported. Otherwise only powers of two would be supported.
>
> Yeah, seems we can only support PowerOfTwo<integer>.
>
> (but technically you can detect overflow for those value-returning
> atomics, but let's think about that later if there is a user)

Yeah, I doubt that a real use-case will pop up soon.

---
Cheers,
Benno

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Benno Lossin]

On Fri Jul 11, 2025 at 9:26 PM CEST, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 08:57:27PM +0200, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 3:32 PM CEST, Boqun Feng wrote:
>> > On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
>> > [...]
>> >> > +}
>> >> > +
>> >> > +/// A full memory barrier.
>> >> > +///
>> >> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
>> >> > +pub fn smp_mb() {
>> >> > +    if cfg!(CONFIG_SMP) {
>> >> > +        // SAFETY: `smp_mb()` is safe to call.
>> >> > +        unsafe {
>> >> > +            bindings::smp_mb();
>> >> 
>> >> Does this really work? How does the Rust compiler know this is a memory
>> >> barrier?
>> >> 
>> >
>> > - Without INLINE_HELPER, this is an FFI call, it's safe to assume that
>> >   Rust compiler would treat it as a compiler barrier and in smp_mb() a
>> >   real memory barrier instruction will be executed. 
>> >
>> > - With INLINE_HELPER, this will be inlined as an asm block with "memory"
>> >   as clobber, and LLVM will know it's a compiler memory barrier, and the
>> >   real memory barrier instruction guarantees it's a memory barrier at
>> >   CPU reordering level as well.
>> >
>> > Think about this, SpinLock and Mutex need memory barriers for critical
>> > section, if this doesn't work, then SpinLock and Mutex don't work
>> > either, then we have a bigger problem ;-)
>> 
>> By "this not working" I meant that he barrier would be too strong :)
>> 
>> So essentially without INLINE_HELPER, all barriers in this file are the
>> same, but with it, we get less strict ones?
>
> Not the same, each barrier function may generate a different hardware
> instruction ;-)
>
> I would say for a Rust function (e.g. smp_mb()), the difference between
> with and without INLINE_HELPER is:
>
> - with INLINE_HELPER enabled, they behave exactly like a C function
>   calling a C smp_mb().
>
> - without INLINE_HELPER enabled, they behave like a C function calling 
>   a function that never inlined:
>
>   void outofline_smp_mb(void)
>   {
>     smp_mb();
>   }
>
>   It might be stronger than the "with INLINE_HELPER" case but both are
>   correct regarding memory ordering.

Yes, this is exactly what I meant with "not working" :)

---
Cheers,
Benno

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Boqun Feng]

On Fri, Jul 11, 2025 at 11:03:24PM +0200, Benno Lossin wrote:
[...]
> > Actually at the beginning, I missed the round-trip transmutablity
> > (thanks to you and Gary for bring that up), that's only safe requirement
> > I thought I needed ;-)
> 
> So technically we only need round-trip transmutablity & same alignment
> (as size is implied as shown above), but I think it's much more
> understandable if we keep it.
> 

Agreed.

[...]
> >
> > Me too! Let's use it then. Combining with your `AtomicAdd<Rhs>`
> > proposal:
> >
> >     /// # Safety
> >     ///
> >     /// Adding any:
> >     /// - one being the value of [`Self::Repr`] obtained through transmuting value of type `Self`,
> >     /// - the other being the value of [`Self::Delta`] obtained through conversion of `rhs_into_delta()`,
> >     /// must yield a value with a bit pattern also valid for `Self`.
> 
> I think this will render wrongly in markdown & we shouldn't use a list,
> so how about:
> 
>     /// Adding any value of type [`Self::Delta`] obtained by [`Self::rhs_into_delta`] to any value of
>     /// type [`Self::Repr`] obtained through transmuting a value of type `Self` to must yield a value
>     /// with a bit pattern also valid for `Self`.
> 

Looks good to me.

> My only gripe with this is that "Adding" isn't really well-defined...
> 

I think it's good enough, and I created a GitHub tracking a few things
we decide to postpone:

	https://github.com/Rust-for-Linux/linux/issues/1180

> >     pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
> >         type Delta = Self::Repr;
> >         fn rhs_into_delta(rhs: Rhs) -> Delta;
> >     }
> >
> > Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
> > because of when pointer support is added, atomic addition is between
> > a `*mut ()` and a `isize`, not two `*mut()`.
> 
> Makes sense, but we don't have default associated types yet :(
> 

Oops, we are lucky enough to only have a few types to begin with ;-)
Maybe we can use `#[derive(AtomicAdd)] to select the default delta type
later.

Regards,
Boqun

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

On Fri, Jul 11, 2025 at 08:34:07PM +0200, Benno Lossin wrote:
[...]
> >
> > So all your disagreement is about the "extra safety requirement" part?
> > How about I drop that:
> >
> >     /// Returns a pointer to the underlying atomic `T`.
> >     pub const fn as_ptr(&self) -> *mut T {
> >         self.0.get()
> >     }
> 
> Yes that's what I had in mind.
> 
> > ? I tried to add something additional information:
> >
> > /// Note that non-atomic reads and writes via the returned pointer may
> > /// cause data races if racing with atomic reads and writes per [LKMM].
> >
> > but that seems redundant, because as you said, data races are UB anyway.
> 
> Yeah... I don't think the stdlib docs are too useful on this function:
> 
>     Doing non-atomic reads and writes on the resulting integer can be a data
>     race. This method is mostly useful for FFI, where the function signature
>     may use *mut i32 instead of &AtomicI32.
>     
>     Returning an *mut pointer from a shared reference to this atomic is safe
>     because the atomic types work with interior mutability. All
>     modifications of an atomic change the value through a shared reference,
>     and can do so safely as long as they use atomic operations. Any use of
>     the returned raw pointer requires an unsafe block and still has to
>     uphold the same restriction: operations on it must be atomic.
> 
> You can mention the use of this function for FFI. People might then be
> discouraged from using it for other things where it doesn't make sense.
> 

I'm going to keep it simple at the beginning (i.e. using the one-line
doc comment above). I added it in an issue so that we can revisit it
later:
	
	https://github.com/Rust-for-Linux/linux/issues/1180

For your other feebacks on patch #4, I think they are reasonable and I'm
going to apply them, except I may need an extra review on the doc
comment of Atomic<T> when I have it. Thanks!

Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

On Fri, Jul 11, 2025 at 11:04:09PM +0200, Benno Lossin wrote:
> On Fri Jul 11, 2025 at 9:26 PM CEST, Boqun Feng wrote:
> > On Fri, Jul 11, 2025 at 08:57:27PM +0200, Benno Lossin wrote:
> >> On Fri Jul 11, 2025 at 3:32 PM CEST, Boqun Feng wrote:
> >> > On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> >> > [...]
> >> >> > +}
> >> >> > +
> >> >> > +/// A full memory barrier.
> >> >> > +///
> >> >> > +/// A barrier that prevents compiler and CPU from reordering memory accesses across the barrier.
> >> >> > +pub fn smp_mb() {
> >> >> > +    if cfg!(CONFIG_SMP) {
> >> >> > +        // SAFETY: `smp_mb()` is safe to call.
> >> >> > +        unsafe {
> >> >> > +            bindings::smp_mb();
> >> >> 
> >> >> Does this really work? How does the Rust compiler know this is a memory
> >> >> barrier?
> >> >> 
> >> >
> >> > - Without INLINE_HELPER, this is an FFI call, it's safe to assume that
> >> >   Rust compiler would treat it as a compiler barrier and in smp_mb() a
> >> >   real memory barrier instruction will be executed. 
> >> >
> >> > - With INLINE_HELPER, this will be inlined as an asm block with "memory"
> >> >   as clobber, and LLVM will know it's a compiler memory barrier, and the
> >> >   real memory barrier instruction guarantees it's a memory barrier at
> >> >   CPU reordering level as well.
> >> >
> >> > Think about this, SpinLock and Mutex need memory barriers for critical
> >> > section, if this doesn't work, then SpinLock and Mutex don't work
> >> > either, then we have a bigger problem ;-)
> >> 
> >> By "this not working" I meant that he barrier would be too strong :)
> >> 
> >> So essentially without INLINE_HELPER, all barriers in this file are the
> >> same, but with it, we get less strict ones?
> >
> > Not the same, each barrier function may generate a different hardware
> > instruction ;-)
> >
> > I would say for a Rust function (e.g. smp_mb()), the difference between
> > with and without INLINE_HELPER is:
> >
> > - with INLINE_HELPER enabled, they behave exactly like a C function
> >   calling a C smp_mb().
> >
> > - without INLINE_HELPER enabled, they behave like a C function calling 
> >   a function that never inlined:
> >
> >   void outofline_smp_mb(void)
> >   {
> >     smp_mb();
> >   }
> >
> >   It might be stronger than the "with INLINE_HELPER" case but both are
> >   correct regarding memory ordering.
> 
> Yes, this is exactly what I meant with "not working" :)
> 

But be stronger is still "working" ;-)

BTW, replying you made me realize I should make these function
#[inline(always)] so thank you ;-)

Regards,
Boqun

> ---
> Cheers,
> Benno
> 

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

On Fri, Jul 11, 2025 at 10:03:07AM +0200, Benno Lossin wrote:
[...]
> > +/// # Round-trip transmutability
[...]
> > +///
> > +/// This essentially means a valid bit pattern of `T: AllowAtomic` has to be a valid bit pattern
> > +/// of `T::Repr`. This is needed because [`Atomic<T: AllowAtomic>`] operates on `T::Repr` to
> > +/// implement atomic operations on `T`.
> > +///
> > +/// Note that this is more relaxed than bidirectional transmutability (i.e. [`transmute()`] is
> > +/// always sound between `T` and `T::Repr`) because of the support for atomic variables over
> 
> s/between `T` and `T::Repr`/from `T` to `T::Repr` and back/
> 

Hmm.. I'm going to keep the "between" form, because here we are talking
about bi-directional transmutability, "from .. to .. and back" sounds
like describing round-trip transmutability.

I also re-aranged the doc comment a bit:

/// Types that support basic atomic operations.
///
/// # Round-trip transmutability
///
/// `T` is round-trip transmutable to `U` if and only if both of these properties hold:
///
/// - Any valid bit pattern for `T` is also a valid bit pattern for `U`.
/// - Transmuting (e.g. using [`transmute()`]) a value of type `T` to `U` and then to `T` again
///   yields a value that is in all aspects equivalent to the original value.
///
/// # Safety
///
/// - [`Self`] must have the same size and alignment as [`Self::Repr`].
/// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
///
/// Note that this is more relaxed than requiring the bi-directional transmutability (i.e.
/// [`transmute()`] is always sound between `U` to `T`) because of the support for atomic variables
/// over unit-only enums, see [Examples].
///
/// # Limitations
///
/// ...
///
/// # Examples
///
/// A unit-only enum that implements [`AllowAtomic`]:
///
/// ```
/// use kernel::sync::atomic::{generic::AllowAtomic, Atomic, Relaxed};
///
/// #[derive(Clone, Copy, PartialEq, Eq)]
/// #[repr(i32)]
/// enum State {
///     Uninit = 0,
///     Working = 1,
///     Done = 2,
/// };
///
/// ...
/// ```
/// [`transmute()`]: core::mem::transmute
/// [round-trip transmutable]: AllowAtomic#round-trip-transmutability
/// [Examples]: AllowAtomic#examples

Thanks!

Regards,
Boqun

> > +/// unit-only enums:
> 
> What are "unit-only" enums? Do you mean enums that don't have associated
> data?
> 
> > +///
> > +/// ```
> > +/// #[repr(i32)]
> > +/// enum State { Init = 0, Working = 1, Done = 2, }
> > +/// ```
[...]
> > +pub unsafe trait AllowAtomic: Sized + Send + Copy {
[...]

Re: [PATCH v6 6/9] rust: sync: atomic: Add the framework of arithmetic operations

[Boqun Feng]

On Fri, Jul 11, 2025 at 02:22:13PM -0700, Boqun Feng wrote:
[...]
> > >     pub unsafe trait AtomicAdd<Rhs>: AllowAtomic {
> > >         type Delta = Self::Repr;
> > >         fn rhs_into_delta(rhs: Rhs) -> Delta;
> > >     }
> > >
> > > Note that I have to provide a `Delta` (or better named as `ReprDelta`?)
> > > because of when pointer support is added, atomic addition is between
> > > a `*mut ()` and a `isize`, not two `*mut()`.
> > 
> > Makes sense, but we don't have default associated types yet :(
> > 
> 
> Oops, we are lucky enough to only have a few types to begin with ;-)
> Maybe we can use `#[derive(AtomicAdd)] to select the default delta type
> later.
> 

Turn out I could give `AtomcImpl` an associate type:

    pub trait AtomicImpl: Sealed {
    	type Delta;
    }

and then

    pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
        /// Converts `Rhs` into the `Delta` type of the atomic implementation.
        fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
    }

(Yeah, I named it `AllowAtomicAdd` ;-))

and I only need to provide `Delta` for three types (i32, i64 and *mut
()).

BTW, since atomic arithmetic is wrapping, do we want things like `impl
AllowAtomicAdd<u32> for i32`, similar to the concept of
`i32::wrapping_add_unsigned()` ;-)

Regards,
Boqun

> Regards,
> Boqun

Re: [PATCH v6 4/9] rust: sync: atomic: Add generic atomics

[Boqun Feng]

On Fri, Jul 11, 2025 at 08:35:25PM +0200, Benno Lossin wrote:
[..]
> >> > +///
> >> > +/// # Guarantees
> >> > +///
> >> > +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race,
> >> > +/// this is guaranteed by the safety requirement of [`Self::from_ptr()`] and the extra safety
> >> > +/// requirement of the usage on pointers returned by [`Self::as_ptr()`].
> >> 
> >> I'd rather think we turn this into an invariant that says any operations
> >> on `self.0` through a shared reference is atomic.
> >> 
[...]
> > [...]
> >> > +        let a = self.as_ptr().cast::<T::Repr>();
> >> > +
> >> > +        // SAFETY:
> >> > +        // - For calling the atomic_read*() function:
> >> > +        //   - `a` is a valid pointer for the function per the CAST justification above.
> >> > +        //   - Per the type guarantees, the following atomic operation won't cause data races.
> >> 
> >> Which type guarantees? `Self`?
> >> 
> >
> > The above "# Guarantees" of `Atomic<T>`, but yeah I think it should be
> > "# Invariants".
> 
> Yeah and if we use invariants/guarantees always mention the type that
> they are of and don't assume the reader will "know" :)
> 

Just FYI, I ended dropping the "# Invariants" (or Guarantees) of
`Atomic<T>` because I don't need them any more. Instead I add a
guarantee for Atoimc::as_ptr(): always returns a properly aligned
pointer, which I actually need for calling C functions.

Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Ralf Jung]

Hi all,

On 11.07.25 20:20, Boqun Feng wrote:
> On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>>> diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
>>> new file mode 100644
>>> index 000000000000..df4015221503
>>> --- /dev/null
>>> +++ b/rust/kernel/sync/barrier.rs
>>> @@ -0,0 +1,65 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Memory barriers.
>>> +//!
>>> +//! These primitives have the same semantics as their C counterparts: and the precise definitions
>>> +//! of semantics can be found at [`LKMM`].
>>> +//!
>>> +//! [`LKMM`]: srctree/tools/memory-model/
>>> +
>>> +/// A compiler barrier.
>>> +///
>>> +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
>>> +pub(crate) fn barrier() {
>>> +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
>>> +    // it suffices as a compiler barrier.
>>
>> I don't know about this, but it also isn't my area of expertise... I
>> think I heard Ralf talk about this at Rust Week, but I don't remember...
>>
> 
> Easy, let's Cc Ralf ;-)
> 
> Ralf, I believe the question here is:
> 
> In kernel C, we define a compiler barrier (barrier()), which is
> implemented as:
> 
> # define barrier() __asm__ __volatile__("": : :"memory")
> 
> Now we want to have a Rust version, and I think an empty `asm!()` should
> be enough as an equivalent as a barrier() in C, because an empty
> `asm!()` in Rust implies "memory" as the clobber:
> 
> 	https://godbolt.org/z/3z3fnWYjs
> 
> ?
> 
> I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
> barrier() and other barriers work for all memory accesses not just
> atomics, so the problem "So, if your program contains no atomic
> accesses, but some atomic fences, those fences do nothing." doesn't
> exist for us. And our barrier() is strictly weaker than other barriers.
> 
> And based on my understanding of the consensus on Rust vs LKMM, "do
> whatever kernel C does and rely on whatever kernel C relies" is the
> general suggestion, so I think an empty `asm!()` works here. Of course
> if in practice, we find an issue, I'm happy to look for solutions ;-)
> 
> Thoughts?
> 
> [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347

If I understood correctly, this is about using "compiler barriers" to order 
volatile accesses that the LKMM uses in lieu of atomic accesses?
I can't give a principled answer here, unfortunately -- as you know, the mapping 
of LKMM through the compiler isn't really in a state where we can make 
principled formal statements. And making principled formal statements is my main 
expertise so I am a bit out of my depth here. ;)

So I agree with your 2nd paragraph: I would say just like the fact that you are 
using volatile accesses in the first place, this falls under "do whatever the C 
code does, it shouldn't be any more broken in Rust than it is in C".

However, saying that it in general "prevents reordering all memory accesses" is 
unlikely to be fully correct -- if the compiler can prove that the inline asm 
block could not possibly have access to a local variable (e.g. because it never 
had its address taken), its accesses can still be reordered. This applies both 
to C compilers and Rust compilers. Extra annotations such as `noalias` (or 
`restrict` in C) can also give rise to reorderings around arbitrary code, 
including such barriers. This is not a problem for concurrent code since it 
would anyway be wrong to claim that some pointer doesn't have aliases when it is 
accessed by multiple threads, but it shows that the framing of barriers in terms 
of preventing reordering of accesses is too imprecise. That's why the C++ memory 
model uses a very different framing, and that's why I can't give a definite 
answer here. :)

Kind regards,
Ralf

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

On Mon, Jul 14, 2025 at 05:42:39PM +0200, Ralf Jung wrote:
> Hi all,
> 
> On 11.07.25 20:20, Boqun Feng wrote:
> > On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> > > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > > > diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
> > > > new file mode 100644
> > > > index 000000000000..df4015221503
> > > > --- /dev/null
> > > > +++ b/rust/kernel/sync/barrier.rs
> > > > @@ -0,0 +1,65 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +//! Memory barriers.
> > > > +//!
> > > > +//! These primitives have the same semantics as their C counterparts: and the precise definitions
> > > > +//! of semantics can be found at [`LKMM`].
> > > > +//!
> > > > +//! [`LKMM`]: srctree/tools/memory-model/
> > > > +
> > > > +/// A compiler barrier.
> > > > +///
> > > > +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
> > > > +pub(crate) fn barrier() {
> > > > +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
> > > > +    // it suffices as a compiler barrier.
> > > 
> > > I don't know about this, but it also isn't my area of expertise... I
> > > think I heard Ralf talk about this at Rust Week, but I don't remember...
> > > 
> > 
> > Easy, let's Cc Ralf ;-)
> > 
> > Ralf, I believe the question here is:
> > 
> > In kernel C, we define a compiler barrier (barrier()), which is
> > implemented as:
> > 
> > # define barrier() __asm__ __volatile__("": : :"memory")
> > 
> > Now we want to have a Rust version, and I think an empty `asm!()` should
> > be enough as an equivalent as a barrier() in C, because an empty
> > `asm!()` in Rust implies "memory" as the clobber:
> > 
> > 	https://godbolt.org/z/3z3fnWYjs
> > 
> > ?
> > 
> > I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
> > barrier() and other barriers work for all memory accesses not just
> > atomics, so the problem "So, if your program contains no atomic
> > accesses, but some atomic fences, those fences do nothing." doesn't
> > exist for us. And our barrier() is strictly weaker than other barriers.
> > 
> > And based on my understanding of the consensus on Rust vs LKMM, "do
> > whatever kernel C does and rely on whatever kernel C relies" is the
> > general suggestion, so I think an empty `asm!()` works here. Of course
> > if in practice, we find an issue, I'm happy to look for solutions ;-)
> > 
> > Thoughts?
> > 
> > [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347
> 
> If I understood correctly, this is about using "compiler barriers" to order
> volatile accesses that the LKMM uses in lieu of atomic accesses?
> I can't give a principled answer here, unfortunately -- as you know, the
> mapping of LKMM through the compiler isn't really in a state where we can
> make principled formal statements. And making principled formal statements
> is my main expertise so I am a bit out of my depth here. ;)
> 

Understood ;-)

> So I agree with your 2nd paragraph: I would say just like the fact that you
> are using volatile accesses in the first place, this falls under "do
> whatever the C code does, it shouldn't be any more broken in Rust than it is
> in C".
> 
> However, saying that it in general "prevents reordering all memory accesses"
> is unlikely to be fully correct -- if the compiler can prove that the inline
> asm block could not possibly have access to a local variable (e.g. because
> it never had its address taken), its accesses can still be reordered. This
> applies both to C compilers and Rust compilers. Extra annotations such as
> `noalias` (or `restrict` in C) can also give rise to reorderings around
> arbitrary code, including such barriers. This is not a problem for
> concurrent code since it would anyway be wrong to claim that some pointer
> doesn't have aliases when it is accessed by multiple threads, but it shows

Right, it shouldn't be a problem for most of the concurrent code, and
thank you for bringing this up. I believe we can rely on the barrier
behavior if the memory accesses on both sides are done via aliased
references/pointers, which should be the same as C code relies on.

One thing though is we don't use much of `restrict` in kernel C, so I
wonder the compiler's behavior in the following code:

    let mut x = KBox::new_uninit(GFP_KERNEL)?;
    // ^ KBox is our own Box implementation based on kmalloc(), and it
    // accepts a flag in new*() functions for different allocation
    // behavior (can sleep or not, etc), of course we want it to behave
    // like an std Box in term of aliasing.

    let x = KBox::write(x, foo); // A

    smp_mb():
      // using Rust asm!() for explanation, it's really implemented in
      // C.
      asm!("mfence");

    let a: &Atomic<*mut Foo> = ...; // `a` was null initially.

    a.store(KBox::into_raw(x), Relaxed); // B

Now we obviously want A and B to be ordered, because smp_mb() is
supposed to be stronger than Release ordering. So if another thread does
an Acquire read or uses address dependency:

    let a: &Atomic<*mut Foo> = ...;
    let foo_ptr = a.load(Acquire); // or load(Relaxed);

    if !foo_ptr.is_null() {
        let y: KBox<Foo> = unsafe { KBox::from_raw(foo_ptr) };
	// ^ this should be safe.
    }

Is it something Rust AM could guarantee? I think it makes no difference
than 1) allocating some normal memory for DMA; 2) writing to the normal
memory; 3) issuing some io barrier instructions to make sure the device
will see the writes in step 2; 4) doing some MMIO to notify the
device for a DMA read. Therefore reordering of A and B by compiler will
be problematic.

Regards,
Boqun

> that the framing of barriers in terms of preventing reordering of accesses
> is too imprecise. That's why the C++ memory model uses a very different
> framing, and that's why I can't give a definite answer here. :)
> 
> Kind regards,
> Ralf
> 

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Ralf Jung]

Hi all,

On 15.07.25 17:21, Boqun Feng wrote:
> On Mon, Jul 14, 2025 at 05:42:39PM +0200, Ralf Jung wrote:
>> Hi all,
>>
>> On 11.07.25 20:20, Boqun Feng wrote:
>>> On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
>>>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>>>>> diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
>>>>> new file mode 100644
>>>>> index 000000000000..df4015221503
>>>>> --- /dev/null
>>>>> +++ b/rust/kernel/sync/barrier.rs
>>>>> @@ -0,0 +1,65 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +//! Memory barriers.
>>>>> +//!
>>>>> +//! These primitives have the same semantics as their C counterparts: and the precise definitions
>>>>> +//! of semantics can be found at [`LKMM`].
>>>>> +//!
>>>>> +//! [`LKMM`]: srctree/tools/memory-model/
>>>>> +
>>>>> +/// A compiler barrier.
>>>>> +///
>>>>> +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
>>>>> +pub(crate) fn barrier() {
>>>>> +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
>>>>> +    // it suffices as a compiler barrier.
>>>>
>>>> I don't know about this, but it also isn't my area of expertise... I
>>>> think I heard Ralf talk about this at Rust Week, but I don't remember...
>>>>
>>>
>>> Easy, let's Cc Ralf ;-)
>>>
>>> Ralf, I believe the question here is:
>>>
>>> In kernel C, we define a compiler barrier (barrier()), which is
>>> implemented as:
>>>
>>> # define barrier() __asm__ __volatile__("": : :"memory")
>>>
>>> Now we want to have a Rust version, and I think an empty `asm!()` should
>>> be enough as an equivalent as a barrier() in C, because an empty
>>> `asm!()` in Rust implies "memory" as the clobber:
>>>
>>> 	https://godbolt.org/z/3z3fnWYjs
>>>
>>> ?
>>>
>>> I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
>>> barrier() and other barriers work for all memory accesses not just
>>> atomics, so the problem "So, if your program contains no atomic
>>> accesses, but some atomic fences, those fences do nothing." doesn't
>>> exist for us. And our barrier() is strictly weaker than other barriers.
>>>
>>> And based on my understanding of the consensus on Rust vs LKMM, "do
>>> whatever kernel C does and rely on whatever kernel C relies" is the
>>> general suggestion, so I think an empty `asm!()` works here. Of course
>>> if in practice, we find an issue, I'm happy to look for solutions ;-)
>>>
>>> Thoughts?
>>>
>>> [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347
>>
>> If I understood correctly, this is about using "compiler barriers" to order
>> volatile accesses that the LKMM uses in lieu of atomic accesses?
>> I can't give a principled answer here, unfortunately -- as you know, the
>> mapping of LKMM through the compiler isn't really in a state where we can
>> make principled formal statements. And making principled formal statements
>> is my main expertise so I am a bit out of my depth here. ;)
>>
> 
> Understood ;-)
> 
>> So I agree with your 2nd paragraph: I would say just like the fact that you
>> are using volatile accesses in the first place, this falls under "do
>> whatever the C code does, it shouldn't be any more broken in Rust than it is
>> in C".
>>
>> However, saying that it in general "prevents reordering all memory accesses"
>> is unlikely to be fully correct -- if the compiler can prove that the inline
>> asm block could not possibly have access to a local variable (e.g. because
>> it never had its address taken), its accesses can still be reordered. This
>> applies both to C compilers and Rust compilers. Extra annotations such as
>> `noalias` (or `restrict` in C) can also give rise to reorderings around
>> arbitrary code, including such barriers. This is not a problem for
>> concurrent code since it would anyway be wrong to claim that some pointer
>> doesn't have aliases when it is accessed by multiple threads, but it shows
> 
> Right, it shouldn't be a problem for most of the concurrent code, and
> thank you for bringing this up. I believe we can rely on the barrier
> behavior if the memory accesses on both sides are done via aliased
> references/pointers, which should be the same as C code relies on.
> 
> One thing though is we don't use much of `restrict` in kernel C, so I
> wonder the compiler's behavior in the following code:
> 
>      let mut x = KBox::new_uninit(GFP_KERNEL)?;
>      // ^ KBox is our own Box implementation based on kmalloc(), and it
>      // accepts a flag in new*() functions for different allocation
>      // behavior (can sleep or not, etc), of course we want it to behave
>      // like an std Box in term of aliasing.
> 
>      let x = KBox::write(x, foo); // A
> 
>      smp_mb():
>        // using Rust asm!() for explanation, it's really implemented in
>        // C.
>        asm!("mfence");
> 
>      let a: &Atomic<*mut Foo> = ...; // `a` was null initially.
> 
>      a.store(KBox::into_raw(x), Relaxed); // B
> 
> Now we obviously want A and B to be ordered, because smp_mb() is
> supposed to be stronger than Release ordering. So if another thread does
> an Acquire read or uses address dependency:
> 
>      let a: &Atomic<*mut Foo> = ...;
>      let foo_ptr = a.load(Acquire); // or load(Relaxed);
> 
>      if !foo_ptr.is_null() {
>          let y: KBox<Foo> = unsafe { KBox::from_raw(foo_ptr) };
> 	// ^ this should be safe.
>      }
> 
> Is it something Rust AM could guarantee?

If we pretend these are normal Rust atomics, and we look at the acquire read, 
then yeah that should work -- the asm block can act like a release fence. With 
the LKMM, it's not a "guarantee" in the same sense any more since it lacks the 
formal foundations, but "it shouldn't be worse than in C".

The Rust/C/C++ memory models do not allow that last example with a relaxed load 
and an address dependency. In C/C++ this requires "consume", which Rust doesn't 
have (and which clang treats as "acquire" -- and GCC does the same, IIRC), and 
which nobody figured out how to properly integrate into any of these languages. 
I will refrain from making any definite statements for the LKMM here. ;)

Kind regards,
Ralf

> I think it makes no difference
> than 1) allocating some normal memory for DMA; 2) writing to the normal
> memory; 3) issuing some io barrier instructions to make sure the device
> will see the writes in step 2; 4) doing some MMIO to notify the
> device for a DMA read. Therefore reordering of A and B by compiler will
> be problematic.
> 
> Regards,
> Boqun
> 
>> that the framing of barriers in terms of preventing reordering of accesses
>> is too imprecise. That's why the C++ memory model uses a very different
>> framing, and that's why I can't give a definite answer here. :)
>>
>> Kind regards,
>> Ralf
>>

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Boqun Feng]

On Tue, Jul 15, 2025 at 05:35:47PM +0200, Ralf Jung wrote:
> Hi all,
> 
> On 15.07.25 17:21, Boqun Feng wrote:
> > On Mon, Jul 14, 2025 at 05:42:39PM +0200, Ralf Jung wrote:
> > > Hi all,
> > > 
> > > On 11.07.25 20:20, Boqun Feng wrote:
> > > > On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
> > > > > On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> > > > > > diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
> > > > > > new file mode 100644
> > > > > > index 000000000000..df4015221503
> > > > > > --- /dev/null
> > > > > > +++ b/rust/kernel/sync/barrier.rs
> > > > > > @@ -0,0 +1,65 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +
> > > > > > +//! Memory barriers.
> > > > > > +//!
> > > > > > +//! These primitives have the same semantics as their C counterparts: and the precise definitions
> > > > > > +//! of semantics can be found at [`LKMM`].
> > > > > > +//!
> > > > > > +//! [`LKMM`]: srctree/tools/memory-model/
> > > > > > +
> > > > > > +/// A compiler barrier.
> > > > > > +///
> > > > > > +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
> > > > > > +pub(crate) fn barrier() {
> > > > > > +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
> > > > > > +    // it suffices as a compiler barrier.
> > > > > 
> > > > > I don't know about this, but it also isn't my area of expertise... I
> > > > > think I heard Ralf talk about this at Rust Week, but I don't remember...
> > > > > 
> > > > 
> > > > Easy, let's Cc Ralf ;-)
> > > > 
> > > > Ralf, I believe the question here is:
> > > > 
> > > > In kernel C, we define a compiler barrier (barrier()), which is
> > > > implemented as:
> > > > 
> > > > # define barrier() __asm__ __volatile__("": : :"memory")
> > > > 
> > > > Now we want to have a Rust version, and I think an empty `asm!()` should
> > > > be enough as an equivalent as a barrier() in C, because an empty
> > > > `asm!()` in Rust implies "memory" as the clobber:
> > > > 
> > > > 	https://godbolt.org/z/3z3fnWYjs
> > > > 
> > > > ?
> > > > 
> > > > I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
> > > > barrier() and other barriers work for all memory accesses not just
> > > > atomics, so the problem "So, if your program contains no atomic
> > > > accesses, but some atomic fences, those fences do nothing." doesn't
> > > > exist for us. And our barrier() is strictly weaker than other barriers.
> > > > 
> > > > And based on my understanding of the consensus on Rust vs LKMM, "do
> > > > whatever kernel C does and rely on whatever kernel C relies" is the
> > > > general suggestion, so I think an empty `asm!()` works here. Of course
> > > > if in practice, we find an issue, I'm happy to look for solutions ;-)
> > > > 
> > > > Thoughts?
> > > > 
> > > > [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347
> > > 
> > > If I understood correctly, this is about using "compiler barriers" to order
> > > volatile accesses that the LKMM uses in lieu of atomic accesses?
> > > I can't give a principled answer here, unfortunately -- as you know, the
> > > mapping of LKMM through the compiler isn't really in a state where we can
> > > make principled formal statements. And making principled formal statements
> > > is my main expertise so I am a bit out of my depth here. ;)
> > > 
> > 
> > Understood ;-)
> > 
> > > So I agree with your 2nd paragraph: I would say just like the fact that you
> > > are using volatile accesses in the first place, this falls under "do
> > > whatever the C code does, it shouldn't be any more broken in Rust than it is
> > > in C".
> > > 
> > > However, saying that it in general "prevents reordering all memory accesses"
> > > is unlikely to be fully correct -- if the compiler can prove that the inline
> > > asm block could not possibly have access to a local variable (e.g. because
> > > it never had its address taken), its accesses can still be reordered. This
> > > applies both to C compilers and Rust compilers. Extra annotations such as
> > > `noalias` (or `restrict` in C) can also give rise to reorderings around
> > > arbitrary code, including such barriers. This is not a problem for
> > > concurrent code since it would anyway be wrong to claim that some pointer
> > > doesn't have aliases when it is accessed by multiple threads, but it shows
> > 
> > Right, it shouldn't be a problem for most of the concurrent code, and
> > thank you for bringing this up. I believe we can rely on the barrier
> > behavior if the memory accesses on both sides are done via aliased
> > references/pointers, which should be the same as C code relies on.
> > 
> > One thing though is we don't use much of `restrict` in kernel C, so I
> > wonder the compiler's behavior in the following code:
> > 
> >      let mut x = KBox::new_uninit(GFP_KERNEL)?;
> >      // ^ KBox is our own Box implementation based on kmalloc(), and it
> >      // accepts a flag in new*() functions for different allocation
> >      // behavior (can sleep or not, etc), of course we want it to behave
> >      // like an std Box in term of aliasing.
> > 
> >      let x = KBox::write(x, foo); // A
> > 
> >      smp_mb():
> >        // using Rust asm!() for explanation, it's really implemented in
> >        // C.
> >        asm!("mfence");
> > 
> >      let a: &Atomic<*mut Foo> = ...; // `a` was null initially.
> > 
> >      a.store(KBox::into_raw(x), Relaxed); // B
> > 
> > Now we obviously want A and B to be ordered, because smp_mb() is
> > supposed to be stronger than Release ordering. So if another thread does
> > an Acquire read or uses address dependency:
> > 
> >      let a: &Atomic<*mut Foo> = ...;
> >      let foo_ptr = a.load(Acquire); // or load(Relaxed);
> > 
> >      if !foo_ptr.is_null() {
> >          let y: KBox<Foo> = unsafe { KBox::from_raw(foo_ptr) };
> > 	// ^ this should be safe.
> >      }
> > 
> > Is it something Rust AM could guarantee?
> 
> If we pretend these are normal Rust atomics, and we look at the acquire
> read, then yeah that should work -- the asm block can act like a release
> fence. With the LKMM, it's not a "guarantee" in the same sense any more
> since it lacks the formal foundations, but "it shouldn't be worse than in
> C".

> 
> The Rust/C/C++ memory models do not allow that last example with a relaxed
> load and an address dependency. In C/C++ this requires "consume", which Rust

Sorry I wasn't clear, of course I wasn't going to start a discussion
about address dependency and formal guarantee about it ;-)

What I meant was the "prevent reordering A and B because of the asm!()"
at the release side, because normally we won't use a restrict pointer to
a kmalloc() result, so I'm curious whether Box make the behavior
different:

    let mut b = Box::new_uninit(...);
    let b = Box::write(b, ...); // <- this is a write done via noalias
    asm!(...);
    a.store(Box::from_raw(b), Relaxed);

But looks like we can just model the asm() as a Rust release fence, so
it should work. Thanks!

Regards,
Boqun


> doesn't have (and which clang treats as "acquire" -- and GCC does the same,
> IIRC), and which nobody figured out how to properly integrate into any of
> these languages. I will refrain from making any definite statements for the
> LKMM here. ;)
> 
[...]

Re: [PATCH v6 8/9] rust: sync: Add memory barriers

[Ralf Jung]

Hi Boqun,

On 15.07.25 17:56, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 05:35:47PM +0200, Ralf Jung wrote:
>> Hi all,
>>
>> On 15.07.25 17:21, Boqun Feng wrote:
>>> On Mon, Jul 14, 2025 at 05:42:39PM +0200, Ralf Jung wrote:
>>>> Hi all,
>>>>
>>>> On 11.07.25 20:20, Boqun Feng wrote:
>>>>> On Fri, Jul 11, 2025 at 10:57:48AM +0200, Benno Lossin wrote:
>>>>>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>>>>>>> diff --git a/rust/kernel/sync/barrier.rs b/rust/kernel/sync/barrier.rs
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..df4015221503
>>>>>>> --- /dev/null
>>>>>>> +++ b/rust/kernel/sync/barrier.rs
>>>>>>> @@ -0,0 +1,65 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>> +
>>>>>>> +//! Memory barriers.
>>>>>>> +//!
>>>>>>> +//! These primitives have the same semantics as their C counterparts: and the precise definitions
>>>>>>> +//! of semantics can be found at [`LKMM`].
>>>>>>> +//!
>>>>>>> +//! [`LKMM`]: srctree/tools/memory-model/
>>>>>>> +
>>>>>>> +/// A compiler barrier.
>>>>>>> +///
>>>>>>> +/// A barrier that prevents compiler from reordering memory accesses across the barrier.
>>>>>>> +pub(crate) fn barrier() {
>>>>>>> +    // By default, Rust inline asms are treated as being able to access any memory or flags, hence
>>>>>>> +    // it suffices as a compiler barrier.
>>>>>>
>>>>>> I don't know about this, but it also isn't my area of expertise... I
>>>>>> think I heard Ralf talk about this at Rust Week, but I don't remember...
>>>>>>
>>>>>
>>>>> Easy, let's Cc Ralf ;-)
>>>>>
>>>>> Ralf, I believe the question here is:
>>>>>
>>>>> In kernel C, we define a compiler barrier (barrier()), which is
>>>>> implemented as:
>>>>>
>>>>> # define barrier() __asm__ __volatile__("": : :"memory")
>>>>>
>>>>> Now we want to have a Rust version, and I think an empty `asm!()` should
>>>>> be enough as an equivalent as a barrier() in C, because an empty
>>>>> `asm!()` in Rust implies "memory" as the clobber:
>>>>>
>>>>> 	https://godbolt.org/z/3z3fnWYjs
>>>>>
>>>>> ?
>>>>>
>>>>> I know you have some opinions on C++ compiler_fence() [1]. But in LKMM,
>>>>> barrier() and other barriers work for all memory accesses not just
>>>>> atomics, so the problem "So, if your program contains no atomic
>>>>> accesses, but some atomic fences, those fences do nothing." doesn't
>>>>> exist for us. And our barrier() is strictly weaker than other barriers.
>>>>>
>>>>> And based on my understanding of the consensus on Rust vs LKMM, "do
>>>>> whatever kernel C does and rely on whatever kernel C relies" is the
>>>>> general suggestion, so I think an empty `asm!()` works here. Of course
>>>>> if in practice, we find an issue, I'm happy to look for solutions ;-)
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/347
>>>>
>>>> If I understood correctly, this is about using "compiler barriers" to order
>>>> volatile accesses that the LKMM uses in lieu of atomic accesses?
>>>> I can't give a principled answer here, unfortunately -- as you know, the
>>>> mapping of LKMM through the compiler isn't really in a state where we can
>>>> make principled formal statements. And making principled formal statements
>>>> is my main expertise so I am a bit out of my depth here. ;)
>>>>
>>>
>>> Understood ;-)
>>>
>>>> So I agree with your 2nd paragraph: I would say just like the fact that you
>>>> are using volatile accesses in the first place, this falls under "do
>>>> whatever the C code does, it shouldn't be any more broken in Rust than it is
>>>> in C".
>>>>
>>>> However, saying that it in general "prevents reordering all memory accesses"
>>>> is unlikely to be fully correct -- if the compiler can prove that the inline
>>>> asm block could not possibly have access to a local variable (e.g. because
>>>> it never had its address taken), its accesses can still be reordered. This
>>>> applies both to C compilers and Rust compilers. Extra annotations such as
>>>> `noalias` (or `restrict` in C) can also give rise to reorderings around
>>>> arbitrary code, including such barriers. This is not a problem for
>>>> concurrent code since it would anyway be wrong to claim that some pointer
>>>> doesn't have aliases when it is accessed by multiple threads, but it shows
>>>
>>> Right, it shouldn't be a problem for most of the concurrent code, and
>>> thank you for bringing this up. I believe we can rely on the barrier
>>> behavior if the memory accesses on both sides are done via aliased
>>> references/pointers, which should be the same as C code relies on.
>>>
>>> One thing though is we don't use much of `restrict` in kernel C, so I
>>> wonder the compiler's behavior in the following code:
>>>
>>>       let mut x = KBox::new_uninit(GFP_KERNEL)?;
>>>       // ^ KBox is our own Box implementation based on kmalloc(), and it
>>>       // accepts a flag in new*() functions for different allocation
>>>       // behavior (can sleep or not, etc), of course we want it to behave
>>>       // like an std Box in term of aliasing.
>>>
>>>       let x = KBox::write(x, foo); // A
>>>
>>>       smp_mb():
>>>         // using Rust asm!() for explanation, it's really implemented in
>>>         // C.
>>>         asm!("mfence");
>>>
>>>       let a: &Atomic<*mut Foo> = ...; // `a` was null initially.
>>>
>>>       a.store(KBox::into_raw(x), Relaxed); // B
>>>
>>> Now we obviously want A and B to be ordered, because smp_mb() is
>>> supposed to be stronger than Release ordering. So if another thread does
>>> an Acquire read or uses address dependency:
>>>
>>>       let a: &Atomic<*mut Foo> = ...;
>>>       let foo_ptr = a.load(Acquire); // or load(Relaxed);
>>>
>>>       if !foo_ptr.is_null() {
>>>           let y: KBox<Foo> = unsafe { KBox::from_raw(foo_ptr) };
>>> 	// ^ this should be safe.
>>>       }
>>>
>>> Is it something Rust AM could guarantee?
>>
>> If we pretend these are normal Rust atomics, and we look at the acquire
>> read, then yeah that should work -- the asm block can act like a release
>> fence. With the LKMM, it's not a "guarantee" in the same sense any more
>> since it lacks the formal foundations, but "it shouldn't be worse than in
>> C".
> 
>>
>> The Rust/C/C++ memory models do not allow that last example with a relaxed
>> load and an address dependency. In C/C++ this requires "consume", which Rust
> 
> Sorry I wasn't clear, of course I wasn't going to start a discussion
> about address dependency and formal guarantee about it ;-)
> 
> What I meant was the "prevent reordering A and B because of the asm!()"
> at the release side, because normally we won't use a restrict pointer to
> a kmalloc() result, so I'm curious whether Box make the behavior
> different:
> 
>      let mut b = Box::new_uninit(...);
>      let b = Box::write(b, ...); // <- this is a write done via noalias
>      asm!(...);
>      a.store(Box::from_raw(b), Relaxed);
> 
> But looks like we can just model the asm() as a Rust release fence, so
> it should work. Thanks!

Yeah... this is actually a subtle case and there are some adjacent compiler bugs 
(when doing the same with local variables, not Box), but those are bugs (and 
they affect both C and Rust).

Kind regards,
Ralf