Re: [PATCH v1 1/2] rust: sync: atomic: Add perfromance-optimal Flag type for atomic booleans

[Boqun Feng <boqun@kernel.org>]

On Wed, Jan 28, 2026 at 01:41:41PM +0000, Gary Guo wrote:
> On Wed Jan 28, 2026 at 11:51 AM GMT, FUJITA Tomonori wrote:
> > From: FUJITA Tomonori <fujita.tomonori@gmail.com>
> >
> > Add AtomicFlag type for boolean flags.
> >
> > Document when AtomicFlag is generally preferable to Atomic<bool>: in
> > particular, when RMW operations such as xchg()/cmpxchg() may be used
> > and minimizing memory usage is not the top priority. On some
> > architectures without byte-sized RMW instructions, Atomic<bool> can be
> > slower for RMW operations.
> >
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > ---
> >  rust/kernel/sync/atomic.rs           | 121 +++++++++++++++++++++++++++
> >  rust/kernel/sync/atomic/predefine.rs |  17 ++++
> >  2 files changed, 138 insertions(+)
> >
> > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs
> > index 4aebeacb961a..7d06193709c0 100644
> > --- a/rust/kernel/sync/atomic.rs
> > +++ b/rust/kernel/sync/atomic.rs
> > @@ -560,3 +560,124 @@ pub fn fetch_add<Rhs, Ordering: ordering::Ordering>(&self, v: Rhs, _: Ordering)
> >          unsafe { from_repr(ret) }
> >      }
> >  }
> > +
> > +/// # Invariants
> > +///
> > +/// `padding` must be all zeroes.
> > +#[cfg(not(any(CONFIG_X86_64, CONFIG_UML, CONFIG_ARM, CONFIG_ARM64)))]
> 
> This config repeats too much.
> 
> I think probably we should just not let `AtomicFlag` alias `Atomic<u8>` (this
> has the benefit of creating a type mismatch, so code cannot rely on this on x86
> and fail to compile on, say, RV).
> 
> This way the `struct Flag`, `struct AtomicFlag` and `impl AtomicFlag` would
> always exist and the config is only needed for much fewer times (plus, you don't
> need to macro to avoid duplicating docs).
> 

You probably still need configs for `#[repr(align(_))]` in that case or
two definitions of `Flag` anyway. but yes the duplicate docs can be
avoided, so are some impl blocks.

BTW, while we are at it, maybe we should use arches that don't support
byte-wise atomic instructions here instead of the ones do, i.e. 

    #[cfg(not(any(CONFIG_RISCV, CONFIG_LOONGARCH)))]
    #[repr(C)]
    #[derive(Clone, Copy)]
    struct Flag {
        bool_flag: bool,
    }

    #[cfg(any(CONFIG_RISCV, CONFIG_LOONGARCH))]
    #[repr(C, align(4)]
    #[derive(Clone, Copy)]
    struct Flag {
        #[cfg(target_endian = "big")]
        padding: [u8; 3],
        bool_flag: bool,
        #[cfg(target_endian = "little")]
        padding: [u8; 3],
    }

and we should do

    unsafe impl AtomicType for Flag {
        #[cfg(any(CONFIG_RISCV, CONFIG_LOONGARCH)))]
	type Repr = i32;
        #[cfg(not(any(CONFIG_RISCV, CONFIG_LOONGARCH)))]
	type Repr = i8;
    }

as well.

