Re: [RFC v2 02/13] rust: sync: Add basic atomic operation mapping framework

On Fri, Dec 13, 2024 at 03:37:13PM +0100, Alice Ryhl wrote:
[...]
> > > > @@ -0,0 +1,19 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +//! Atomic primitives.
> > > > +//!
> > > > +//! These primitives have the same semantics as their C counterparts: and the precise definitions of
> > > > +//! semantics can be found at [`LKMM`]. Note that Linux Kernel Memory (Consistency) Model is the
> > > > +//! only model for Rust code in kernel, and Rust's own atomics should be avoided.
> > > > +//!
> > > > +//! # Data races
> > > > +//!
> > > > +//! [`LKMM`] atomics have different rules regarding data races:
> > > > +//!
> > > > +//! - A normal read doesn't data-race with an atomic read.
> > >
> > > This was fixed:
> > > https://github.com/rust-lang/rust/pull/128778
> > >
> >
> > Yeah, I was aware of that effort, and good to know it's finally merged.
> > Thanks!
> >
> > This will be in 1.83, right? If so, we will still need the above until
> > we bump up the minimal rustc version to 1.83 or beyond. I will handle
> > this properly with the minimal rustc 1.83 (i.e. if this goes in first,
> > will send a follow up patch). I will also mention in the above that this
> > has been changed in 1.83.
> >
> > This also reminds that I should add that LKMM allows mixed-size atomic
> > accesses (as non data race), I will add that in the version.
> 
> This is just documentation. I don't think you need to do any special

The PR also contained miri changes, so the same code will be reported
differently by miri. That was what I was thinking of. However, now think
about it, we are not going to use Rust atomics, so this difference
shouldn't affect us. Therefore I agree, I will drop this.

> MSRV handling.
> 
> > > > +mod private {
> > > > +    /// Sealed trait marker to disable customized impls on atomic implementation traits.
> > > > +    pub trait Sealed {}
> > > > +}
> > >
> > > Just make the trait unsafe?
> > >
> >
> > And make the safety requirement of `AtomicImpl` something like:
> >
> >     The type must have the implementation for atomic operations.
> >
> > ? Hmm.. I don't think that's a good safety requirement TBH. Actually the
> > reason that we need to restrict `AtomicImpl` types is more of an
> > iplementation issue (the implementation need to be done if we want to
> > support i8 or i16) rather than safety issue. So a sealed trait is proper
> > here. Does this make sense? Or am I missing something?
> 
> Where is the AtomicImpl trait used?
> 

It's used when `impl`ing an `AllowAtomic` type, `AllowAtomic` has an
associate type named `Repr` which must be an `AtomicImpl`, i.e. each
type that has atomic operation support must select the underlying
implementation types (currently we only have i32 and i64 from C side
APIs). Using a sealed trait is appropriate in this case, because unless
you are adding atomic support for different sizes of data, you shouldn't
impl `AtomicImpl`.

Regards,
Boqun

> Alice