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

[Boqun Feng]

On Thu, Jun 26, 2025 at 02:15:35PM +0200, Andreas Hindborg wrote:
> "Boqun Feng" <boqun.feng@xxxxxxxxx> 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
> 
>