> > +#[repr(C, align(4))]
> > +#[derive(Clone, Copy)]
> > +struct Flag {
> > +    bool_field: bool,
> > +    padding: [u8; 3],
> 
> You probably still want, on big endian platforms, put padding first, so the
> generated instruction uses small immediates (0 & 1) instead of 0 & 0x01000000.
> 
> Best,
> Gary
> 
> > +}
> > +
> > +#[cfg(not(any(CONFIG_X86_64, CONFIG_UML, CONFIG_ARM, CONFIG_ARM64)))]
> > +impl Flag {
> > +    #[inline(always)]
> > +    const fn new(b: bool) -> Self {
> > +        // INVARIANT: `padding` is all zeroes.
> > +        Self {
> > +            bool_field: b,
> > +            padding: [0; 3],
> > +        }
> > +    }
> > +}
> > +
> > +// SAFETY: `Flag` and `i32` have the same size and alignment, and it's round-trip
> > +// transmutable to `i32`.
> > +#[cfg(not(any(CONFIG_X86_64, CONFIG_UML, CONFIG_ARM, CONFIG_ARM64)))]
> > +unsafe impl AtomicType for Flag {
> > +    type Repr = i32;
> > +}
> > +
> > +macro_rules! atomic_flag_doc {
> > +    () => {
> > +        concat!(
> > +            "An atomic flag type intended to be backed by performance-optimal integer type.\n\n",
> > +            "The backing integer type is an implementation detail; it may vary by architecture and change\n",
> > +            "in the future.\n\n",
> > +            "[`AtomicFlag`] is generally preferable to [`Atomic<bool>`] when you need read-modify-write\n",
> > +            "(RMW) operations (e.g. [`Atomic::xchg()`]/[`Atomic::cmpxchg()`]) or when [`Atomic<bool>`] does\n",
> > +            "not save memory due to padding. On some architectures that do not support byte-sized atomic\n",
> > +            "RMW operations, RMW operations on [`Atomic<bool>`] are slower.\n\n",
> > +            "If you only use [`Atomic::load()`]/[`Atomic::store()`], [`Atomic<bool>`] is fine.\n\n",
> > +            "# Examples\n\n",
> > +            "```\n",
> > +            "use kernel::sync::atomic::{Atomic, AtomicFlag, Relaxed};\n\n",
> > +            "let flag = AtomicFlag::new(false);\n",
> > +            "assert_eq!(false, flag.load(Relaxed));\n",
> > +            "flag.store(true, Relaxed);\n",
> > +            "assert_eq!(true, flag.load(Relaxed));\n",
> > +            "```\n"
> > +        )
> > +    };
> > +}
> > +
> > +#[cfg(not(any(CONFIG_X86_64, CONFIG_UML, CONFIG_ARM, CONFIG_ARM64)))]
> > +#[doc = atomic_flag_doc!()]
> > +pub struct AtomicFlag(Atomic<Flag>);
> > +
> > +#[cfg(any(CONFIG_X86_64, CONFIG_UML, CONFIG_ARM, CONFIG_ARM64))]
> > +#[doc = atomic_flag_doc!()]
> > +pub type AtomicFlag = Atomic<bool>;
> > +
> > +#[cfg(not(any(CONFIG_X86_64, CONFIG_UML, CONFIG_ARM, CONFIG_ARM64)))]
> > +impl AtomicFlag {
> > +    /// Creates a new atomic flag.
> > +    #[inline(always)]
> > +    pub const fn new(b: bool) -> Self {
> > +        Self(Atomic::new(Flag::new(b)))
> > +    }
> > +
> > +    /// Returns a mutable reference to the underlying flag as a [`bool`].
> > +    ///
> > +    /// This is safe because the mutable reference of the atomic flag guarantees exclusive access.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::sync::atomic::{AtomicFlag, Relaxed};
> > +    ///
> > +    /// let mut atomic_flag = AtomicFlag::new(false);
> > +    /// assert_eq!(false, atomic_flag.load(Relaxed));
> > +    /// *atomic_flag.get_mut() = true;
> > +    /// assert_eq!(true, atomic_flag.load(Relaxed));
> > +    /// ```
> > +    #[inline(always)]
> > +    pub fn get_mut(&mut self) -> &mut bool {
> > +        &mut self.0.get_mut().bool_field
> > +    }
> > +
> > +    /// Loads the value from the atomic flag.
> > +    #[inline(always)]
> > +    pub fn load<Ordering: ordering::AcquireOrRelaxed>(&self, o: Ordering) -> bool {
> > +        self.0.load(o).bool_field

For load(), xchg() and cmpxchg(), I think we should not use
`.bool_field`, because we know that `padding` is always zero, but
compilers don't. Using `.bool_field` will make the compiler generate i32
to i8 or a bit mask instruction to get the boolean. We need to implement
`From<Flag>` for `bool` and use `.into()` here.

Regards,
Boqun


> > +    }
> > +
> > +    /// Stores a value to the atomic flag.
> > +    #[inline(always)]
> > +    pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, v: bool, o: Ordering) {
> > +        self.0.store(Flag::new(v), o);
> > +    }
> > +
> > +    /// Stores a value to the atomic flag and returns the previous value.
> > +    #[inline(always)]
> > +    pub fn xchg<Ordering: ordering::Ordering>(&self, new: bool, o: Ordering) -> bool {
> > +        self.0.xchg(Flag::new(new), o).bool_field
> > +    }
> > +
> > +    /// Store a value to the atomic flag if the current value is equal to `old`.
> > +    #[inline(always)]
> > +    pub fn cmpxchg<Ordering: ordering::Ordering>(
> > +        &self,
> > +        old: bool,
> > +        new: bool,
> > +        o: Ordering,
> > +    ) -> Result<bool, bool> {
> > +        match self.0.cmpxchg(Flag::new(old), Flag::new(new), o) {
> > +            Ok(_) => Ok(old),
> > +            Err(f) => Err(f.bool_field),
> > +        }
> > +    }
> > +}
> > diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs
> > index 42067c6a266c..d14e10544dcf 100644
> > --- a/rust/kernel/sync/atomic/predefine.rs
> > +++ b/rust/kernel/sync/atomic/predefine.rs
> > @@ -215,4 +215,21 @@ fn atomic_bool_tests() {
> >          assert_eq!(false, x.load(Relaxed));
> >          assert_eq!(Ok(false), x.cmpxchg(false, true, Full));
> >      }
> > +
> > +    #[test]
> > +    fn atomic_flag_tests() {
> > +        let mut flag = AtomicFlag::new(false);
> > +
> > +        assert_eq!(false, flag.load(Relaxed));
> > +
> > +        *flag.get_mut() = true;
> > +        assert_eq!(true, flag.load(Relaxed));
> > +
> > +        assert_eq!(true, flag.xchg(false, Relaxed));
> > +        assert_eq!(false, flag.load(Relaxed));
> > +
> > +        *flag.get_mut() = true;
> > +        assert_eq!(Ok(true), flag.cmpxchg(true, false, Full));
> > +        assert_eq!(false, flag.load(Relaxed));
> > +    }
> >  }
>