Message-Id: <DFRTTN1T431A.2BLP8QZY02E6Q@garyguo.net>
Date: Sun, 18 Jan 2026 15:39:40 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Boqun Feng" <boqun.feng@...il.com>, "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 Sun Jan 18, 2026 at 4:19 AM GMT, Boqun Feng wrote:
> 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`.

Ok.

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

Actually `NonNull` is internally `*const T`, because it's covariant, unlike
`*mut T` which is invariant.

Now, for atomics, it's less likely that you actually want covariance. So this
difference matters less.

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

It just feels that it is somewhat inconsistent. There's no good motivation right
now. I am fine to leave it out and add when needed.

Best,
Gary

Powered by blists - more mailing lists