* [PATCH v2 0/2] rust: add AtomicFlag::get_mut @ 2026-01-27 12:52 FUJITA Tomonori 2026-01-27 12:52 ` [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut FUJITA Tomonori 2026-01-27 12:53 ` [PATCH v2 2/2] rust: list: Use AtomicFlag in AtomicTracker FUJITA Tomonori 0 siblings, 2 replies; 15+ messages in thread From: FUJITA Tomonori @ 2026-01-27 12:52 UTC (permalink / raw) To: boqun.feng, ojeda, peterz, will Cc: a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori From: FUJITA Tomonori <fujita.tomonori@gmail.com> This series adds AtomicFlag::get_mut to expose a bool API under exclusive access, and switches the list atomic tracker to use AtomicFlag. Note that I sent this patchset yesterday, but it didn't seem to be delivered properly, so I'm resending it via a different mail server. Hopefully it goes through this time. v2: - Add AtomicFlag::get_mut() - Make AtomicTracker use get_mut() v1: https://lore.kernel.org/rust-for-linux/20260119231757.3460885-1-fujita.tomonori@gmail.com/ FUJITA Tomonori (2): rust: sync: atomic: Add AtomicFlag::get_mut rust: list: Use AtomicFlag in AtomicTracker rust/kernel/list/arc.rs | 8 ++++---- rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) base-commit: ed45fc4e7712dabca517bee9f73b91e7da0030a6 -- 2.43.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 12:52 [PATCH v2 0/2] rust: add AtomicFlag::get_mut FUJITA Tomonori @ 2026-01-27 12:52 ` FUJITA Tomonori 2026-01-27 13:53 ` Gary Guo ` (2 more replies) 2026-01-27 12:53 ` [PATCH v2 2/2] rust: list: Use AtomicFlag in AtomicTracker FUJITA Tomonori 1 sibling, 3 replies; 15+ messages in thread From: FUJITA Tomonori @ 2026-01-27 12:52 UTC (permalink / raw) To: boqun.feng, ojeda, peterz, will Cc: a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori From: FUJITA Tomonori <fujita.tomonori@gmail.com> AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to Atomic<T>::get_mut(). Also add kunit tests for AtomicFlag. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs index 6c46335bdb8c..b6c01d9f3a46 100644 --- a/rust/kernel/sync/atomic.rs +++ b/rust/kernel/sync/atomic.rs @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) self.0.store(b.into(), o) } + /// 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. + pub fn get_mut(&mut self) -> &mut bool { + let byte_ptr = { + let ptr = self.0.as_ptr().cast::<u8>(); + let offset = if cfg!(target_endian = "big") { + core::mem::size_of::<Flag>() - 1 + } else { + 0 + }; + + // SAFETY: `ptr` is valid for `size_of::<Flag>()` bytes; `offset` selects the LSB. + unsafe { ptr.add(offset) } + }; + + // SAFETY: The LSB holds `0`/`1` for `Flag::Clear/Set`, and `bool` is `i8`-sized/aligned. + unsafe { &mut *byte_ptr.cast::<bool>() } + } + /// Stores a value to the atomic flag and returns the previous value. #[inline(always)] pub fn xchg<Ordering: ordering::Ordering>(&self, b: bool, o: Ordering) -> bool { diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs index 11bc67ab70a3..e413b9e9fe21 100644 --- a/rust/kernel/sync/atomic/predefine.rs +++ b/rust/kernel/sync/atomic/predefine.rs @@ -351,6 +351,23 @@ fn atomic_bool_tests() { 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)); + } + #[test] fn atomic_ptr_tests() { let mut v = 42; -- 2.43.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 12:52 ` [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut FUJITA Tomonori @ 2026-01-27 13:53 ` Gary Guo 2026-01-27 14:32 ` Miguel Ojeda 2026-01-27 15:35 ` Boqun Feng 2 siblings, 0 replies; 15+ messages in thread From: Gary Guo @ 2026-01-27 13:53 UTC (permalink / raw) To: FUJITA Tomonori, boqun.feng, ojeda, peterz, will Cc: a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue Jan 27, 2026 at 12:52 PM GMT, FUJITA Tomonori wrote: > From: FUJITA Tomonori <fujita.tomonori@gmail.com> > > AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to > Atomic<T>::get_mut(). > > Also add kunit tests for AtomicFlag. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Reviewed-by: Gary Guo <gary@garyguo.net> > --- > rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ > rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ > 2 files changed, 37 insertions(+) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 12:52 ` [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut FUJITA Tomonori 2026-01-27 13:53 ` Gary Guo @ 2026-01-27 14:32 ` Miguel Ojeda 2026-01-28 3:47 ` FUJITA Tomonori 2026-01-27 15:35 ` Boqun Feng 2 siblings, 1 reply; 15+ messages in thread From: Miguel Ojeda @ 2026-01-27 14:32 UTC (permalink / raw) To: FUJITA Tomonori Cc: boqun.feng, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue, Jan 27, 2026 at 1:53 PM FUJITA Tomonori <tomo@aliasing.net> wrote: > > + /// Returns a mutable reference to the underlying flag as a `bool`. [`bool`] > + /// This is safe because the mutable reference of the atomic flag guarantees exclusive access. > + pub fn get_mut(&mut self) -> &mut bool { Should we give an example here, perhaps promoting (part of) the test below? Thanks! Cheers, Miguel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 14:32 ` Miguel Ojeda @ 2026-01-28 3:47 ` FUJITA Tomonori 2026-01-28 3:55 ` Boqun Feng 0 siblings, 1 reply; 15+ messages in thread From: FUJITA Tomonori @ 2026-01-28 3:47 UTC (permalink / raw) To: miguel.ojeda.sandonis, boqun.feng Cc: ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, fujita.tomonori On Tue, 27 Jan 2026 15:32:29 +0100 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Tue, Jan 27, 2026 at 1:53 PM FUJITA Tomonori <tomo@aliasing.net> wrote: >> >> + /// Returns a mutable reference to the underlying flag as a `bool`. > > [`bool`] Oops, I'll fix. >> + /// This is safe because the mutable reference of the atomic flag guarantees exclusive access. >> + pub fn get_mut(&mut self) -> &mut bool { > > Should we give an example here, perhaps promoting (part of) the test below? Boqun, you prefer to have the examples of get_mut() for AtomicFlag and Atomic<T>? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-28 3:47 ` FUJITA Tomonori @ 2026-01-28 3:55 ` Boqun Feng 0 siblings, 0 replies; 15+ messages in thread From: Boqun Feng @ 2026-01-28 3:55 UTC (permalink / raw) To: FUJITA Tomonori Cc: miguel.ojeda.sandonis, boqun.feng, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, fujita.tomonori On Wed, Jan 28, 2026 at 12:47:34PM +0900, FUJITA Tomonori wrote: > On Tue, 27 Jan 2026 15:32:29 +0100 > Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > > On Tue, Jan 27, 2026 at 1:53 PM FUJITA Tomonori <tomo@aliasing.net> wrote: > >> > >> + /// Returns a mutable reference to the underlying flag as a `bool`. > > > > [`bool`] > > Oops, I'll fix. > > >> + /// This is safe because the mutable reference of the atomic flag guarantees exclusive access. > >> + pub fn get_mut(&mut self) -> &mut bool { > > > > Should we give an example here, perhaps promoting (part of) the test below? > > Boqun, you prefer to have the examples of get_mut() for AtomicFlag and > Atomic<T>? > Sure, that makes sense to me, thank you! Regards, Boqun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 12:52 ` [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut FUJITA Tomonori 2026-01-27 13:53 ` Gary Guo 2026-01-27 14:32 ` Miguel Ojeda @ 2026-01-27 15:35 ` Boqun Feng 2026-01-27 15:42 ` Gary Guo 2 siblings, 1 reply; 15+ messages in thread From: Boqun Feng @ 2026-01-27 15:35 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori [For some unknown reasons, I cannot send my reply via gmail hence reply via kernel.org account, I might switch from gmail later on] On Tue, Jan 27, 2026 at 09:52:59PM +0900, FUJITA Tomonori wrote: > From: FUJITA Tomonori <fujita.tomonori@gmail.com> > > AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to > Atomic<T>::get_mut(). > > Also add kunit tests for AtomicFlag. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ > rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ > 2 files changed, 37 insertions(+) > > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > index 6c46335bdb8c..b6c01d9f3a46 100644 > --- a/rust/kernel/sync/atomic.rs > +++ b/rust/kernel/sync/atomic.rs > @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) > self.0.store(b.into(), o) > } > > + /// 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. > + pub fn get_mut(&mut self) -> &mut bool { > + let byte_ptr = { > + let ptr = self.0.as_ptr().cast::<u8>(); > + let offset = if cfg!(target_endian = "big") { > + core::mem::size_of::<Flag>() - 1 > + } else { > + 0 > + }; The idea is solid, but I want to avoid endian handling in the function, I would prefer a "struct declaration" solution like: #[cfg(target_endian = "big")] #[repr(align(4))] pub(super) struct FlagInner { _pad: [i8; 3], bool_field: bool, } #[cfg(target_endian = "little")] #[repr(align(4))] struct FlagInner { bool_field: bool, _pad: [i8; 3], } redefine `Flag` as `BoolFlag` #[repr(i32)] pub enum BoolFlag { Clear = 0, Set = 1, } and `Flag` becomes a union of `BoolFlag` and `FlagInner`: /// # Invariants /// `Flag` is either 0 or 1 in a i32 representation which implies /// that `inner` is always valid as long as `_pad` stays 0. pub union Flag { pub(super) inner: FlagInner, pub flag: BoolFlag, } // can static_assert that `Flag` and `BoolFlag` has the same // alignement and size. then impl AtomicFlag { pub fn get_mut(&mut self) -> &mut bool { let flag = self.0.get_mut(); // <- &mut Flag // INVARIANTS: flag.inner._pad cannot be modified via the // returned reference. // SAFETY: Per type invariants, `flag.inner.bool_field` is // always a valid bool. unsafe { &mut flag.inner.bool_field } } } Thoughts? Regards, Boqun > + > + // SAFETY: `ptr` is valid for `size_of::<Flag>()` bytes; `offset` selects the LSB. > + unsafe { ptr.add(offset) } > + }; > + > + // SAFETY: The LSB holds `0`/`1` for `Flag::Clear/Set`, and `bool` is `i8`-sized/aligned. > + unsafe { &mut *byte_ptr.cast::<bool>() } > + } > + > /// Stores a value to the atomic flag and returns the previous value. > #[inline(always)] > pub fn xchg<Ordering: ordering::Ordering>(&self, b: bool, o: Ordering) -> bool { > diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs > index 11bc67ab70a3..e413b9e9fe21 100644 > --- a/rust/kernel/sync/atomic/predefine.rs > +++ b/rust/kernel/sync/atomic/predefine.rs > @@ -351,6 +351,23 @@ fn atomic_bool_tests() { > 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)); > + } > + > #[test] > fn atomic_ptr_tests() { > let mut v = 42; > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 15:35 ` Boqun Feng @ 2026-01-27 15:42 ` Gary Guo 2026-01-27 15:59 ` Boqun Feng 0 siblings, 1 reply; 15+ messages in thread From: Gary Guo @ 2026-01-27 15:42 UTC (permalink / raw) To: Boqun Feng, FUJITA Tomonori Cc: ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue Jan 27, 2026 at 3:35 PM GMT, Boqun Feng wrote: > [For some unknown reasons, I cannot send my reply via gmail hence reply > via kernel.org account, I might switch from gmail later on] > > On Tue, Jan 27, 2026 at 09:52:59PM +0900, FUJITA Tomonori wrote: >> From: FUJITA Tomonori <fujita.tomonori@gmail.com> >> >> AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to >> Atomic<T>::get_mut(). >> >> Also add kunit tests for AtomicFlag. >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> --- >> rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ >> rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs >> index 6c46335bdb8c..b6c01d9f3a46 100644 >> --- a/rust/kernel/sync/atomic.rs >> +++ b/rust/kernel/sync/atomic.rs >> @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) >> self.0.store(b.into(), o) >> } >> >> + /// 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. >> + pub fn get_mut(&mut self) -> &mut bool { >> + let byte_ptr = { >> + let ptr = self.0.as_ptr().cast::<u8>(); >> + let offset = if cfg!(target_endian = "big") { >> + core::mem::size_of::<Flag>() - 1 >> + } else { >> + 0 >> + }; > > The idea is solid, but I want to avoid endian handling in the function, > I would prefer a "struct declaration" solution like: > > #[cfg(target_endian = "big")] > #[repr(align(4))] > pub(super) struct FlagInner { > _pad: [i8; 3], > bool_field: bool, > } > > #[cfg(target_endian = "little")] > #[repr(align(4))] > struct FlagInner { > bool_field: bool, > _pad: [i8; 3], > } > > redefine `Flag` as `BoolFlag` > > #[repr(i32)] > pub enum BoolFlag { > Clear = 0, > Set = 1, > } > > and `Flag` becomes a union of `BoolFlag` and `FlagInner`: > > /// # Invariants > /// `Flag` is either 0 or 1 in a i32 representation which implies > /// that `inner` is always valid as long as `_pad` stays 0. > pub union Flag { > pub(super) inner: FlagInner, > pub flag: BoolFlag, > } > > // can static_assert that `Flag` and `BoolFlag` has the same > // alignement and size. > > then > > impl AtomicFlag { > pub fn get_mut(&mut self) -> &mut bool { > let flag = self.0.get_mut(); // <- &mut Flag > > // INVARIANTS: flag.inner._pad cannot be modified via the > // returned reference. > // SAFETY: Per type invariants, `flag.inner.bool_field` is > // always a valid bool. > unsafe { &mut flag.inner.bool_field } > } > } > > Thoughts? > > Regards, > Boqun I think we really need special handling for endianness for this one single function, so doing all the extra stuff feels really unnecessary. I prefer Fujita's current version. Best, Gary > >> + >> + // SAFETY: `ptr` is valid for `size_of::<Flag>()` bytes; `offset` selects the LSB. >> + unsafe { ptr.add(offset) } >> + }; >> + >> + // SAFETY: The LSB holds `0`/`1` for `Flag::Clear/Set`, and `bool` is `i8`-sized/aligned. >> + unsafe { &mut *byte_ptr.cast::<bool>() } >> + } >> + >> /// Stores a value to the atomic flag and returns the previous value. >> #[inline(always)] >> pub fn xchg<Ordering: ordering::Ordering>(&self, b: bool, o: Ordering) -> bool { >> diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs >> index 11bc67ab70a3..e413b9e9fe21 100644 >> --- a/rust/kernel/sync/atomic/predefine.rs >> +++ b/rust/kernel/sync/atomic/predefine.rs >> @@ -351,6 +351,23 @@ fn atomic_bool_tests() { >> 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)); >> + } >> + >> #[test] >> fn atomic_ptr_tests() { >> let mut v = 42; >> -- >> 2.43.0 >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 15:42 ` Gary Guo @ 2026-01-27 15:59 ` Boqun Feng 2026-01-27 16:10 ` Gary Guo 0 siblings, 1 reply; 15+ messages in thread From: Boqun Feng @ 2026-01-27 15:59 UTC (permalink / raw) To: Gary Guo Cc: FUJITA Tomonori, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue, Jan 27, 2026 at 03:42:42PM +0000, Gary Guo wrote: > On Tue Jan 27, 2026 at 3:35 PM GMT, Boqun Feng wrote: > > [For some unknown reasons, I cannot send my reply via gmail hence reply > > via kernel.org account, I might switch from gmail later on] > > > > On Tue, Jan 27, 2026 at 09:52:59PM +0900, FUJITA Tomonori wrote: > >> From: FUJITA Tomonori <fujita.tomonori@gmail.com> > >> > >> AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to > >> Atomic<T>::get_mut(). > >> > >> Also add kunit tests for AtomicFlag. > >> > >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > >> --- > >> rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ > >> rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ > >> 2 files changed, 37 insertions(+) > >> > >> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > >> index 6c46335bdb8c..b6c01d9f3a46 100644 > >> --- a/rust/kernel/sync/atomic.rs > >> +++ b/rust/kernel/sync/atomic.rs > >> @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) > >> self.0.store(b.into(), o) > >> } > >> > >> + /// 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. > >> + pub fn get_mut(&mut self) -> &mut bool { > >> + let byte_ptr = { > >> + let ptr = self.0.as_ptr().cast::<u8>(); > >> + let offset = if cfg!(target_endian = "big") { > >> + core::mem::size_of::<Flag>() - 1 > >> + } else { > >> + 0 > >> + }; > > > > The idea is solid, but I want to avoid endian handling in the function, > > I would prefer a "struct declaration" solution like: > > > > #[cfg(target_endian = "big")] > > #[repr(align(4))] > > pub(super) struct FlagInner { > > _pad: [i8; 3], > > bool_field: bool, > > } > > > > #[cfg(target_endian = "little")] > > #[repr(align(4))] > > struct FlagInner { > > bool_field: bool, > > _pad: [i8; 3], > > } > > > > redefine `Flag` as `BoolFlag` > > > > #[repr(i32)] > > pub enum BoolFlag { > > Clear = 0, > > Set = 1, > > } > > > > and `Flag` becomes a union of `BoolFlag` and `FlagInner`: > > > > /// # Invariants > > /// `Flag` is either 0 or 1 in a i32 representation which implies > > /// that `inner` is always valid as long as `_pad` stays 0. > > pub union Flag { > > pub(super) inner: FlagInner, > > pub flag: BoolFlag, > > } > > > > // can static_assert that `Flag` and `BoolFlag` has the same > > // alignement and size. > > > > then > > > > impl AtomicFlag { > > pub fn get_mut(&mut self) -> &mut bool { > > let flag = self.0.get_mut(); // <- &mut Flag > > > > // INVARIANTS: flag.inner._pad cannot be modified via the > > // returned reference. > > // SAFETY: Per type invariants, `flag.inner.bool_field` is > > // always a valid bool. > > unsafe { &mut flag.inner.bool_field } > > } > > } > > > > Thoughts? > > > > Regards, > > Boqun > > I think we really need special handling for endianness for this one single > function, so doing all the extra stuff feels really unnecessary. > First, this one single function changes the design actually, previously you can even implement a Flag as: enum Flag { Clear = 6, Set = 7, } and it'll work, that is as long as `Flag` behaves like a bool, it's fine. But now this function implies there is actually a bool in `Flag`, which is kinda totally different. Besides, by using the current implement, we set an example about "how to do a byte offset in an i32 for different endians", and then if anyone wanted to do something similar, very likely they would copy-paste and modify what we have here. The potential tech debts are significant. So I would like to do it in a right way ("right" is probably subjective, but it comes from someone who needs to live with the code as a maintainer ;-) and I'm happy to switch to a better way if necessary). Hope this can explain why I want to do this right now. Regards, Boqun > I prefer Fujita's current version. > > Best, > Gary > > > > >> + > >> + // SAFETY: `ptr` is valid for `size_of::<Flag>()` bytes; `offset` selects the LSB. > >> + unsafe { ptr.add(offset) } > >> + }; > >> + > >> + // SAFETY: The LSB holds `0`/`1` for `Flag::Clear/Set`, and `bool` is `i8`-sized/aligned. > >> + unsafe { &mut *byte_ptr.cast::<bool>() } > >> + } > >> + > >> /// Stores a value to the atomic flag and returns the previous value. > >> #[inline(always)] > >> pub fn xchg<Ordering: ordering::Ordering>(&self, b: bool, o: Ordering) -> bool { > >> diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs > >> index 11bc67ab70a3..e413b9e9fe21 100644 > >> --- a/rust/kernel/sync/atomic/predefine.rs > >> +++ b/rust/kernel/sync/atomic/predefine.rs > >> @@ -351,6 +351,23 @@ fn atomic_bool_tests() { > >> 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)); > >> + } > >> + > >> #[test] > >> fn atomic_ptr_tests() { > >> let mut v = 42; > >> -- > >> 2.43.0 > >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 15:59 ` Boqun Feng @ 2026-01-27 16:10 ` Gary Guo 2026-01-27 16:34 ` Boqun Feng 0 siblings, 1 reply; 15+ messages in thread From: Gary Guo @ 2026-01-27 16:10 UTC (permalink / raw) To: Boqun Feng, Gary Guo Cc: FUJITA Tomonori, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue Jan 27, 2026 at 3:59 PM GMT, Boqun Feng wrote: > On Tue, Jan 27, 2026 at 03:42:42PM +0000, Gary Guo wrote: >> On Tue Jan 27, 2026 at 3:35 PM GMT, Boqun Feng wrote: >> > [For some unknown reasons, I cannot send my reply via gmail hence reply >> > via kernel.org account, I might switch from gmail later on] >> > >> > On Tue, Jan 27, 2026 at 09:52:59PM +0900, FUJITA Tomonori wrote: >> >> From: FUJITA Tomonori <fujita.tomonori@gmail.com> >> >> >> >> AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to >> >> Atomic<T>::get_mut(). >> >> >> >> Also add kunit tests for AtomicFlag. >> >> >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> >> --- >> >> rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ >> >> rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ >> >> 2 files changed, 37 insertions(+) >> >> >> >> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs >> >> index 6c46335bdb8c..b6c01d9f3a46 100644 >> >> --- a/rust/kernel/sync/atomic.rs >> >> +++ b/rust/kernel/sync/atomic.rs >> >> @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) >> >> self.0.store(b.into(), o) >> >> } >> >> >> >> + /// 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. >> >> + pub fn get_mut(&mut self) -> &mut bool { >> >> + let byte_ptr = { >> >> + let ptr = self.0.as_ptr().cast::<u8>(); >> >> + let offset = if cfg!(target_endian = "big") { >> >> + core::mem::size_of::<Flag>() - 1 >> >> + } else { >> >> + 0 >> >> + }; >> > >> > The idea is solid, but I want to avoid endian handling in the function, >> > I would prefer a "struct declaration" solution like: >> > >> > #[cfg(target_endian = "big")] >> > #[repr(align(4))] >> > pub(super) struct FlagInner { >> > _pad: [i8; 3], >> > bool_field: bool, >> > } >> > >> > #[cfg(target_endian = "little")] >> > #[repr(align(4))] >> > struct FlagInner { >> > bool_field: bool, >> > _pad: [i8; 3], >> > } >> > >> > redefine `Flag` as `BoolFlag` >> > >> > #[repr(i32)] >> > pub enum BoolFlag { >> > Clear = 0, >> > Set = 1, >> > } >> > >> > and `Flag` becomes a union of `BoolFlag` and `FlagInner`: >> > >> > /// # Invariants >> > /// `Flag` is either 0 or 1 in a i32 representation which implies >> > /// that `inner` is always valid as long as `_pad` stays 0. >> > pub union Flag { >> > pub(super) inner: FlagInner, >> > pub flag: BoolFlag, >> > } >> > >> > // can static_assert that `Flag` and `BoolFlag` has the same >> > // alignement and size. >> > >> > then >> > >> > impl AtomicFlag { >> > pub fn get_mut(&mut self) -> &mut bool { >> > let flag = self.0.get_mut(); // <- &mut Flag >> > >> > // INVARIANTS: flag.inner._pad cannot be modified via the >> > // returned reference. >> > // SAFETY: Per type invariants, `flag.inner.bool_field` is >> > // always a valid bool. >> > unsafe { &mut flag.inner.bool_field } >> > } >> > } >> > >> > Thoughts? >> > >> > Regards, >> > Boqun >> >> I think we really need special handling for endianness for this one single >> function, so doing all the extra stuff feels really unnecessary. >> > > First, this one single function changes the design actually, previously > you can even implement a Flag as: > > enum Flag { > Clear = 6, > Set = 7, > } > > and it'll work, that is as long as `Flag` behaves like a bool, it's > fine. But now this function implies there is actually a bool in `Flag`, > which is kinda totally different. > > Besides, by using the current implement, we set an example about "how to > do a byte offset in an i32 for different endians", and then if anyone > wanted to do something similar, very likely they would copy-paste and > modify what we have here. The potential tech debts are significant. So I > would like to do it in a right way ("right" is probably subjective, but > it comes from someone who needs to live with the code as a maintainer > ;-) and I'm happy to switch to a better way if necessary). I think what Fujita has is more "proper". Your approach still have the issue of requiring a specific ordering of the fields. If this is messed up, then the entire thing is broken. I.e. the safety proof of `get_mut` depends on the fields being ordered correctly in `FlagInner`. If you want to go down this route then I would just scrap `enum Flag` all together and always define it as struct, with an internal `bool` + 3 bytes of zero padding. This way we don't even need unsafe for `get_mut`. Best, Gary > > Hope this can explain why I want to do this right now. > > Regards, > Boqun > >> I prefer Fujita's current version. >> >> Best, >> Gary >> >> > >> >> + >> >> + // SAFETY: `ptr` is valid for `size_of::<Flag>()` bytes; `offset` selects the LSB. >> >> + unsafe { ptr.add(offset) } >> >> + }; >> >> + >> >> + // SAFETY: The LSB holds `0`/`1` for `Flag::Clear/Set`, and `bool` is `i8`-sized/aligned. >> >> + unsafe { &mut *byte_ptr.cast::<bool>() } >> >> + } >> >> + >> >> /// Stores a value to the atomic flag and returns the previous value. >> >> #[inline(always)] >> >> pub fn xchg<Ordering: ordering::Ordering>(&self, b: bool, o: Ordering) -> bool { >> >> diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs >> >> index 11bc67ab70a3..e413b9e9fe21 100644 >> >> --- a/rust/kernel/sync/atomic/predefine.rs >> >> +++ b/rust/kernel/sync/atomic/predefine.rs >> >> @@ -351,6 +351,23 @@ fn atomic_bool_tests() { >> >> 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)); >> >> + } >> >> + >> >> #[test] >> >> fn atomic_ptr_tests() { >> >> let mut v = 42; >> >> -- >> >> 2.43.0 >> >> >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 16:10 ` Gary Guo @ 2026-01-27 16:34 ` Boqun Feng 2026-01-28 3:32 ` FUJITA Tomonori 0 siblings, 1 reply; 15+ messages in thread From: Boqun Feng @ 2026-01-27 16:34 UTC (permalink / raw) To: Gary Guo Cc: FUJITA Tomonori, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue, Jan 27, 2026 at 04:10:35PM +0000, Gary Guo wrote: > On Tue Jan 27, 2026 at 3:59 PM GMT, Boqun Feng wrote: > > On Tue, Jan 27, 2026 at 03:42:42PM +0000, Gary Guo wrote: > >> On Tue Jan 27, 2026 at 3:35 PM GMT, Boqun Feng wrote: > >> > [For some unknown reasons, I cannot send my reply via gmail hence reply > >> > via kernel.org account, I might switch from gmail later on] > >> > > >> > On Tue, Jan 27, 2026 at 09:52:59PM +0900, FUJITA Tomonori wrote: > >> >> From: FUJITA Tomonori <fujita.tomonori@gmail.com> > >> >> > >> >> AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to > >> >> Atomic<T>::get_mut(). > >> >> > >> >> Also add kunit tests for AtomicFlag. > >> >> > >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > >> >> --- > >> >> rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ > >> >> rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ > >> >> 2 files changed, 37 insertions(+) > >> >> > >> >> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > >> >> index 6c46335bdb8c..b6c01d9f3a46 100644 > >> >> --- a/rust/kernel/sync/atomic.rs > >> >> +++ b/rust/kernel/sync/atomic.rs > >> >> @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) > >> >> self.0.store(b.into(), o) > >> >> } > >> >> > >> >> + /// 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. > >> >> + pub fn get_mut(&mut self) -> &mut bool { > >> >> + let byte_ptr = { > >> >> + let ptr = self.0.as_ptr().cast::<u8>(); > >> >> + let offset = if cfg!(target_endian = "big") { > >> >> + core::mem::size_of::<Flag>() - 1 > >> >> + } else { > >> >> + 0 > >> >> + }; > >> > > >> > The idea is solid, but I want to avoid endian handling in the function, > >> > I would prefer a "struct declaration" solution like: > >> > > >> > #[cfg(target_endian = "big")] > >> > #[repr(align(4))] > >> > pub(super) struct FlagInner { > >> > _pad: [i8; 3], > >> > bool_field: bool, > >> > } > >> > > >> > #[cfg(target_endian = "little")] > >> > #[repr(align(4))] > >> > struct FlagInner { > >> > bool_field: bool, > >> > _pad: [i8; 3], > >> > } > >> > > >> > redefine `Flag` as `BoolFlag` > >> > > >> > #[repr(i32)] > >> > pub enum BoolFlag { > >> > Clear = 0, > >> > Set = 1, > >> > } > >> > > >> > and `Flag` becomes a union of `BoolFlag` and `FlagInner`: > >> > > >> > /// # Invariants > >> > /// `Flag` is either 0 or 1 in a i32 representation which implies > >> > /// that `inner` is always valid as long as `_pad` stays 0. > >> > pub union Flag { > >> > pub(super) inner: FlagInner, > >> > pub flag: BoolFlag, > >> > } > >> > > >> > // can static_assert that `Flag` and `BoolFlag` has the same > >> > // alignement and size. > >> > > >> > then > >> > > >> > impl AtomicFlag { > >> > pub fn get_mut(&mut self) -> &mut bool { > >> > let flag = self.0.get_mut(); // <- &mut Flag > >> > > >> > // INVARIANTS: flag.inner._pad cannot be modified via the > >> > // returned reference. > >> > // SAFETY: Per type invariants, `flag.inner.bool_field` is > >> > // always a valid bool. > >> > unsafe { &mut flag.inner.bool_field } > >> > } > >> > } > >> > > >> > Thoughts? > >> > > >> > Regards, > >> > Boqun > >> > >> I think we really need special handling for endianness for this one single > >> function, so doing all the extra stuff feels really unnecessary. > >> > > > > First, this one single function changes the design actually, previously > > you can even implement a Flag as: > > > > enum Flag { > > Clear = 6, > > Set = 7, > > } > > > > and it'll work, that is as long as `Flag` behaves like a bool, it's > > fine. But now this function implies there is actually a bool in `Flag`, > > which is kinda totally different. > > > > Besides, by using the current implement, we set an example about "how to > > do a byte offset in an i32 for different endians", and then if anyone > > wanted to do something similar, very likely they would copy-paste and > > modify what we have here. The potential tech debts are significant. So I > > would like to do it in a right way ("right" is probably subjective, but > > it comes from someone who needs to live with the code as a maintainer > > ;-) and I'm happy to switch to a better way if necessary). > > I think what Fujita has is more "proper". Your approach still have the issue of > requiring a specific ordering of the fields. If this is messed up, then the > entire thing is broken. I.e. the safety proof of `get_mut` depends on the fields > being ordered correctly in `FlagInner`. > > If you want to go down this route then I would just scrap `enum Flag` all > together and always define it as struct, with an internal `bool` + 3 bytes of > zero padding. This way we don't even need unsafe for `get_mut`. > Hmm.. so like: const PAD_SIZE: usize = <3 or 0 depending on ARCHs> /// # Invariants /// `pad` has to be all zero. struct Flag { bool_field: bool, pad: [i8; PAD_SIZE], } impl Flag { pub const fn set() -> Flag { Self { true, pad: [0; PAD_SIZE] } } pub const fn clear() -> Flag { Self { false, pad: [0; PAD_SIZE] } } } ? Yes, I think it's better ;-) Also, now given that `AtomicFlag` behaves exactly like a `Atomic<bool>`, should we do: /// `AtomicFlag` documentation here. #[cfg(<Arch supports byte-wise atomic>)] type AtomicFlag = Atomic<bool>; #[cfg(<Arch doesn't supprot byte-wise atomic>)] struct AtomicFlag(Atomic<BooleanFlag>); // `Flag` doesn't even need to be public. #[cfg(<Arch doesn't supprot byte-wise atomic>)] struct BooleanFlag { ... } (I renamed `Flag` -> `BooleanFlag`) Thoughts? I don't think there is any extra benefit of exposing `Atomic<BooleanFlag>`. Regards, Boqun > Best, > Gary > > > > Hope this can explain why I want to do this right now. > > > > Regards, > > Boqun > > > >> I prefer Fujita's current version. > >> > >> Best, > >> Gary > >> > >> > > >> >> + > >> >> + // SAFETY: `ptr` is valid for `size_of::<Flag>()` bytes; `offset` selects the LSB. > >> >> + unsafe { ptr.add(offset) } > >> >> + }; > >> >> + > >> >> + // SAFETY: The LSB holds `0`/`1` for `Flag::Clear/Set`, and `bool` is `i8`-sized/aligned. > >> >> + unsafe { &mut *byte_ptr.cast::<bool>() } > >> >> + } > >> >> + > >> >> /// Stores a value to the atomic flag and returns the previous value. > >> >> #[inline(always)] > >> >> pub fn xchg<Ordering: ordering::Ordering>(&self, b: bool, o: Ordering) -> bool { > >> >> diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs > >> >> index 11bc67ab70a3..e413b9e9fe21 100644 > >> >> --- a/rust/kernel/sync/atomic/predefine.rs > >> >> +++ b/rust/kernel/sync/atomic/predefine.rs > >> >> @@ -351,6 +351,23 @@ fn atomic_bool_tests() { > >> >> 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)); > >> >> + } > >> >> + > >> >> #[test] > >> >> fn atomic_ptr_tests() { > >> >> let mut v = 42; > >> >> -- > >> >> 2.43.0 > >> >> > >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-27 16:34 ` Boqun Feng @ 2026-01-28 3:32 ` FUJITA Tomonori 2026-01-28 3:38 ` Boqun Feng 0 siblings, 1 reply; 15+ messages in thread From: FUJITA Tomonori @ 2026-01-28 3:32 UTC (permalink / raw) To: boqun Cc: gary, tomo, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, lossin, mark.rutland, tmgross, rust-for-linux, fujita.tomonori On Tue, 27 Jan 2026 08:34:58 -0800 Boqun Feng <boqun@kernel.org> wrote: > On Tue, Jan 27, 2026 at 04:10:35PM +0000, Gary Guo wrote: >> On Tue Jan 27, 2026 at 3:59 PM GMT, Boqun Feng wrote: >> > On Tue, Jan 27, 2026 at 03:42:42PM +0000, Gary Guo wrote: >> >> On Tue Jan 27, 2026 at 3:35 PM GMT, Boqun Feng wrote: >> >> > [For some unknown reasons, I cannot send my reply via gmail hence reply >> >> > via kernel.org account, I might switch from gmail later on] >> >> > >> >> > On Tue, Jan 27, 2026 at 09:52:59PM +0900, FUJITA Tomonori wrote: >> >> >> From: FUJITA Tomonori <fujita.tomonori@gmail.com> >> >> >> >> >> >> AtomicFlag exposes a bool API, but it lacks a get_mut() equivalent to >> >> >> Atomic<T>::get_mut(). >> >> >> >> >> >> Also add kunit tests for AtomicFlag. >> >> >> >> >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> >> >> --- >> >> >> rust/kernel/sync/atomic.rs | 20 ++++++++++++++++++++ >> >> >> rust/kernel/sync/atomic/predefine.rs | 17 +++++++++++++++++ >> >> >> 2 files changed, 37 insertions(+) >> >> >> >> >> >> diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs >> >> >> index 6c46335bdb8c..b6c01d9f3a46 100644 >> >> >> --- a/rust/kernel/sync/atomic.rs >> >> >> +++ b/rust/kernel/sync/atomic.rs >> >> >> @@ -591,6 +591,26 @@ pub fn store<Ordering: ordering::ReleaseOrRelaxed>(&self, b: bool, o: Ordering) >> >> >> self.0.store(b.into(), o) >> >> >> } >> >> >> >> >> >> + /// 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. >> >> >> + pub fn get_mut(&mut self) -> &mut bool { >> >> >> + let byte_ptr = { >> >> >> + let ptr = self.0.as_ptr().cast::<u8>(); >> >> >> + let offset = if cfg!(target_endian = "big") { >> >> >> + core::mem::size_of::<Flag>() - 1 >> >> >> + } else { >> >> >> + 0 >> >> >> + }; >> >> > >> >> > The idea is solid, but I want to avoid endian handling in the function, >> >> > I would prefer a "struct declaration" solution like: >> >> > >> >> > #[cfg(target_endian = "big")] >> >> > #[repr(align(4))] >> >> > pub(super) struct FlagInner { >> >> > _pad: [i8; 3], >> >> > bool_field: bool, >> >> > } >> >> > >> >> > #[cfg(target_endian = "little")] >> >> > #[repr(align(4))] >> >> > struct FlagInner { >> >> > bool_field: bool, >> >> > _pad: [i8; 3], >> >> > } >> >> > >> >> > redefine `Flag` as `BoolFlag` >> >> > >> >> > #[repr(i32)] >> >> > pub enum BoolFlag { >> >> > Clear = 0, >> >> > Set = 1, >> >> > } >> >> > >> >> > and `Flag` becomes a union of `BoolFlag` and `FlagInner`: >> >> > >> >> > /// # Invariants >> >> > /// `Flag` is either 0 or 1 in a i32 representation which implies >> >> > /// that `inner` is always valid as long as `_pad` stays 0. >> >> > pub union Flag { >> >> > pub(super) inner: FlagInner, >> >> > pub flag: BoolFlag, >> >> > } >> >> > >> >> > // can static_assert that `Flag` and `BoolFlag` has the same >> >> > // alignement and size. >> >> > >> >> > then >> >> > >> >> > impl AtomicFlag { >> >> > pub fn get_mut(&mut self) -> &mut bool { >> >> > let flag = self.0.get_mut(); // <- &mut Flag >> >> > >> >> > // INVARIANTS: flag.inner._pad cannot be modified via the >> >> > // returned reference. >> >> > // SAFETY: Per type invariants, `flag.inner.bool_field` is >> >> > // always a valid bool. >> >> > unsafe { &mut flag.inner.bool_field } >> >> > } >> >> > } >> >> > >> >> > Thoughts? >> >> > >> >> > Regards, >> >> > Boqun >> >> >> >> I think we really need special handling for endianness for this one single >> >> function, so doing all the extra stuff feels really unnecessary. >> >> >> > >> > First, this one single function changes the design actually, previously >> > you can even implement a Flag as: >> > >> > enum Flag { >> > Clear = 6, >> > Set = 7, >> > } >> > >> > and it'll work, that is as long as `Flag` behaves like a bool, it's >> > fine. But now this function implies there is actually a bool in `Flag`, >> > which is kinda totally different. >> > >> > Besides, by using the current implement, we set an example about "how to >> > do a byte offset in an i32 for different endians", and then if anyone >> > wanted to do something similar, very likely they would copy-paste and >> > modify what we have here. The potential tech debts are significant. So I >> > would like to do it in a right way ("right" is probably subjective, but >> > it comes from someone who needs to live with the code as a maintainer >> > ;-) and I'm happy to switch to a better way if necessary). >> >> I think what Fujita has is more "proper". Your approach still have the issue of >> requiring a specific ordering of the fields. If this is messed up, then the >> entire thing is broken. I.e. the safety proof of `get_mut` depends on the fields >> being ordered correctly in `FlagInner`. >> >> If you want to go down this route then I would just scrap `enum Flag` all >> together and always define it as struct, with an internal `bool` + 3 bytes of >> zero padding. This way we don't even need unsafe for `get_mut`. >> > > Hmm.. so like: > > const PAD_SIZE: usize = <3 or 0 depending on ARCHs> > > /// # Invariants > /// `pad` has to be all zero. > struct Flag { > bool_field: bool, > pad: [i8; PAD_SIZE], > } > > impl Flag { > pub const fn set() -> Flag { > Self { true, pad: [0; PAD_SIZE] } > } > > pub const fn clear() -> Flag { > Self { false, pad: [0; PAD_SIZE] } > } > } > > ? > > Yes, I think it's better ;-) > > Also, now given that `AtomicFlag` behaves exactly like a `Atomic<bool>`, > should we do: > > /// `AtomicFlag` documentation here. > #[cfg(<Arch supports byte-wise atomic>)] > type AtomicFlag = Atomic<bool>; > #[cfg(<Arch doesn't supprot byte-wise atomic>)] > struct AtomicFlag(Atomic<BooleanFlag>); > > // `Flag` doesn't even need to be public. > #[cfg(<Arch doesn't supprot byte-wise atomic>)] > struct BooleanFlag { ... } > > (I renamed `Flag` -> `BooleanFlag`) > > Thoughts? I don't think there is any extra benefit of exposing > `Atomic<BooleanFlag>`. Agreed. You prefer the cleaner git history? Then I'll send the next version for rust-sync-fixes, to implement BooleanFlag (or Flag) and AtomicFlag from scratch. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut 2026-01-28 3:32 ` FUJITA Tomonori @ 2026-01-28 3:38 ` Boqun Feng 0 siblings, 0 replies; 15+ messages in thread From: Boqun Feng @ 2026-01-28 3:38 UTC (permalink / raw) To: FUJITA Tomonori Cc: gary, ojeda, peterz, will, a.hindborg, aliceryhl, bjorn3_gh, dakr, lossin, mark.rutland, tmgross, rust-for-linux, fujita.tomonori On Wed, Jan 28, 2026 at 12:32:12PM +0900, FUJITA Tomonori wrote: [...] > >> > >> I think what Fujita has is more "proper". Your approach still have the issue of > >> requiring a specific ordering of the fields. If this is messed up, then the > >> entire thing is broken. I.e. the safety proof of `get_mut` depends on the fields > >> being ordered correctly in `FlagInner`. > >> > >> If you want to go down this route then I would just scrap `enum Flag` all > >> together and always define it as struct, with an internal `bool` + 3 bytes of > >> zero padding. This way we don't even need unsafe for `get_mut`. > >> > > > > Hmm.. so like: > > > > const PAD_SIZE: usize = <3 or 0 depending on ARCHs> > > > > /// # Invariants > > /// `pad` has to be all zero. > > struct Flag { > > bool_field: bool, > > pad: [i8; PAD_SIZE], > > } > > > > impl Flag { > > pub const fn set() -> Flag { > > Self { true, pad: [0; PAD_SIZE] } > > } > > > > pub const fn clear() -> Flag { > > Self { false, pad: [0; PAD_SIZE] } > > } > > } > > > > ? > > > > Yes, I think it's better ;-) > > > > Also, now given that `AtomicFlag` behaves exactly like a `Atomic<bool>`, > > should we do: > > > > /// `AtomicFlag` documentation here. > > #[cfg(<Arch supports byte-wise atomic>)] > > type AtomicFlag = Atomic<bool>; > > #[cfg(<Arch doesn't supprot byte-wise atomic>)] > > struct AtomicFlag(Atomic<BooleanFlag>); > > > > // `Flag` doesn't even need to be public. > > #[cfg(<Arch doesn't supprot byte-wise atomic>)] > > struct BooleanFlag { ... } > > > > (I renamed `Flag` -> `BooleanFlag`) > > > > Thoughts? I don't think there is any extra benefit of exposing > > `Atomic<BooleanFlag>`. > > Agreed. > > You prefer the cleaner git history? Then I'll send the next version > for rust-sync-fixes, to implement BooleanFlag (or Flag) and AtomicFlag > from scratch. > Yes, that'll be great, thank you! Regards, Boqun ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] rust: list: Use AtomicFlag in AtomicTracker 2026-01-27 12:52 [PATCH v2 0/2] rust: add AtomicFlag::get_mut FUJITA Tomonori 2026-01-27 12:52 ` [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut FUJITA Tomonori @ 2026-01-27 12:53 ` FUJITA Tomonori 2026-01-27 13:54 ` Gary Guo 1 sibling, 1 reply; 15+ messages in thread From: FUJITA Tomonori @ 2026-01-27 12:53 UTC (permalink / raw) To: boqun.feng, ojeda, peterz, will Cc: a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori From: FUJITA Tomonori <fujita.tomonori@gmail.com> Make AtomicTracker use AtomicFlag instead of Atomic<bool> to avoid slow byte-sized RMWs on architectures that don't support them. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/list/arc.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/kernel/list/arc.rs b/rust/kernel/list/arc.rs index 2282f33913ee..5e84f500a3fe 100644 --- a/rust/kernel/list/arc.rs +++ b/rust/kernel/list/arc.rs @@ -6,7 +6,7 @@ use crate::alloc::{AllocError, Flags}; use crate::prelude::*; -use crate::sync::atomic::{ordering, Atomic}; +use crate::sync::atomic::{ordering, AtomicFlag}; use crate::sync::{Arc, ArcBorrow, UniqueArc}; use core::marker::PhantomPinned; use core::ops::Deref; @@ -469,7 +469,7 @@ impl<T, U, const ID: u64> core::ops::DispatchFromDyn<ListArc<U, ID>> for ListArc /// If the boolean is `false`, then there is no [`ListArc`] for this value. #[repr(transparent)] pub struct AtomicTracker<const ID: u64 = 0> { - inner: Atomic<bool>, + inner: AtomicFlag, // This value needs to be pinned to justify the INVARIANT: comment in `AtomicTracker::new`. _pin: PhantomPinned, } @@ -480,12 +480,12 @@ pub fn new() -> impl PinInit<Self> { // INVARIANT: Pin-init initializers can't be used on an existing `Arc`, so this value will // not be constructed in an `Arc` that already has a `ListArc`. Self { - inner: Atomic::new(false), + inner: AtomicFlag::new(false), _pin: PhantomPinned, } } - fn project_inner(self: Pin<&mut Self>) -> &mut Atomic<bool> { + fn project_inner(self: Pin<&mut Self>) -> &mut AtomicFlag { // SAFETY: The `inner` field is not structurally pinned, so we may obtain a mutable // reference to it even if we only have a pinned reference to `self`. unsafe { &mut Pin::into_inner_unchecked(self).inner } -- 2.43.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] rust: list: Use AtomicFlag in AtomicTracker 2026-01-27 12:53 ` [PATCH v2 2/2] rust: list: Use AtomicFlag in AtomicTracker FUJITA Tomonori @ 2026-01-27 13:54 ` Gary Guo 0 siblings, 0 replies; 15+ messages in thread From: Gary Guo @ 2026-01-27 13:54 UTC (permalink / raw) To: FUJITA Tomonori, boqun.feng, ojeda, peterz, will Cc: a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, mark.rutland, tmgross, rust-for-linux, FUJITA Tomonori On Tue Jan 27, 2026 at 12:53 PM GMT, FUJITA Tomonori wrote: > From: FUJITA Tomonori <fujita.tomonori@gmail.com> > > Make AtomicTracker use AtomicFlag instead of Atomic<bool> to avoid > slow byte-sized RMWs on architectures that don't support them. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> The diff is neat! Reviewed-by: Gary Guo <gary@garyguo.net> > --- > rust/kernel/list/arc.rs | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-01-28 3:55 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-27 12:52 [PATCH v2 0/2] rust: add AtomicFlag::get_mut FUJITA Tomonori 2026-01-27 12:52 ` [PATCH v2 1/2] rust: sync: atomic: Add AtomicFlag::get_mut FUJITA Tomonori 2026-01-27 13:53 ` Gary Guo 2026-01-27 14:32 ` Miguel Ojeda 2026-01-28 3:47 ` FUJITA Tomonori 2026-01-28 3:55 ` Boqun Feng 2026-01-27 15:35 ` Boqun Feng 2026-01-27 15:42 ` Gary Guo 2026-01-27 15:59 ` Boqun Feng 2026-01-27 16:10 ` Gary Guo 2026-01-27 16:34 ` Boqun Feng 2026-01-28 3:32 ` FUJITA Tomonori 2026-01-28 3:38 ` Boqun Feng 2026-01-27 12:53 ` [PATCH v2 2/2] rust: list: Use AtomicFlag in AtomicTracker FUJITA Tomonori 2026-01-27 13:54 ` Gary Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox