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

Benno Lossin
2025-07-10T15:47:02Z

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