Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> On Thu, Jul 10, 2025 at 01:04:38PM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
>> > +declare_and_impl_atomic_methods!(
>> > +    AtomicHasBasicOps ("Basic atomic operations") {
>> > +        read[acquire](ptr: *mut Self) -> Self {
>> > +            call(ptr.cast())
>> > +        }
>> > +
>> > +        set[release](ptr: *mut Self, v: Self) {
>> > +            call(ptr.cast(), v)
>> > +        }
>> > +    }
>> 
>> I think this would look a bit better:
>> 
>>     /// Basic atomic operations.
>>     pub trait AtomicHasBasicOps {
>>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
>>             bindings::#call(ptr.cast())
>>         }
>> 
>>         unsafe fn set[release](ptr: *mut Self, v: Self) {
>>             bindings::#call(ptr.cast(), v)
>>         }
>>     }
>> 
>
> Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
> hard-coded:
>
> macro_rules! declare_and_impl_atomic_methods {
>     (#[doc = $doc:expr] pub trait $ops:ident {

You should allow any kind of attribute (and multiple), that makes it
much simpler.

>         $(
>             unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
>                 bindings::#call($($arg:tt)*)
>             }
>         )*
>     }) => {
>
> It shouldn't be very hard to make use of the actual visibility or
> unsafe, but we currently don't have other visibility or safe function,
> so it's simple to keep it as it is.

Yeah I also meant hardcoding them.

>> And then we could also put the safety comments inline:
>> 
>>     /// Basic atomic operations.
>>     pub trait AtomicHasBasicOps {
>>         /// Atomic read
>>         ///
>>         /// # Safety
>>         /// - Any pointer passed to the function has to be a valid pointer
>>         /// - Accesses must not cause data races per LKMM:
>>         ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>>         ///   - Atomic write racing with normal read or normal write is a data race, unless the
>>         ///     normal access is done from the C side and considered immune to data races, e.g.
>>         ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
>>             // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>>             // cause data race per LKMM.
>>             unsafe { bindings::#call(ptr.cast()) }
>>         }
>> 
>>         /// Atomic read
>
> Copy-pasta ;-)
>
>>         ///
>>         /// # Safety
>>         /// - Any pointer passed to the function has to be a valid pointer
>>         /// - Accesses must not cause data races per LKMM:
>>         ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>>         ///   - Atomic write racing with normal read or normal write is a data race, unless the
>>         ///     normal access is done from the C side and considered immune to data races, e.g.
>>         ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>>         unsafe fn set[release](ptr: *mut Self, v: Self) {
>>             // SAFETY: Per function safety requirement, all pointers are valid, and accesses won't
>>             // cause data race per LKMM.
>>             unsafe { bindings::#call(ptr.cast(), v) }
>>         }
>>     }
>> 
>> I'm not sure if this is worth it, but for reading the definitions of
>> these operations directly in the code this is going to be a lot more
>> readable. I don't think it's too bad to duplicate it.
>> 
>> I'm also not fully satisfied with the safety comment on
>> `bindings::#call`...
>> 
>
> Based on the above, I'm not going to do the change (i.e. duplicating
> the safe comments and improve them), and I would make an issue tracking
> it, and we can revisit it when we have time. Sounds good?

Sure, I feel like some kind of method duplication macro might be much
better here, like:

    multi_functions! {
        pub trait AtomicHasBasicOps {
            /// Atomic read
            ///
            /// # Safety
            /// - Any pointer passed to the function has to be a valid pointer
            /// - Accesses must not cause data races per LKMM:
            ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
            ///   - Atomic write racing with normal read or normal write is a data race, unless the
            ///     normal access is done from the C side and considered immune to data races, e.g.
            ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
            unsafe fn [<read, read_acquire>](ptr: *mut Self) -> Self;

            // ...
        }
    }

And then also allow it on impls. I don't really like the idea of
duplicating and thus hiding the safety docs... But I also see that just
copy pasting them everywhere isn't really a good solution either...

---
Cheers,
Benno