Re: [PATCH v5 04/10] rust: sync: atomic: Add generic atomics

Boqun Feng
2025-06-27T15:01:42Z

On Thu, Jun 26, 2025 at 02:15:35PM +0200, Andreas Hindborg wrote:
> "Boqun Feng" <boqun.feng@gmail.com> writes:
>
> [...]
>
> > +
> > +impl<T: AllowAtomic> Atomic<T> {
> > + /// Creates a new atomic.
> > + pub const fn new(v: T) -> Self {
> > + Self(Opaque::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 whole lifetime of '`a`, other accesses to the object cannot cause data races
> > + /// (defined by [`LKMM`]) against atomic operations on the returned reference.
>
> I feel the wording is a bit tangled here. How about something along the
> lines of
>
> For the duration of `'a`, all accesses to the object must be atomic.
>

Well, a non-atomic read vs an atomic read is not a data race (for both
Rust memory model and LKMM), so your proposal is overly restricted.

I can s/the whole lifetime/the duration.

> > + ///
> > + /// [`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 inbound.
>
> Did you mean to say "in bounds"? Or what is "inbound"?
>

I think I meant to say "inbounds", fixed.

> > + /// let foo_a_ptr = unsafe { core::ptr::addr_of_mut!((*foo_ptr).a) };
>
> This should be `&raw mut` by now, right?
>

Right.

[...]
>
> Typo `AllocAtomic`.
>

All fixed! Thanks!

Regards,
Boqun

>
> Best regards,
> Andreas Hindborg
>
>