Message-ID: <aWxfV4Y3fnNR0Kje@tardis-2.local>
Date: Sun, 18 Jan 2026 12:19:35 +0800
From: Boqun Feng <boqun.feng@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	rcu@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, Will Deacon <will@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Mark Rutland <mark.rutland@....com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Frederic Weisbecker <frederic@...nel.org>,
	Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
	Joel Fernandes <joelagnelf@...dia.com>,
	Josh Triplett <josh@...htriplett.org>,
	Uladzislau Rezki <urezki@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Zqiang <qiang.zhang@...ux.dev>,
	FUJITA Tomonori <fujita.tomonori@...il.com>
Subject: Re: [PATCH 4/5] rust: sync: atomic: Add Atomic<*mut T> support

On Sat, Jan 17, 2026 at 05:03:15PM +0000, Gary Guo wrote:
> On Sat Jan 17, 2026 at 12:22 PM GMT, Boqun Feng wrote:
> > Atomic pointer support is an important piece of synchronization
> > algorithm, e.g. RCU, hence provide the support for that.
> >
> > Note that instead of relying on atomic_long or the implementation of
> > `Atomic<usize>`, a new set of helpers (atomic_ptr_*) is introduced for
> > atomic pointer specifically, this is because ptr2int casting would
> > lose the provenance of a pointer and even though in theory there are a
> > few tricks the provenance can be restored, it'll still be a simpler
> > implementation if C could provide atomic pointers directly. The side
> > effects of this approach are: we don't have the arithmetic and logical
> > operations for pointers yet and the current implementation only works
> > on ARCH_SUPPORTS_ATOMIC_RMW architectures, but these are implementation
> > issues and can be added later.
> >
> > Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> 
> I am happy that this is now using dedicated helpers for pointers, and not going
> through an intermediate integer which can lose provenance.
> 
> Some feedbacks below, but in general LGTM.
> 
> Reviewed-by: Gary Guo <gary@...yguo.net>
> 

Thanks!

> > ---
> > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
[...]
> > index 4aebeacb961a..4d2a5228c2e4 100644
> > --- a/rust/kernel/sync/atomic.rs
> > +++ b/rust/kernel/sync/atomic.rs
> > @@ -51,6 +51,10 @@
> >  #[repr(transparent)]
> >  pub struct Atomic<T: AtomicType>(AtomicRepr<T::Repr>);
> >  
> > +// SAFETY: `Atomic<T>` is safe to transfer between execution contexts because of the safety
> > +// requirement of `AtomicType`.
> > +unsafe impl<T: AtomicType> Send for Atomic<T> {}
> > +
> >  // SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> >  unsafe impl<T: AtomicType> Sync for Atomic<T> {}
> >  
> > @@ -68,6 +72,11 @@ unsafe impl<T: AtomicType> Sync for Atomic<T> {}
> >  ///
> >  /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
> >  /// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
> > +/// - [`Self`] must be safe to transfer between execution contexts, if it's [`Send`], this is
> > +///   automatically satisfied. The exception is pointer types that are even though marked as
> > +///   `!Send` (e.g. raw pointers and [`NonNull<T>`]) but requiring `unsafe` to do anything
> > +///   meaningful on them. This is because transferring pointer values between execution contexts is
> > +///   safe as long as the actual `unsafe` dereferencing is justified.
> 
> I think the discussion about `Send` on pointers should be moved to the `impl<T>
> AtomicType for *mut T` side.
> 

The reason I put something here was to answer the potential question
"why don't you require AtomicType being a subtrait of Send?", that's
more of a question for people who read about `AtomicType`, so I figured
we need some explanation. But I'm fine if you think we should move some
of the comments to the impl block, or we duplicate some. Although I
don't think the current version is worse. Considering we do:

    /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
    /// - [`Self`] must be [round-trip transmutable] to  [`Self::Repr`].
    /// - [`Self`] must be safe to transfer between execution contexts, if it's [`Send`], this is
    ///   automatically satisfied.

for AtomicType, I'm not sure someone read about `AtomicType` could have
everything they need to understand why it's not `: Send`.

[...]
> > +// SAFETY:
> > +//
> > +// - `*mut T` has the same size and alignment with `*const c_void`, and is round-trip
> > +//   transmutable to `*const c_void`.
> > +// - `*mut T` is safe to transfer between execution contexts. See the safety requirement of
> > +//   [`AtomicType`].
> > +unsafe impl<T: Sized> super::AtomicType for *mut T {
> > +    type Repr = *const c_void;
> > +}
> 
> How about *const T?
> 

In general I want to avoid const raw pointers since it provides very
little extra compared to mut raw pointers. For compiler optimization,
provenenace is more important than "const vs mut" modifier, for
dereference, it's unsafe anyway and users need to provide reasoning
(including knowing the provenance and other accesses may happen to the
same address), so I feel the type difference of "*const T" vs "*mut T"
doesn't do anything extra either.

Think about it, in Rust std, there are two pointer types only maps to
"*mut T": NonNull<T> (as_ptr() returns a `*mut T`) and AtomicPtr<T>
(as_ptr() returns a `*mut *mut T`). And there is no type like
NonNullConst<T> and AtomicConstPtr<T>. This is a lint to me that we may
not need to support `*const T` in most cases.

But maybe I'm missing something? If you have a good reason, we can
obviously add the support for `*const T`.

Regards,
Boqun

> > +
> >  // SAFETY: `i32` has the same size and alignment with itself, and is round-trip transmutable to
> >  // itself.
> >  unsafe impl super::AtomicType for i32 {
> > @@ -215,4 +226,16 @@ fn atomic_bool_tests() {
> >          assert_eq!(false, x.load(Relaxed));
> >          assert_eq!(Ok(false), x.cmpxchg(false, true, Full));
> >      }
> > +
> > +    #[test]
> > +    fn atomic_ptr_tests() {
> > +        let mut v = 42;
> > +        let mut u = 43;
> > +        let x = Atomic::new(&raw mut v);
> > +
> > +        assert_eq!(x.load(Acquire), &raw mut v);
> > +        assert_eq!(x.cmpxchg(&raw mut u, &raw mut u, Relaxed), Err(&raw mut v));
> > +        assert_eq!(x.cmpxchg(&raw mut v, &raw mut u, Relaxed), Ok(&raw mut v));
> > +        assert_eq!(x.load(Relaxed), &raw mut u);
> > +    }
> >  }
> 

Powered by blists - more mailing lists