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

["Benno Lossin" <lossin@kernel.org>]

On Tue Jul 15, 2025 at 3:33 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 01:21:20PM +0200, Benno Lossin wrote:
>> On Mon Jul 14, 2025 at 7:36 AM CEST, Boqun Feng wrote:
>> > +/// Types that support atomic add operations.
>> > +///
>> > +/// # Safety
>> > +///
>> > +/// Wrapping adding any value of type `Self::Repr::Delta` obtained by [`Self::rhs_into_delta()`] to
>> 
>> I don't like "wrapping adding", either we use "`wrapping_add`" or we use
>> some other phrasing.
>> 
>
> Let's use `wrapping_add` then.
>
>     /// `wrapping_add` any value of type `Self::Repr::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`.
>
>> > +pub unsafe trait AllowAtomicAdd<Rhs = Self>: AllowAtomic {
>> 
>> Why `Allow*`? I think `AtomicAdd` is better?
>> 
>
> To be consistent with `AllowAtomic` (the super trait), if we use
> `AtomicAdd` here, should we change `AllowAtomic` to `AtomicBase`?

Ideally, we would name that trait just `Atomic` :) But it then
conflicts with the `Atomic<T>` struct (this would be motivation to put
them in different modules :). I like `AtomicBase` better than
`AllowAtomic`, but maybe there is a better name, how about `AtomicType`?

>> > +    /// Converts `Rhs` into the `Delta` type of the atomic implementation.
>> > +    fn rhs_into_delta(rhs: Rhs) -> <Self::Repr as AtomicImpl>::Delta;
>> > +}
>> > +
>> >  impl<T: AllowAtomic> Atomic<T> {
>> >      /// Creates a new atomic `T`.
>> >      pub const fn new(v: T) -> Self {
>> > @@ -462,3 +474,100 @@ fn try_cmpxchg<Ordering: ordering::Any>(&self, old: &mut T, new: T, _: Ordering)
>> >          ret
>> >      }
>> >  }
>> > +
>> > +impl<T: AllowAtomic> Atomic<T>
>> > +where
>> > +    T::Repr: AtomicHasArithmeticOps,
>> > +{
>> > +    /// Atomic add.
>> > +    ///
>> > +    /// Atomically updates `*self` to `(*self).wrapping_add(v)`.
>> > +    ///
>> > +    /// # Examples
>> > +    ///
>> > +    /// ```
>> > +    /// 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<Rhs, Ordering: ordering::RelaxedOnly>(&self, v: Rhs, _: Ordering)
>> > +    where
>> > +        T: AllowAtomicAdd<Rhs>,
>> > +    {
>> > +        let v = T::rhs_into_delta(v);
>> > +        // CAST: Per the safety requirement of `AllowAtomic`, a valid pointer of `T` is a valid
>> > +        // pointer of `T::Repr` for reads and valid for writes of values transmutable to `T`.
>> > +        let a = self.as_ptr().cast::<T::Repr>();
>> > +
>> > +        // `*self` remains valid after `atomic_add()` because of the safety requirement of
>> > +        // `AllowAtomicAdd`.
>> 
>> This part should be moved to the CAST comment above, since we're not
>> only writing a value transmuted from `T` into `*self`.
>> 
>
> Hmm.. the CAST comment should explain why a pointer of `T` can be a
> valid pointer of `T::Repr` because the atomic_add() below is going to
> read through the pointer and write value back. The comment starting with
> "`*self`" explains the value written is a valid `T`, therefore
> conceptually atomic_add() below writes a valid `T` in form of `T::Repr`
> into `a`.

I see, my interpretation was that if we put it on the cast, then the
operation that `atomic_add` does also is valid.

But I think this comment should either be part of the `CAST` or the
`SAFETY` comment. Going by your interpretation, it would make more sense
in the SAFETY one, since there you justify that you're actually writing
a value of type `T`.

---
Cheers,
Benno

> Basically
>
> // CAST
> let a = ..
>
> ^ explains what `a` is a valid for and why it's valid.
>
> // `*self` remains
>
> ^ explains that we write a valid value to `a`.
>
>
> So I don't think we need to move?