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
