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 6:13 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 05:45:34PM +0200, Benno Lossin wrote:
>> 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
>
> Oh, if we move `Atomic<T>` to atomic.rs and keep atomic::generic, then
> we can name it atomic::generic::Atomic ;-)

That would be an argument for having the `generic` module :)

Well, I'm not so sure about having two types with the same name right
away, so maybe let's discuss this in our meeting.

>> `AllowAtomic`, but maybe there is a better name, how about `AtomicType`?
>> 
>
> AtomicType may be better than AtomicBase to me.

Yeah I like it better too.

>> >> > +    /// 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`.
>> 
>
> Hmm.. you're probably right. There are two safety things about
> atomic_add():
>
> - Whether calling it is safe
> - Whether the operation on `a` (a pointer to `T` essentially) is safe.

Well part of calling `T::Repr::atomic_add` is that the pointer is valid.
But it actually isn't valid for all operations, only for the specific
one you have here. If we want to be 100% correct, we actually need to
change the safety comment of `atomic_add` to say that it only requires
the result of `*a + v` to be writable... But that is most likely very
annoying... (note that we also have this issue for `store`)

I'm not too sure on what the right way to do this is. The formal answer
is to "just do it right", but then safety comments really just devolve
into formally proving the correctness of the program. I think -- for now
at least :) -- that we shouldn't do this here & now (since we also have
a lot of other code that isn't using normal good safety comments, let
alone formally correct ones).

> How about the following:
>
>         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`.
>         //
>         // SAFETY:
>         // - For calling `atomic_add()`:
>         //   - `a` is aligned to `align_of::<T::Repr>()` because of the safety requirement of
>         //   `AllowAtomic` and the guarantee of `Atomic::as_ptr()`.
>         //   - `a` is a valid pointer per the CAST justification above.
>         // - For accessing `*a`: the value written is transmutable to `T`
>         //   due to the safety requirement of `AllowAtomicAdd`.
>         unsafe { T::Repr::atomic_add(a, v) };

That looks fine for now. But isn't this duplicating the sentence
starting with `*self`?

---
Cheers,
Benno