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

Boqun Feng
2025-07-10T16:16:19Z

On Thu, Jul 10, 2025 at 05:46:56PM +0200, Benno Lossin wrote:
> 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.
>

I didn't know I could do that, updated:

($(#[$attr:meta])* pub trait $ops:ident {
$(
unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
bindings::#call($($arg:tt)*)
}
)*
}) => {
$(#[$attr])*

Thanks!

> > $(
> > 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.
[..]
> >> 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

At least the rustdoc has safety section for each function. ;-)

> copy pasting them everywhere isn't really a good solution either...
>

Yeah, perhaps there is no immediate resolution, but open to any
suggestion.

Regards,
Boqun

> ---
> Cheers,
> Benno