Re: [PATCH v2 3/4] rust: sync: atomic: Add i8/i16 load and store support

["Alexandre Courbot" <acourbot@nvidia.com>]

On Tue Dec 9, 2025 at 8:14 AM JST, FUJITA Tomonori wrote:
> On Mon, 08 Dec 2025 12:08:23 +0900
> "Alexandre Courbot" <acourbot@nvidia.com> wrote:
>
>> On Mon Nov 17, 2025 at 9:10 AM JST, FUJITA Tomonori wrote:
>>> Add atomic operation support for i8 and i16 types using volatile
>>> read/write and smp_load_acquire/smp_store_release helpers.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>> ---
>>>  rust/kernel/sync/atomic/internal.rs  | 48 ++++++++++++++++++++++++++++
>>>  rust/kernel/sync/atomic/predefine.rs | 14 +++++++-
>>>  2 files changed, 61 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/rust/kernel/sync/atomic/internal.rs b/rust/kernel/sync/atomic/internal.rs
>>> index 6fdd8e59f45b..4c9f4f76dbdf 100644
>>> --- a/rust/kernel/sync/atomic/internal.rs
>>> +++ b/rust/kernel/sync/atomic/internal.rs
>>> @@ -263,3 +263,51 @@ fn fetch_add[acquire, release, relaxed](a: &AtomicRepr<Self>, v: Self::Delta) ->
>>>          }
>>>      }
>>>  );
>>> +
>>> +impl private::Sealed for i8 {}
>>> +impl private::Sealed for i16 {}
>>> +
>>> +impl AtomicImpl for i8 {
>>> +    type Delta = Self;
>>> +}
>>> +
>>> +impl AtomicImpl for i16 {
>>> +    type Delta = Self;
>>> +}
>> 
>> I'd suggest putting these next to the impls for `i32` and `i64`, for clarity.
>
> I'm fine with either. Boqun, what do you think?
>
> The reason I didn't put i8/i16 next to i32/i64 is that I assume that
> i8/i16 won't support all the methods supported by i32/i64. I thought
> keeping them separate would make that distinction clearer.

The `AtomicImpl` blocks are the same, it is the macro-implemented blocks
that differ and these can come later. But having all the types and their
basic implementations at the same place helps understanding the basics
of what this module provides IMHO.

>
>>> +macro_rules! impl_atomic_only_load_and_store_ops {
>>> +    ($($ty:ty),* $(,)?) => {
>>> +        $(
>>> +            impl AtomicBasicOps for $ty {
>>> +                paste! {
>>> +                    #[inline(always)]
>>> +                    fn atomic_read(a: &AtomicRepr<Self>) -> Self {
>>> +                        // SAFETY: `a.as_ptr()` is valid and properly aligned.
>>> +                        unsafe { bindings::[< atomic_ $ty _load >](a.as_ptr().cast()) }
>>> +                    }
>>> +
>>> +                    #[inline(always)]
>>> +                    fn atomic_read_acquire(a: &AtomicRepr<Self>) -> Self {
>>> +                        // SAFETY: `a.as_ptr()` is valid and properly aligned.
>>> +                        unsafe { bindings::[< atomic_ $ty _load_acquire >](a.as_ptr().cast()) }
>>> +                    }
>>> +
>>> +                    // Generate atomic_set and atomic_set_release
>>> +                    #[inline(always)]
>>> +                    fn atomic_set(a: &AtomicRepr<Self>, v: Self) {
>>> +                        // SAFETY: `a.as_ptr()` is valid and properly aligned.
>>> +                        unsafe { bindings::[< atomic_ $ty _store >](a.as_ptr().cast(), v) }
>>> +                    }
>>> +
>>> +                    #[inline(always)]
>>> +                    fn atomic_set_release(a: &AtomicRepr<Self>, v: Self) {
>>> +                        // SAFETY: `a.as_ptr()` is valid and properly aligned.
>>> +                        unsafe { bindings::[< atomic_ $ty _store_release >](a.as_ptr().cast(), v) }
>>> +                    }
>>> +                }
>>> +            }
>>> +        )*
>>> +    };
>>> +}
>> 
>> Can you document this macro a bit, in particular the motivations for not
>> leveraging the existing ones (I guess this has to be with the new
>> helpers, but following the code through is a bit difficult without
>> comments).
>
> // Since i8 and i16 are not expected to support the full feature set
> // of i32 and i64, using the current declare_and_impl_atomic_methods!
> // would require refactoring it to handle specific types or splitting
> // the definitions. Furthermore, supporting Atomic<bool> will require
> // even more significant structural changes.
> //
> // To avoid premature refactoring, a separate macro for i8 and i16 is
> // used for now, leaving the existing macros untouched until the overall
> // design requirements are settled.
>
> makes sense?

Absolutely, thanks! Maybe also give a short explanation for *why* i8 and
i16 won't support the same feature set as i32 and i64.