* [PATCH v1 0/2] Add support for print exactly once @ 2025-11-05 5:47 ` FUJITA Tomonori 2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori ` (3 more replies) 0 siblings, 4 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-05 5:47 UTC (permalink / raw) To: ojeda Cc: a.hindborg, aliceryhl, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io This adds the Rust equivalent of the kernel's DO_ONCE_LITE and pr_*_once macros. A proposal for this feature was made in the past [1], but it didn't reach consensus on the implementation and wasn't merged. After reading the previous discussions, I implemented it using a different approach. In the previous proposal, a structure equivalent to std::sync::Once was implemented to realize the DO_ONCE_LITE macro. The approach tried to provide Once-like semantics by using two atomic values. As pointed out in the previous review comments, I think this approach tries to provide more functionality than needed, making it unnecessarily complex. Also, because data structures in the .data..once section can be cleared at any time (via debugfs clear_warn_once), an implementation using two atomics wouldn't work correctly. Therefore, I decided to drop the idea of emulating Once and took a minimal approach to implement DO_ONCE_LITE with only one atomic variable. While it would be possible to implement the feature entirely as a Rust macro, the functionality that can be implemented as regular functions has been extracted and implemented as the OnceLite struct for better code readability. Of course, unlike the previous proposal, this uses LKMM atomics. [1] https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/ FUJITA Tomonori (2): rust: Add support for calling a function exactly once rust: Add pr_*_once macros rust/kernel/lib.rs | 1 + rust/kernel/once_lite.rs | 71 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/print.rs | 70 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 rust/kernel/once_lite.rs base-commit: 3b83f5d5e78ac5cddd811a5e431af73959864390 -- 2.43.0 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 5:47 ` [PATCH v1 0/2] Add support for print exactly once FUJITA Tomonori @ 2025-11-05 5:47 ` FUJITA Tomonori 2025-11-05 9:21 ` Onur Özkan ` (2 more replies) 2025-11-05 5:47 ` [PATCH v1 2/2] rust: Add pr_*_once macros FUJITA Tomonori ` (2 subsequent siblings) 3 siblings, 3 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-05 5:47 UTC (permalink / raw) To: ojeda Cc: a.hindborg, aliceryhl, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io Add the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. While it would be possible to implement the feature entirely as a Rust macro, the functionality that can be implemented as regular functions has been extracted and implemented as the `OnceLite` struct for better code maintainability. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/lib.rs | 1 + rust/kernel/once_lite.rs | 71 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 rust/kernel/once_lite.rs diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 3dd7bebe7888..19553eb8c188 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -110,6 +110,7 @@ #[cfg(CONFIG_NET)] pub mod net; pub mod of; +pub mod once_lite; #[cfg(CONFIG_PM_OPP)] pub mod opp; pub mod page; diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs new file mode 100644 index 000000000000..44ad5dbdc67e --- /dev/null +++ b/rust/kernel/once_lite.rs @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Support for calling a function exactly once. +//! +//! C header: [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) + +use crate::sync::atomic::{Atomic, AtomicType, Relaxed}; + +/// A lightweight `call_once` primitive. +/// +/// This structure provides the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. +/// While it would be possible to implement the feature entirely as a Rust macro, +/// the functionality that can be implemented as regular functions has been +/// extracted and implemented as the `OnceLite` struct for better code maintainability. +pub struct OnceLite(Atomic<State>); + +#[derive(Clone, Copy, PartialEq, Eq)] +#[repr(i32)] +enum State { + Incomplete = 0, + Complete = 1, +} + +// SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip +// transmutable to `i32`. +unsafe impl AtomicType for State { + type Repr = i32; +} + +impl OnceLite { + /// Creates a new [`OnceLite`] in the incomplete state. + #[inline(always)] + #[allow(clippy::new_without_default)] + pub const fn new() -> Self { + OnceLite(Atomic::new(State::Incomplete)) + } + + /// Calls the provided function exactly once. + pub fn call_once<F>(&self, f: F) -> bool + where + F: FnOnce(), + { + let old = self.0.xchg(State::Complete, Relaxed); + if old == State::Complete { + return false; + } + + f(); + true + } +} + +/// Run the given function exactly once. +/// +/// This is equivalent to the kernel's `DO_ONCE_LITE` macro. +/// +/// # Examples +/// +/// ``` +/// kernel::do_once_lite!(|| { +/// kernel::pr_info!("This will be printed only once\n"); +/// }); +/// ``` +#[macro_export] +macro_rules! do_once_lite { + ($e:expr) => {{ + #[link_section = ".data..once"] + static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); + ONCE.call_once($e) + }}; +} -- 2.43.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori @ 2025-11-05 9:21 ` Onur Özkan 2025-11-05 10:35 ` Alice Ryhl 2025-11-05 10:32 ` Alice Ryhl 2025-11-05 16:19 ` Boqun Feng 2 siblings, 1 reply; 52+ messages in thread From: Onur Özkan @ 2025-11-05 9:21 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, aliceryhl, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Wed, 5 Nov 2025 14:47:30 +0900 FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > Add the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. While it > would be possible to implement the feature entirely as a Rust macro, > the functionality that can be implemented as regular functions has > been extracted and implemented as the `OnceLite` struct for better > code maintainability. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/lib.rs | 1 + > rust/kernel/once_lite.rs | 71 > ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 > insertions(+) create mode 100644 rust/kernel/once_lite.rs > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 3dd7bebe7888..19553eb8c188 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -110,6 +110,7 @@ > #[cfg(CONFIG_NET)] > pub mod net; > pub mod of; > +pub mod once_lite; > #[cfg(CONFIG_PM_OPP)] > pub mod opp; > pub mod page; > diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs > new file mode 100644 > index 000000000000..44ad5dbdc67e > --- /dev/null > +++ b/rust/kernel/once_lite.rs > @@ -0,0 +1,71 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Support for calling a function exactly once. > +//! > +//! C header: > [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) + This seems like a pure Rust implementation. What exactly do we need from the "once_lite.h" in this module? -Onur > +use crate::sync::atomic::{Atomic, AtomicType, Relaxed}; > + > +/// A lightweight `call_once` primitive. > +/// > +/// This structure provides the Rust equivalent of the kernel's > `DO_ONCE_LITE` macro. +/// While it would be possible to implement > the feature entirely as a Rust macro, +/// the functionality that can > be implemented as regular functions has been +/// extracted and > implemented as the `OnceLite` struct for better code maintainability. > +pub struct OnceLite(Atomic<State>); + > +#[derive(Clone, Copy, PartialEq, Eq)] > +#[repr(i32)] > +enum State { > + Incomplete = 0, > + Complete = 1, > +} > + > +// SAFETY: `State` and `i32` has the same size and alignment, and > it's round-trip +// transmutable to `i32`. > +unsafe impl AtomicType for State { > + type Repr = i32; > +} > + > +impl OnceLite { > + /// Creates a new [`OnceLite`] in the incomplete state. > + #[inline(always)] > + #[allow(clippy::new_without_default)] > + pub const fn new() -> Self { > + OnceLite(Atomic::new(State::Incomplete)) > + } > + > + /// Calls the provided function exactly once. > + pub fn call_once<F>(&self, f: F) -> bool > + where > + F: FnOnce(), > + { > + let old = self.0.xchg(State::Complete, Relaxed); > + if old == State::Complete { > + return false; > + } > + > + f(); > + true > + } > +} > + > +/// Run the given function exactly once. > +/// > +/// This is equivalent to the kernel's `DO_ONCE_LITE` macro. > +/// > +/// # Examples > +/// > +/// ``` > +/// kernel::do_once_lite!(|| { > +/// kernel::pr_info!("This will be printed only once\n"); > +/// }); > +/// ``` > +#[macro_export] > +macro_rules! do_once_lite { > + ($e:expr) => {{ > + #[link_section = ".data..once"] > + static ONCE: $crate::once_lite::OnceLite = > $crate::once_lite::OnceLite::new(); > + ONCE.call_once($e) > + }}; > +} ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 9:21 ` Onur Özkan @ 2025-11-05 10:35 ` Alice Ryhl 0 siblings, 0 replies; 52+ messages in thread From: Alice Ryhl @ 2025-11-05 10:35 UTC (permalink / raw) To: Onur Özkan Cc: FUJITA Tomonori, ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Wed, Nov 5, 2025 at 10:21 AM Onur Özkan <work@onurozkan.dev> wrote: > > On Wed, 5 Nov 2025 14:47:30 +0900 > FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > > Add the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. While it > > would be possible to implement the feature entirely as a Rust macro, > > the functionality that can be implemented as regular functions has > > been extracted and implemented as the `OnceLite` struct for better > > code maintainability. > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > --- > > rust/kernel/lib.rs | 1 + > > rust/kernel/once_lite.rs | 71 > > ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 > > insertions(+) create mode 100644 rust/kernel/once_lite.rs > > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > index 3dd7bebe7888..19553eb8c188 100644 > > --- a/rust/kernel/lib.rs > > +++ b/rust/kernel/lib.rs > > @@ -110,6 +110,7 @@ > > #[cfg(CONFIG_NET)] > > pub mod net; > > pub mod of; > > +pub mod once_lite; > > #[cfg(CONFIG_PM_OPP)] > > pub mod opp; > > pub mod page; > > diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs > > new file mode 100644 > > index 000000000000..44ad5dbdc67e > > --- /dev/null > > +++ b/rust/kernel/once_lite.rs > > @@ -0,0 +1,71 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Support for calling a function exactly once. > > +//! > > +//! C header: > > [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) + > > This seems like a pure Rust implementation. What exactly do we need > from the "once_lite.h" in this module? It's just a documentation link. I don't think it hurts for the Rust impl to link to the equivalent C feature. Alice ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori 2025-11-05 9:21 ` Onur Özkan @ 2025-11-05 10:32 ` Alice Ryhl 2025-11-06 0:34 ` FUJITA Tomonori 2025-11-05 16:19 ` Boqun Feng 2 siblings, 1 reply; 52+ messages in thread From: Alice Ryhl @ 2025-11-05 10:32 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Wed, Nov 05, 2025 at 02:47:30PM +0900, FUJITA Tomonori wrote: > Add the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. While it > would be possible to implement the feature entirely as a Rust macro, > the functionality that can be implemented as regular functions has > been extracted and implemented as the `OnceLite` struct for better > code maintainability. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> The OnceLite implementation LGTM, but I have suggestions for the macro. > +/// Run the given function exactly once. > +/// > +/// This is equivalent to the kernel's `DO_ONCE_LITE` macro. > +/// > +/// # Examples > +/// > +/// ``` > +/// kernel::do_once_lite!(|| { > +/// kernel::pr_info!("This will be printed only once\n"); > +/// }); > +/// ``` > +#[macro_export] > +macro_rules! do_once_lite { > + ($e:expr) => {{ > + #[link_section = ".data..once"] > + static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); > + ONCE.call_once($e) > + }}; > +} Wouldn't it be nicer if the macro syntax looks like this? kernel::do_once_lite! { kernel::pr_info!("This will be printed only once\n"); } You might implement it like this: macro_rules! do_once_lite { { $($e:tt)* } => {{ #[link_section = ".data..once"] static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); ONCE.call_once(|| { $($e)* }) }}; } Alice ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 10:32 ` Alice Ryhl @ 2025-11-06 0:34 ` FUJITA Tomonori 0 siblings, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-06 0:34 UTC (permalink / raw) To: aliceryhl Cc: fujita.tomonori, ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross On Wed, 5 Nov 2025 10:32:41 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > On Wed, Nov 05, 2025 at 02:47:30PM +0900, FUJITA Tomonori wrote: >> Add the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. While it >> would be possible to implement the feature entirely as a Rust macro, >> the functionality that can be implemented as regular functions has >> been extracted and implemented as the `OnceLite` struct for better >> code maintainability. >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > The OnceLite implementation LGTM, but I have suggestions for the macro. > >> +/// Run the given function exactly once. >> +/// >> +/// This is equivalent to the kernel's `DO_ONCE_LITE` macro. >> +/// >> +/// # Examples >> +/// >> +/// ``` >> +/// kernel::do_once_lite!(|| { >> +/// kernel::pr_info!("This will be printed only once\n"); >> +/// }); >> +/// ``` >> +#[macro_export] >> +macro_rules! do_once_lite { >> + ($e:expr) => {{ >> + #[link_section = ".data..once"] >> + static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); >> + ONCE.call_once($e) >> + }}; >> +} > > Wouldn't it be nicer if the macro syntax looks like this? > > kernel::do_once_lite! { > kernel::pr_info!("This will be printed only once\n"); > } > > You might implement it like this: > > macro_rules! do_once_lite { > { $($e:tt)* } => {{ > #[link_section = ".data..once"] > static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); > ONCE.call_once(|| { > $($e)* > }) > }}; > } Indeed, looks more readable and flexible. I'll use the syntax in the next version. Thanks! ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori 2025-11-05 9:21 ` Onur Özkan 2025-11-05 10:32 ` Alice Ryhl @ 2025-11-05 16:19 ` Boqun Feng 2025-11-06 0:10 ` FUJITA Tomonori 2 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-05 16:19 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io Hi Tomo, On Wed, Nov 05, 2025 at 02:47:30PM +0900, FUJITA Tomonori wrote: > Add the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. While it > would be possible to implement the feature entirely as a Rust macro, > the functionality that can be implemented as regular functions has > been extracted and implemented as the `OnceLite` struct for better > code maintainability. > Thanks for the patch. > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/lib.rs | 1 + > rust/kernel/once_lite.rs | 71 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > create mode 100644 rust/kernel/once_lite.rs > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 3dd7bebe7888..19553eb8c188 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -110,6 +110,7 @@ > #[cfg(CONFIG_NET)] > pub mod net; > pub mod of; > +pub mod once_lite; Would it make more sense to put it the kernel::sync module? > #[cfg(CONFIG_PM_OPP)] > pub mod opp; > pub mod page; > diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs > new file mode 100644 > index 000000000000..44ad5dbdc67e > --- /dev/null > +++ b/rust/kernel/once_lite.rs > @@ -0,0 +1,71 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Support for calling a function exactly once. > +//! > +//! C header: [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) > + > +use crate::sync::atomic::{Atomic, AtomicType, Relaxed}; > + > +/// A lightweight `call_once` primitive. > +/// > +/// This structure provides the Rust equivalent of the kernel's `DO_ONCE_LITE` macro. > +/// While it would be possible to implement the feature entirely as a Rust macro, > +/// the functionality that can be implemented as regular functions has been > +/// extracted and implemented as the `OnceLite` struct for better code maintainability. > +pub struct OnceLite(Atomic<State>); > + > +#[derive(Clone, Copy, PartialEq, Eq)] > +#[repr(i32)] > +enum State { > + Incomplete = 0, > + Complete = 1, > +} > + > +// SAFETY: `State` and `i32` has the same size and alignment, and it's round-trip > +// transmutable to `i32`. > +unsafe impl AtomicType for State { > + type Repr = i32; > +} > + > +impl OnceLite { > + /// Creates a new [`OnceLite`] in the incomplete state. > + #[inline(always)] > + #[allow(clippy::new_without_default)] > + pub const fn new() -> Self { > + OnceLite(Atomic::new(State::Incomplete)) > + } > + > + /// Calls the provided function exactly once. I think a few more comments here won't hurt: /// There is no other synchronization between two `call_once()`s /// except that only one will execute `f`, in other words, callers /// should not use a failed `call_once()` as a proof that another /// `call_once()` has already finished and the effect is observable /// to this thread. Thoughts? > + pub fn call_once<F>(&self, f: F) -> bool > + where > + F: FnOnce(), > + { > + let old = self.0.xchg(State::Complete, Relaxed); And we probably want a // ORDERING: comment here to explain why `Relaxed` is used here (because of the semantics mentioned above in the comment). Otherwise it looks good to me. Thanks! Regards, Boqun > + if old == State::Complete { > + return false; > + } > + > + f(); > + true > + } > +} > + > +/// Run the given function exactly once. > +/// > +/// This is equivalent to the kernel's `DO_ONCE_LITE` macro. > +/// > +/// # Examples > +/// > +/// ``` > +/// kernel::do_once_lite!(|| { > +/// kernel::pr_info!("This will be printed only once\n"); > +/// }); > +/// ``` > +#[macro_export] > +macro_rules! do_once_lite { > + ($e:expr) => {{ > + #[link_section = ".data..once"] > + static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); > + ONCE.call_once($e) > + }}; > +} > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-05 16:19 ` Boqun Feng @ 2025-11-06 0:10 ` FUJITA Tomonori 2025-11-06 14:46 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-06 0:10 UTC (permalink / raw) To: boqun.feng Cc: fujita.tomonori, ojeda, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Wed, 5 Nov 2025 08:19:37 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> index 3dd7bebe7888..19553eb8c188 100644 >> --- a/rust/kernel/lib.rs >> +++ b/rust/kernel/lib.rs >> @@ -110,6 +110,7 @@ >> #[cfg(CONFIG_NET)] >> pub mod net; >> pub mod of; >> +pub mod once_lite; > > Would it make more sense to put it the kernel::sync module? I actually considered that as well. The OnceLite structure could probably live under the sync module. However, the do_once_lite macro places its data in the .data..once section, which can be zero-cleared when a user writes to debugfs clear_warn_once. In that case, there is no guarantee of any atomicity, so it doesn't really fit the semantics expected for the sync module. For now, OnceLite the only used by do_once_lite macro, so I didn't see much benefit in splitting it into separate files. Also, data placed in the .data..once section should ideally be of a type whose zero-cleared state is clearly valid, which makes it doubtful that OnceLite would be generally useful in the way that other synchronization primitives in sync are. From a Rust perspective, data that is shared with the C side and can be zero-cleared at any time might ideally require some special structures? However, what we actually want to achieve here is simply something like "probably print only once", which is a very simple use case. So I'm not sure it's worth introducing something complicated. >> +impl OnceLite { >> + /// Creates a new [`OnceLite`] in the incomplete state. >> + #[inline(always)] >> + #[allow(clippy::new_without_default)] >> + pub const fn new() -> Self { >> + OnceLite(Atomic::new(State::Incomplete)) >> + } >> + >> + /// Calls the provided function exactly once. > > I think a few more comments here won't hurt: > > /// There is no other synchronization between two `call_once()`s > /// except that only one will execute `f`, in other words, callers > /// should not use a failed `call_once()` as a proof that another > /// `call_once()` has already finished and the effect is observable > /// to this thread. > > Thoughts? I don't expect OnceLite to be used in cases where it matters whether another thread has completed call_once(), but adding the above comment wouldn't hurt, I think. >> + pub fn call_once<F>(&self, f: F) -> bool >> + where >> + F: FnOnce(), >> + { >> + let old = self.0.xchg(State::Complete, Relaxed); > > And we probably want a // ORDERING: comment here to explain why > `Relaxed` is used here (because of the semantics mentioned above in the > comment). What kind of comment do you think would be appropriate here? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-06 0:10 ` FUJITA Tomonori @ 2025-11-06 14:46 ` Boqun Feng 2025-11-07 9:03 ` Alice Ryhl 0 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-06 14:46 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, Nov 06, 2025 at 09:10:26AM +0900, FUJITA Tomonori wrote: > On Wed, 5 Nov 2025 08:19:37 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > >> index 3dd7bebe7888..19553eb8c188 100644 > >> --- a/rust/kernel/lib.rs > >> +++ b/rust/kernel/lib.rs > >> @@ -110,6 +110,7 @@ > >> #[cfg(CONFIG_NET)] > >> pub mod net; > >> pub mod of; > >> +pub mod once_lite; > > > > Would it make more sense to put it the kernel::sync module? > > I actually considered that as well. The OnceLite structure could > probably live under the sync module. > > However, the do_once_lite macro places its data in the .data..once > section, which can be zero-cleared when a user writes to debugfs > clear_warn_once. In that case, there is no guarantee of any atomicity, This is not true actually, clear_warn_once_set() uses memset() to zero the memory, which is usually implemented by kernel itself and at least indicates per-byte atomicity, so it totally works with other atomic accesses in the kernel memory model. Otherwise pr_*_once() will be considered as data races. > so it doesn't really fit the semantics expected for the sync module. > > For now, OnceLite the only used by do_once_lite macro, so I didn't see > much benefit in splitting it into separate files. I lean towards Andreas' suggestion that we should use SetOnce() here if possible. I was missing that before this reply, thank Andrea for bring it up. > Also, data placed in the .data..once section should ideally be of a > type whose zero-cleared state is clearly valid, which makes it > doubtful that OnceLite would be generally useful in the way that other > synchronization primitives in sync are. > Why? A lot of synchronization primitives have 0 as a valid value, no? > From a Rust perspective, data that is shared with the C side and can > be zero-cleared at any time might ideally require some special > structures? However, what we actually want to achieve here is simply I don't think special structures are required or it's already implemented in our Atomic type. > something like "probably print only once", which is a very simple use > case. So I'm not sure it's worth introducing something complicated. > That's my point (and probably also Andreas' point), we already has the type `SetOnce` to do this, no need for a `OnceLite` type if not necessary, and the fact that it can be zero'd by debugfs doesn't change it as I explained above. > > >> +impl OnceLite { > >> + /// Creates a new [`OnceLite`] in the incomplete state. > >> + #[inline(always)] > >> + #[allow(clippy::new_without_default)] > >> + pub const fn new() -> Self { > >> + OnceLite(Atomic::new(State::Incomplete)) > >> + } > >> + > >> + /// Calls the provided function exactly once. > > > > I think a few more comments here won't hurt: > > > > /// There is no other synchronization between two `call_once()`s > > /// except that only one will execute `f`, in other words, callers > > /// should not use a failed `call_once()` as a proof that another > > /// `call_once()` has already finished and the effect is observable > > /// to this thread. > > > > Thoughts? > > I don't expect OnceLite to be used in cases where it matters whether > another thread has completed call_once(), but adding the above comment > wouldn't hurt, I think. > > > >> + pub fn call_once<F>(&self, f: F) -> bool > >> + where > >> + F: FnOnce(), > >> + { > >> + let old = self.0.xchg(State::Complete, Relaxed); > > > > And we probably want a // ORDERING: comment here to explain why > > `Relaxed` is used here (because of the semantics mentioned above in the > > comment). > > What kind of comment do you think would be appropriate here? > If we still need this (i.e not using SetOnce), then something like: // ORDERING: `Relaxed` is used here since no synchronization is required // for `call_once()`. Regards, Boqun ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-06 14:46 ` Boqun Feng @ 2025-11-07 9:03 ` Alice Ryhl 2025-11-10 9:21 ` FUJITA Tomonori 0 siblings, 1 reply; 52+ messages in thread From: Alice Ryhl @ 2025-11-07 9:03 UTC (permalink / raw) To: Boqun Feng Cc: FUJITA Tomonori, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, Nov 06, 2025 at 06:46:39AM -0800, Boqun Feng wrote: > On Thu, Nov 06, 2025 at 09:10:26AM +0900, FUJITA Tomonori wrote: > > On Wed, 5 Nov 2025 08:19:37 -0800 > > Boqun Feng <boqun.feng@gmail.com> wrote: > > > > >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > >> index 3dd7bebe7888..19553eb8c188 100644 > > >> --- a/rust/kernel/lib.rs > > >> +++ b/rust/kernel/lib.rs > > >> @@ -110,6 +110,7 @@ > > >> #[cfg(CONFIG_NET)] > > >> pub mod net; > > >> pub mod of; > > >> +pub mod once_lite; > > > > > > Would it make more sense to put it the kernel::sync module? > > > > I actually considered that as well. The OnceLite structure could > > probably live under the sync module. > > > > However, the do_once_lite macro places its data in the .data..once > > section, which can be zero-cleared when a user writes to debugfs > > clear_warn_once. In that case, there is no guarantee of any atomicity, > > This is not true actually, clear_warn_once_set() uses memset() to zero > the memory, which is usually implemented by kernel itself and at least > indicates per-byte atomicity, so it totally works with other atomic > accesses in the kernel memory model. Otherwise pr_*_once() will be > considered as data races. > > > so it doesn't really fit the semantics expected for the sync module. > > > > For now, OnceLite the only used by do_once_lite macro, so I didn't see > > much benefit in splitting it into separate files. > > I lean towards Andreas' suggestion that we should use SetOnce() here if > possible. I was missing that before this reply, thank Andrea for bring > it up. > > > Also, data placed in the .data..once section should ideally be of a > > type whose zero-cleared state is clearly valid, which makes it > > doubtful that OnceLite would be generally useful in the way that other > > synchronization primitives in sync are. > > > > Why? A lot of synchronization primitives have 0 as a valid value, no? > > > From a Rust perspective, data that is shared with the C side and can > > be zero-cleared at any time might ideally require some special > > structures? However, what we actually want to achieve here is simply > > I don't think special structures are required or it's already > implemented in our Atomic type. > > > something like "probably print only once", which is a very simple use > > case. So I'm not sure it's worth introducing something complicated. > > > > That's my point (and probably also Andreas' point), we already has the > type `SetOnce` to do this, no need for a `OnceLite` type if not > necessary, and the fact that it can be zero'd by debugfs doesn't change > it as I explained above. The SetOnce type doesn't do the same thing as OnceLite. SetOnce has three different states, but OnceLite only needs two. I don't think we should be reusing SetOnce here. Alice ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-07 9:03 ` Alice Ryhl @ 2025-11-10 9:21 ` FUJITA Tomonori 2025-11-10 16:14 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-10 9:21 UTC (permalink / raw) To: aliceryhl, boqun.feng, a.hindborg Cc: ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Fri, 7 Nov 2025 09:03:01 +0000 Alice Ryhl <aliceryhl@google.com> wrote: >> That's my point (and probably also Andreas' point), we already has the >> type `SetOnce` to do this, no need for a `OnceLite` type if not >> necessary, and the fact that it can be zero'd by debugfs doesn't change >> it as I explained above. > > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has > three different states, but OnceLite only needs two. I don't think we > should be reusing SetOnce here. Yes, SetOnce provides three different states and the feature to set a value, which are not necessary for this case. So there are two design options: 1) to add a new minimal implementation that provides only the features required for this case, or 2) to use the exisiting, more complex implementation with richer functionality. Could we make a decision so that we can move forward? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-10 9:21 ` FUJITA Tomonori @ 2025-11-10 16:14 ` Boqun Feng 2025-11-10 16:37 ` Miguel Ojeda 2025-11-11 3:09 ` FUJITA Tomonori 0 siblings, 2 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-10 16:14 UTC (permalink / raw) To: FUJITA Tomonori Cc: aliceryhl, a.hindborg, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: > On Fri, 7 Nov 2025 09:03:01 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: > > >> That's my point (and probably also Andreas' point), we already has the > >> type `SetOnce` to do this, no need for a `OnceLite` type if not > >> necessary, and the fact that it can be zero'd by debugfs doesn't change > >> it as I explained above. > > > > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has > > three different states, but OnceLite only needs two. I don't think we > > should be reusing SetOnce here. I mean 3 states should cover 2 states, right? In this case we only need to support SetOnce<()>, so I think it's fine, see below: > > Yes, SetOnce provides three different states and the feature to set a > value, which are not necessary for this case. > > So there are two design options: 1) to add a new minimal > implementation that provides only the features required for this case, Just to be clear 1) will still need to be in kernel::sync module, as per our previous discussion. > or 2) to use the exisiting, more complex implementation with richer > functionality. > or we can just add it into `SetOnce<()>`, for exammple: impl SetOnce<()> { pub fn call_once<F: FnOnce()>(&self, f: F) -> bool { // ORDERING: Relaxed is fine because we don't expect // synchronization here. let old = self.init.xchg(1, Relaxed); // We are the first one here. if old == 0 { f(); return true; } return false; } } Thoughts? Regards, Boqun > Could we make a decision so that we can move forward? > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-10 16:14 ` Boqun Feng @ 2025-11-10 16:37 ` Miguel Ojeda 2025-11-10 16:55 ` Boqun Feng 2025-11-11 3:09 ` FUJITA Tomonori 1 sibling, 1 reply; 52+ messages in thread From: Miguel Ojeda @ 2025-11-10 16:37 UTC (permalink / raw) To: Boqun Feng Cc: FUJITA Tomonori, aliceryhl, a.hindborg, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, Nov 10, 2025 at 5:15 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > // ORDERING: Relaxed is fine because we don't expect > // synchronization here. > let old = self.init.xchg(1, Relaxed); Meta-comment: I like these `// ORDERING: ...` comments -- I will add them to the other ones we are getting implemented in Clippy (`// PANIC: ...`, `// CAST: ...`), i.e. we can request them when calling certain methods. Well, unless you think it is a bad idea for some reason -- I guess in some cases they may be obvious, but just like `unsafe`, even "obvious" ones may not be as obvious as one may initially think... :) Cheers, Miguel ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-10 16:37 ` Miguel Ojeda @ 2025-11-10 16:55 ` Boqun Feng 2025-11-11 21:42 ` Miguel Ojeda 0 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-10 16:55 UTC (permalink / raw) To: Miguel Ojeda Cc: FUJITA Tomonori, aliceryhl, a.hindborg, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, Nov 10, 2025 at 05:37:17PM +0100, Miguel Ojeda wrote: > On Mon, Nov 10, 2025 at 5:15 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > // ORDERING: Relaxed is fine because we don't expect > > // synchronization here. > > let old = self.init.xchg(1, Relaxed); > > Meta-comment: I like these `// ORDERING: ...` comments -- I will add > them to the other ones we are getting implemented in Clippy (`// > PANIC: ...`, `// CAST: ...`), i.e. we can request them when calling > certain methods. > That's very good. We do have a soft(?) rule about commenting the usage of memory barriers in kernel. So enforcing "ORDERING" comments seems good to me. Regards, Boqun > Well, unless you think it is a bad idea for some reason -- I guess in > some cases they may be obvious, but just like `unsafe`, even "obvious" > ones may not be as obvious as one may initially think... :) > > Cheers, > Miguel ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-10 16:55 ` Boqun Feng @ 2025-11-11 21:42 ` Miguel Ojeda 0 siblings, 0 replies; 52+ messages in thread From: Miguel Ojeda @ 2025-11-11 21:42 UTC (permalink / raw) To: Boqun Feng Cc: FUJITA Tomonori, aliceryhl, a.hindborg, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, Nov 10, 2025 at 5:55 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > That's very good. > > We do have a soft(?) rule about commenting the usage of memory barriers > in kernel. So enforcing "ORDERING" comments seems good to me. Thanks! Done -- filled and linked these: https://github.com/rust-lang/rust-clippy/issues/16073 https://github.com/rust-lang/rust-clippy/issues/16074 Cheers, Miguel ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-10 16:14 ` Boqun Feng 2025-11-10 16:37 ` Miguel Ojeda @ 2025-11-11 3:09 ` FUJITA Tomonori 2025-11-11 5:17 ` Boqun Feng 1 sibling, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-11 3:09 UTC (permalink / raw) To: boqun.feng, aliceryhl, a.hindborg Cc: fujita.tomonori, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, 10 Nov 2025 08:14:56 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: >> On Fri, 7 Nov 2025 09:03:01 +0000 >> Alice Ryhl <aliceryhl@google.com> wrote: >> >> >> That's my point (and probably also Andreas' point), we already has the >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change >> >> it as I explained above. >> > >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has >> > three different states, but OnceLite only needs two. I don't think we >> > should be reusing SetOnce here. > > I mean 3 states should cover 2 states, right? In this case we only need > to support SetOnce<()>, so I think it's fine, see below: Yeah, that would remove access to the value member, but I think that init member could still be accessed in an unintended way. >> Yes, SetOnce provides three different states and the feature to set a >> value, which are not necessary for this case. >> >> So there are two design options: 1) to add a new minimal >> implementation that provides only the features required for this case, > > Just to be clear 1) will still need to be in kernel::sync module, as per > our previous discussion. As I wrote before, I think that once_lite.rs could be put in kernel::sync but I'm not sure it's a good idea because I'm not sure if there are any other users who actually need this feature. Any thoughts from others? >> or 2) to use the exisiting, more complex implementation with richer >> functionality. >> > > or we can just add it into `SetOnce<()>`, for exammple: > > impl SetOnce<()> { > pub fn call_once<F: FnOnce()>(&self, f: F) -> bool { > // ORDERING: Relaxed is fine because we don't expect > // synchronization here. > let old = self.init.xchg(1, Relaxed); > > // We are the first one here. > if old == 0 { > f(); > return true; > } > > return false; > } > } > > Thoughts? Why not using populate() method instead of accesing the init member directly by xchg()? Anyway, I tried and got a following compile error: error[E0277]: `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely --> /home/fujita/git/linux-rust/samples/rust/rust_configfs.rs:109:9 | 109 | kernel::pr_info_once!("print once\n"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely We need Sync for SetOnce? I haven't been following the SetOnce discussion, but I wonder if there are use cases that require Sync. If all access goes through its methods, it seems safe to add Sync. I personally prefer the OnceLite implementation of not using SetOnce, however since it's not directly used by users and can be easily changed later, so I'm fine with any implementation as long as we can reach an agreement soon. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-11 3:09 ` FUJITA Tomonori @ 2025-11-11 5:17 ` Boqun Feng 2025-11-11 9:12 ` Andreas Hindborg 2025-11-11 21:43 ` FUJITA Tomonori 0 siblings, 2 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-11 5:17 UTC (permalink / raw) To: FUJITA Tomonori Cc: aliceryhl, a.hindborg, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: > On Mon, 10 Nov 2025 08:14:56 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: > >> On Fri, 7 Nov 2025 09:03:01 +0000 > >> Alice Ryhl <aliceryhl@google.com> wrote: > >> > >> >> That's my point (and probably also Andreas' point), we already has the > >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not > >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change > >> >> it as I explained above. > >> > > >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has > >> > three different states, but OnceLite only needs two. I don't think we > >> > should be reusing SetOnce here. > > > > I mean 3 states should cover 2 states, right? In this case we only need > > to support SetOnce<()>, so I think it's fine, see below: > > Yeah, that would remove access to the value member, but I think that > init member could still be accessed in an unintended way. > What an unintended way you mean? Do you have an example? > > >> Yes, SetOnce provides three different states and the feature to set a > >> value, which are not necessary for this case. > >> > >> So there are two design options: 1) to add a new minimal > >> implementation that provides only the features required for this case, > > > > Just to be clear 1) will still need to be in kernel::sync module, as per > > our previous discussion. > > As I wrote before, I think that once_lite.rs could be put in > kernel::sync but I'm not sure it's a good idea because I'm not sure if > there are any other users who actually need this feature. > Rust std has its own Once[1] module, so I think the concept of "doing something once" is going to be more general in Rust code. Moreover, I don't see any downside, right now you are putting it in a random place, and as we discussed, there is no synchronization problem on the implementation, so putting it in kernel::sync is not worse than a random place? > Any thoughts from others? > > > >> or 2) to use the exisiting, more complex implementation with richer > >> functionality. > >> > > > > or we can just add it into `SetOnce<()>`, for exammple: > > > > impl SetOnce<()> { > > pub fn call_once<F: FnOnce()>(&self, f: F) -> bool { > > // ORDERING: Relaxed is fine because we don't expect > > // synchronization here. > > let old = self.init.xchg(1, Relaxed); > > > > // We are the first one here. > > if old == 0 { > > f(); > > return true; > > } > > > > return false; > > } > > } > > > > Thoughts? > > Why not using populate() method instead of accesing the init member > directly by xchg()? > To follow the exact behavior in your patchset, but either way it's fine. > Anyway, I tried and got a following compile error: > > error[E0277]: `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely > --> /home/fujita/git/linux-rust/samples/rust/rust_configfs.rs:109:9 > | > 109 | kernel::pr_info_once!("print once\n"); > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely > > > We need Sync for SetOnce? I haven't been following the SetOnce Yes, I think `SetOnce<T>` should be `Sync` if T is `Sync`. So good finding! I would say it's actually the effort of trying to reuse the existing solution makes that we find places that needs to be improved. @Andreas, thoughts? > discussion, but I wonder if there are use cases that require Sync. If > all access goes through its methods, it seems safe to add Sync. > > > I personally prefer the OnceLite implementation of not using SetOnce, > however since it's not directly used by users and can be easily > changed later, so I'm fine with any implementation as long as we can > reach an agreement soon. > We should always try to find a unified solution, if there is not much trouble. Otherwise, we will end up having MyOnce, YourOnce, HisOnce, HerOnce, TheirOnce etc in our code base. [1]: https://doc.rust-lang.org/std/sync/struct.Once.html Regards, Boqun ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-11 5:17 ` Boqun Feng @ 2025-11-11 9:12 ` Andreas Hindborg 2025-11-11 23:38 ` FUJITA Tomonori 2025-11-11 21:43 ` FUJITA Tomonori 1 sibling, 1 reply; 52+ messages in thread From: Andreas Hindborg @ 2025-11-11 9:12 UTC (permalink / raw) To: Boqun Feng, FUJITA Tomonori Cc: aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross Boqun Feng <boqun.feng@gmail.com> writes: > On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: >> On Mon, 10 Nov 2025 08:14:56 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: <cut> >> Anyway, I tried and got a following compile error: >> >> error[E0277]: `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely >> --> /home/fujita/git/linux-rust/samples/rust/rust_configfs.rs:109:9 >> | >> 109 | kernel::pr_info_once!("print once\n"); >> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely >> >> >> We need Sync for SetOnce? I haven't been following the SetOnce > > Yes, I think `SetOnce<T>` should be `Sync` if T is `Sync`. So good > finding! @Tomonori you can include a patch to add the impl, if you end up using `SetOnce`. > I would say it's actually the effort of trying to reuse the > existing solution makes that we find places that needs to be improved. > > @Andreas, thoughts? Sorry, I did not follow this branch of the thread yet. Please see my comment on the other branch of the thread tree. TLDR, I agree we should reuse if we could, but I am unsure if external reset would violate some rules. I think we should modify `SetOnce` to support this use case if we can. > >> discussion, but I wonder if there are use cases that require Sync. If >> all access goes through its methods, it seems safe to add Sync. >> >> >> I personally prefer the OnceLite implementation of not using SetOnce, >> however since it's not directly used by users and can be easily >> changed later, so I'm fine with any implementation as long as we can >> reach an agreement soon. >> > > We should always try to find a unified solution, if there is not much > trouble. Otherwise, we will end up having MyOnce, YourOnce, HisOnce, > HerOnce, TheirOnce etc in our code base. I agree. Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-11 9:12 ` Andreas Hindborg @ 2025-11-11 23:38 ` FUJITA Tomonori 2025-11-12 9:04 ` Andreas Hindborg 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-11 23:38 UTC (permalink / raw) To: a.hindborg Cc: boqun.feng, fujita.tomonori, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, 11 Nov 2025 10:12:36 +0100 Andreas Hindborg <a.hindborg@kernel.org> wrote: >>> error[E0277]: `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely >>> --> /home/fujita/git/linux-rust/samples/rust/rust_configfs.rs:109:9 >>> | >>> 109 | kernel::pr_info_once!("print once\n"); >>> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely >>> >>> >>> We need Sync for SetOnce? I haven't been following the SetOnce >> >> Yes, I think `SetOnce<T>` should be `Sync` if T is `Sync`. So good >> finding! > > @Tomonori you can include a patch to add the impl, if you end up using > `SetOnce`. I was planning to send a patch for adding Send and Sync anyway, regardless of whether I end up using SetOnce for OnceLite. Another change I'd like to propose is to use an enum, such as State, instead of u32 for the init member, as I did in the OnceLite implementation [1], something like: +#[derive(Clone, Copy, PartialEq, Eq)] +#[repr(i32)] +enum State { + NotStarted = 0, + InProgress = 1, + Completed = 2, +} + +unsafe impl AtomicType for State { + type Repr = i32; +} + I think that this would make the code's intent clearer. What do you think? If you think that change sounds reasonable, I can send a patch. [1] https://lore.kernel.org/rust-for-linux/20251105054731.3194118-2-fujita.tomonori@gmail.com/ ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-11 23:38 ` FUJITA Tomonori @ 2025-11-12 9:04 ` Andreas Hindborg 2025-11-15 13:37 ` FUJITA Tomonori 0 siblings, 1 reply; 52+ messages in thread From: Andreas Hindborg @ 2025-11-12 9:04 UTC (permalink / raw) To: FUJITA Tomonori Cc: boqun.feng, fujita.tomonori, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross FUJITA Tomonori <fujita.tomonori@gmail.com> writes: > On Tue, 11 Nov 2025 10:12:36 +0100 > Andreas Hindborg <a.hindborg@kernel.org> wrote: > >>>> error[E0277]: `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely >>>> --> /home/fujita/git/linux-rust/samples/rust/rust_configfs.rs:109:9 >>>> | >>>> 109 | kernel::pr_info_once!("print once\n"); >>>> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `core::cell::UnsafeCell<core::mem::MaybeUninit<()>>` cannot be shared between threads safely >>>> >>>> >>>> We need Sync for SetOnce? I haven't been following the SetOnce >>> >>> Yes, I think `SetOnce<T>` should be `Sync` if T is `Sync`. So good >>> finding! >> >> @Tomonori you can include a patch to add the impl, if you end up using >> `SetOnce`. > > I was planning to send a patch for adding Send and Sync anyway, > regardless of whether I end up using SetOnce for OnceLite. > > Another change I'd like to propose is to use an enum, such as State, > instead of u32 for the init member, as I did in the OnceLite > implementation [1], something like: > > +#[derive(Clone, Copy, PartialEq, Eq)] > +#[repr(i32)] > +enum State { > + NotStarted = 0, > + InProgress = 1, > + Completed = 2, > +} > + > +unsafe impl AtomicType for State { > + type Repr = i32; > +} > + > > I think that this would make the code's intent clearer. What do you > think? > > If you think that change sounds reasonable, I can send a patch. The reason I did not do this was to avoid the unsafe impl. I don't have any strong opinion for either way though. Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-12 9:04 ` Andreas Hindborg @ 2025-11-15 13:37 ` FUJITA Tomonori 0 siblings, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-15 13:37 UTC (permalink / raw) To: a.hindborg Cc: fujita.tomonori, boqun.feng, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Wed, 12 Nov 2025 10:04:15 +0100 Andreas Hindborg <a.hindborg@kernel.org> wrote: >> Another change I'd like to propose is to use an enum, such as State, >> instead of u32 for the init member, as I did in the OnceLite >> implementation [1], something like: >> >> +#[derive(Clone, Copy, PartialEq, Eq)] >> +#[repr(i32)] >> +enum State { >> + NotStarted = 0, >> + InProgress = 1, >> + Completed = 2, >> +} >> + >> +unsafe impl AtomicType for State { >> + type Repr = i32; >> +} >> + >> >> I think that this would make the code's intent clearer. What do you >> think? >> >> If you think that change sounds reasonable, I can send a patch. > > The reason I did not do this was to avoid the unsafe impl. I don't have > any strong opinion for either way though. Understood, I don't have a strong preference either, so I'll leave it as is. I'll send the other patches after 6.18. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-11 5:17 ` Boqun Feng 2025-11-11 9:12 ` Andreas Hindborg @ 2025-11-11 21:43 ` FUJITA Tomonori 2025-11-12 1:30 ` Boqun Feng 1 sibling, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-11 21:43 UTC (permalink / raw) To: boqun.feng, a.hindborg Cc: fujita.tomonori, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, 10 Nov 2025 21:17:08 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: >> On Mon, 10 Nov 2025 08:14:56 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >> > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: >> >> On Fri, 7 Nov 2025 09:03:01 +0000 >> >> Alice Ryhl <aliceryhl@google.com> wrote: >> >> >> >> >> That's my point (and probably also Andreas' point), we already has the >> >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not >> >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change >> >> >> it as I explained above. >> >> > >> >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has >> >> > three different states, but OnceLite only needs two. I don't think we >> >> > should be reusing SetOnce here. >> > >> > I mean 3 states should cover 2 states, right? In this case we only need >> > to support SetOnce<()>, so I think it's fine, see below: >> >> Yeah, that would remove access to the value member, but I think that >> init member could still be accessed in an unintended way. >> > > What an unintended way you mean? Do you have an example? From my understanding, the init member of SetOnce is designed to be accessed by using atomic operations, only through its methods. If we use SetOnce for OnceLite, however, the init member would be written using non-atomic operations, which is what I referred to as the "unintended way" since I don't believe Andreas intended it to be modified in that manner; i.e., not through its methods or by non-atomic operations. As I wrote before, in this particular case, I think that it would still work even if both atomic and non-atomic operations access the same variable though. >> >> Yes, SetOnce provides three different states and the feature to set a >> >> value, which are not necessary for this case. >> >> >> >> So there are two design options: 1) to add a new minimal >> >> implementation that provides only the features required for this case, >> > >> > Just to be clear 1) will still need to be in kernel::sync module, as per >> > our previous discussion. >> >> As I wrote before, I think that once_lite.rs could be put in >> kernel::sync but I'm not sure it's a good idea because I'm not sure if >> there are any other users who actually need this feature. >> > > Rust std has its own Once[1] module, so I think the concept of "doing > something once" is going to be more general in Rust code. That was one of the points of discussion a few months ago, implementing an equivalent of std's Once for OnceLite would be over-engineering. To clarify the behavior, it might be better for OnceLite to be implemented without atomic operations, similar to the C version? Unlike Once, as the name suggests, OnceLite is a best-effort mechanism that only tries to run once. >> discussion, but I wonder if there are use cases that require Sync. If >> all access goes through its methods, it seems safe to add Sync. >> >> >> I personally prefer the OnceLite implementation of not using SetOnce, >> however since it's not directly used by users and can be easily >> changed later, so I'm fine with any implementation as long as we can >> reach an agreement soon. >> > > We should always try to find a unified solution, if there is not much > trouble. Otherwise, we will end up having MyOnce, YourOnce, HisOnce, > HerOnce, TheirOnce etc in our code base. I agree that we should always try to use a unified solution whatever feasible. That said, implementing OnceLite would require modifications to SetOnce, and I'm concerned that such modifications would just make SetOnce complicated, which serves no purpose for other use cases. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-11 21:43 ` FUJITA Tomonori @ 2025-11-12 1:30 ` Boqun Feng 2025-11-12 2:23 ` Boqun Feng ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-12 1:30 UTC (permalink / raw) To: FUJITA Tomonori Cc: a.hindborg, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Wed, Nov 12, 2025 at 06:43:49AM +0900, FUJITA Tomonori wrote: > On Mon, 10 Nov 2025 21:17:08 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: > >> On Mon, 10 Nov 2025 08:14:56 -0800 > >> Boqun Feng <boqun.feng@gmail.com> wrote: > >> > >> > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: > >> >> On Fri, 7 Nov 2025 09:03:01 +0000 > >> >> Alice Ryhl <aliceryhl@google.com> wrote: > >> >> > >> >> >> That's my point (and probably also Andreas' point), we already has the > >> >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not > >> >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change > >> >> >> it as I explained above. > >> >> > > >> >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has > >> >> > three different states, but OnceLite only needs two. I don't think we > >> >> > should be reusing SetOnce here. > >> > > >> > I mean 3 states should cover 2 states, right? In this case we only need > >> > to support SetOnce<()>, so I think it's fine, see below: > >> > >> Yeah, that would remove access to the value member, but I think that > >> init member could still be accessed in an unintended way. > >> > > > > What an unintended way you mean? Do you have an example? > > From my understanding, the init member of SetOnce is designed to be > accessed by using atomic operations, only through its methods. If we > use SetOnce for OnceLite, however, the init member would be written > using non-atomic operations, which is what I referred to as the > "unintended way" since I don't believe Andreas intended it to be > modified in that manner; i.e., not through its methods or by > non-atomic operations. > Ok, the "non-atomic operations" seems to be a distraction for me to see the real problem ;-) Let's clear things out a bit. First of all, data races are not allowed in kernel, but kernel has special rules about data races, namely, a few operation should be treated as "per-byte atomics" so that they will have defined behaviors. This is kinda a gray area, but it's what it is. Now for Rust, we shouldn't do what C code did previously that so atomics have to be used in OnceLite, but when interacting with C, we can still use the special rule of data races in kernel for reasoning, so the "non-atomic operations" part in your argument shouldn't affect the validity of OnceLite or SetOnce. What I was missing, and I think you were pointing it correctly is that SetOnce's internal logic doesn't handle a concurrent write to zero, even if no "non-atomic operations" issue, this still breaks the synchronization of SetOnce. It's like adding a invalidate() function for SetOnce, that could invalidate the SetOnce at any time. So yes, SetOnce may not be a good fit for OnceLite, but still OnceLite should be in kernel::sync. Hope I make it clear. Thanks! Regards, Boqun > As I wrote before, in this particular case, I think that it would > still work even if both atomic and non-atomic operations access the > same variable though. > > [..] ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-12 1:30 ` Boqun Feng @ 2025-11-12 2:23 ` Boqun Feng 2025-11-12 9:10 ` Andreas Hindborg 2025-11-12 13:17 ` FUJITA Tomonori 2 siblings, 0 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-12 2:23 UTC (permalink / raw) To: FUJITA Tomonori Cc: a.hindborg, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, Nov 11, 2025 at 05:30:29PM -0800, Boqun Feng wrote: > On Wed, Nov 12, 2025 at 06:43:49AM +0900, FUJITA Tomonori wrote: > > On Mon, 10 Nov 2025 21:17:08 -0800 > > Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: > > >> On Mon, 10 Nov 2025 08:14:56 -0800 > > >> Boqun Feng <boqun.feng@gmail.com> wrote: > > >> > > >> > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: > > >> >> On Fri, 7 Nov 2025 09:03:01 +0000 > > >> >> Alice Ryhl <aliceryhl@google.com> wrote: > > >> >> > > >> >> >> That's my point (and probably also Andreas' point), we already has the > > >> >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not > > >> >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change > > >> >> >> it as I explained above. > > >> >> > > > >> >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has > > >> >> > three different states, but OnceLite only needs two. I don't think we > > >> >> > should be reusing SetOnce here. > > >> > > > >> > I mean 3 states should cover 2 states, right? In this case we only need > > >> > to support SetOnce<()>, so I think it's fine, see below: > > >> > > >> Yeah, that would remove access to the value member, but I think that > > >> init member could still be accessed in an unintended way. > > >> > > > > > > What an unintended way you mean? Do you have an example? > > > > From my understanding, the init member of SetOnce is designed to be > > accessed by using atomic operations, only through its methods. If we > > use SetOnce for OnceLite, however, the init member would be written > > using non-atomic operations, which is what I referred to as the > > "unintended way" since I don't believe Andreas intended it to be > > modified in that manner; i.e., not through its methods or by > > non-atomic operations. > > > > Ok, the "non-atomic operations" seems to be a distraction for me to see > the real problem ;-) Let's clear things out a bit. > > First of all, data races are not allowed in kernel, but kernel has > special rules about data races, namely, a few operation should be > treated as "per-byte atomics" so that they will have defined behaviors. > This is kinda a gray area, but it's what it is. > > Now for Rust, we shouldn't do what C code did previously that so atomics > have to be used in OnceLite, but when interacting with C, we can still > use the special rule of data races in kernel for reasoning, so the > "non-atomic operations" part in your argument shouldn't affect the > validity of OnceLite or SetOnce. > Btw, the "non-atomic operations" can be simply "fixed" by the following diff (only compile tested and I haven't done the "move to kernel::sync" part). Regards, Boqun --------------------->8 diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 8a9a2e732a65..61dc35e49a63 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -361,6 +361,9 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG) __start_once = .; \ *(.data..once) \ __end_once = .; \ + __rstart_once = .; \ + *(.data..ronce) \ + __rend_once = .; \ *(.data..do_once) \ STRUCT_ALIGN(); \ *(__tracepoints) \ diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs index 44ad5dbdc67e..b94834d2fd29 100644 --- a/rust/kernel/once_lite.rs +++ b/rust/kernel/once_lite.rs @@ -48,8 +48,32 @@ pub fn call_once<F>(&self, f: F) -> bool f(); true } + + /// Reset so that `call_once()` can call the function again. + pub fn reset(&self) { + self.0.store(State::Incomplete, Relaxed); + } } +/// # Safety +/// +/// This can be only called with the current `start` and `end` range for `pr_*_once!()`. +#[no_mangle] +pub unsafe extern "C" fn reset_rust_call_once(start: *mut ffi::c_void, end: *mut ffi::c_void) +{ + let start = start.cast::<OnceLite>(); + let end = end.cast::<OnceLite>(); + // SAFETY: Function safety requirement + let offset = unsafe { end.offset_from_unsigned(start) }; + + if offset != 0 { + // SAFETY: Function safety requirement + let onces = unsafe { core::slice::from_raw_parts(start, offset) }; + for once in onces { + once.reset(); + } + } +} /// Run the given function exactly once. /// /// This is equivalent to the kernel's `DO_ONCE_LITE` macro. @@ -64,7 +88,7 @@ pub fn call_once<F>(&self, f: F) -> bool #[macro_export] macro_rules! do_once_lite { ($e:expr) => {{ - #[link_section = ".data..once"] + #[link_section = ".data..ronce"] static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); ONCE.call_once($e) }}; ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-12 1:30 ` Boqun Feng 2025-11-12 2:23 ` Boqun Feng @ 2025-11-12 9:10 ` Andreas Hindborg 2025-11-14 15:03 ` Boqun Feng 2025-11-12 13:17 ` FUJITA Tomonori 2 siblings, 1 reply; 52+ messages in thread From: Andreas Hindborg @ 2025-11-12 9:10 UTC (permalink / raw) To: Boqun Feng, FUJITA Tomonori Cc: aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross Boqun Feng <boqun.feng@gmail.com> writes: > On Wed, Nov 12, 2025 at 06:43:49AM +0900, FUJITA Tomonori wrote: >> On Mon, 10 Nov 2025 21:17:08 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >> > On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: >> >> On Mon, 10 Nov 2025 08:14:56 -0800 >> >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >> >> >> > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: >> >> >> On Fri, 7 Nov 2025 09:03:01 +0000 >> >> >> Alice Ryhl <aliceryhl@google.com> wrote: >> >> >> >> >> >> >> That's my point (and probably also Andreas' point), we already has the >> >> >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not >> >> >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change >> >> >> >> it as I explained above. >> >> >> > >> >> >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has >> >> >> > three different states, but OnceLite only needs two. I don't think we >> >> >> > should be reusing SetOnce here. >> >> > >> >> > I mean 3 states should cover 2 states, right? In this case we only need >> >> > to support SetOnce<()>, so I think it's fine, see below: >> >> >> >> Yeah, that would remove access to the value member, but I think that >> >> init member could still be accessed in an unintended way. >> >> >> > >> > What an unintended way you mean? Do you have an example? >> >> From my understanding, the init member of SetOnce is designed to be >> accessed by using atomic operations, only through its methods. If we >> use SetOnce for OnceLite, however, the init member would be written >> using non-atomic operations, which is what I referred to as the >> "unintended way" since I don't believe Andreas intended it to be >> modified in that manner; i.e., not through its methods or by >> non-atomic operations. >> > > Ok, the "non-atomic operations" seems to be a distraction for me to see > the real problem ;-) Let's clear things out a bit. > > First of all, data races are not allowed in kernel, but kernel has > special rules about data races, namely, a few operation should be > treated as "per-byte atomics" so that they will have defined behaviors. > This is kinda a gray area, but it's what it is. In general, I would assume that a 32 bit rust atomic read racing with "per-byte atomic" writes to the field is not great. We would risk observing torn writes? We could use a u8 atomic in this case? Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-12 9:10 ` Andreas Hindborg @ 2025-11-14 15:03 ` Boqun Feng 0 siblings, 0 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-14 15:03 UTC (permalink / raw) To: Andreas Hindborg Cc: FUJITA Tomonori, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Wed, Nov 12, 2025 at 10:10:14AM +0100, Andreas Hindborg wrote: > Boqun Feng <boqun.feng@gmail.com> writes: > > > On Wed, Nov 12, 2025 at 06:43:49AM +0900, FUJITA Tomonori wrote: > >> On Mon, 10 Nov 2025 21:17:08 -0800 > >> Boqun Feng <boqun.feng@gmail.com> wrote: > >> > >> > On Tue, Nov 11, 2025 at 12:09:49PM +0900, FUJITA Tomonori wrote: > >> >> On Mon, 10 Nov 2025 08:14:56 -0800 > >> >> Boqun Feng <boqun.feng@gmail.com> wrote: > >> >> > >> >> > On Mon, Nov 10, 2025 at 06:21:50PM +0900, FUJITA Tomonori wrote: > >> >> >> On Fri, 7 Nov 2025 09:03:01 +0000 > >> >> >> Alice Ryhl <aliceryhl@google.com> wrote: > >> >> >> > >> >> >> >> That's my point (and probably also Andreas' point), we already has the > >> >> >> >> type `SetOnce` to do this, no need for a `OnceLite` type if not > >> >> >> >> necessary, and the fact that it can be zero'd by debugfs doesn't change > >> >> >> >> it as I explained above. > >> >> >> > > >> >> >> > The SetOnce type doesn't do the same thing as OnceLite. SetOnce has > >> >> >> > three different states, but OnceLite only needs two. I don't think we > >> >> >> > should be reusing SetOnce here. > >> >> > > >> >> > I mean 3 states should cover 2 states, right? In this case we only need > >> >> > to support SetOnce<()>, so I think it's fine, see below: > >> >> > >> >> Yeah, that would remove access to the value member, but I think that > >> >> init member could still be accessed in an unintended way. > >> >> > >> > > >> > What an unintended way you mean? Do you have an example? > >> > >> From my understanding, the init member of SetOnce is designed to be > >> accessed by using atomic operations, only through its methods. If we > >> use SetOnce for OnceLite, however, the init member would be written > >> using non-atomic operations, which is what I referred to as the > >> "unintended way" since I don't believe Andreas intended it to be > >> modified in that manner; i.e., not through its methods or by > >> non-atomic operations. > >> > > > > Ok, the "non-atomic operations" seems to be a distraction for me to see > > the real problem ;-) Let's clear things out a bit. > > > > First of all, data races are not allowed in kernel, but kernel has > > special rules about data races, namely, a few operation should be > > treated as "per-byte atomics" so that they will have defined behaviors. > > This is kinda a gray area, but it's what it is. > > In general, I would assume that a 32 bit rust atomic read racing with > "per-byte atomic" writes to the field is not great. We would risk > observing torn writes? > Given that OnceLite only has one byte that can be non-zero, it's fine. Also if that's a problem, we should just change the memset() part as I posted [1]. I believe that's a better fix (regardless of using i8 or i32). Regards, Boqun [1]: https://lore.kernel.org/rust-for-linux/aRPvjQJLdzvxLUpr@tardis.local/ > We could use a u8 atomic in this case? > > > Best regards, > Andreas Hindborg > > > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once 2025-11-12 1:30 ` Boqun Feng 2025-11-12 2:23 ` Boqun Feng 2025-11-12 9:10 ` Andreas Hindborg @ 2025-11-12 13:17 ` FUJITA Tomonori 2 siblings, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-12 13:17 UTC (permalink / raw) To: boqun.feng Cc: fujita.tomonori, a.hindborg, aliceryhl, ojeda, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, 11 Nov 2025 17:30:29 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > So yes, SetOnce may not be a good fit for OnceLite, but still OnceLite > should be in kernel::sync. I'm glad we reached consensus. I've just sent v2. ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v1 2/2] rust: Add pr_*_once macros 2025-11-05 5:47 ` [PATCH v1 0/2] Add support for print exactly once FUJITA Tomonori 2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori @ 2025-11-05 5:47 ` FUJITA Tomonori 2025-11-05 10:33 ` Alice Ryhl 2025-11-05 20:59 ` [PATCH v1 0/2] Add support for print exactly once Andreas Hindborg 2025-11-13 10:07 ` Alice Ryhl 3 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-05 5:47 UTC (permalink / raw) To: ojeda Cc: a.hindborg, aliceryhl, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once macros, which print a message only once. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/print.rs | 70 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index 2d743d78d220..77e2208f222e 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -423,3 +423,73 @@ macro_rules! pr_cont ( $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*) ) ); + +/// Prints an emergency-level message (level 0) only once. +/// +/// Equivalent to the kernel's [`pr_emerg_once`] macro. +#[macro_export] +macro_rules! pr_emerg_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_emerg!($($arg)*)) + ) +); + +/// Prints an alert-level message (level 1) only once. +/// +/// Equivalent to the kernel's [`pr_alert_once`] macro. +#[macro_export] +macro_rules! pr_alert_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_alert!($($arg)*)) + ) +); + +/// Prints a critical-level message (level 2) only once. +/// +/// Equivalent to the kernel's [`pr_crit_once`] macro. +#[macro_export] +macro_rules! pr_crit_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_crit!($($arg)*)) + ) +); + +/// Prints an error-level message (level 3) only once. +/// +/// Equivalent to the kernel's [`pr_err_once`] macro. +#[macro_export] +macro_rules! pr_err_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_err!($($arg)*)) + ) +); + +/// Prints a warning-level message (level 4) only once. +/// +/// Equivalent to the kernel's [`pr_warn_once`] macro. +#[macro_export] +macro_rules! pr_warn_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_warn!($($arg)*)) + ) +); + +/// Prints a notice-level message (level 5) only once. +/// +/// Equivalent to the kernel's [`pr_notice_once`] macro. +#[macro_export] +macro_rules! pr_notice_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_notice!($($arg)*)) + ) +); + +/// Prints an info-level message (level 6) only once. +/// +/// Equivalent to the kernel's [`pr_info_once`] macro. +#[macro_export] +macro_rules! pr_info_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!(|| $crate::pr_info!($($arg)*)) + ) +); -- 2.43.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v1 2/2] rust: Add pr_*_once macros 2025-11-05 5:47 ` [PATCH v1 2/2] rust: Add pr_*_once macros FUJITA Tomonori @ 2025-11-05 10:33 ` Alice Ryhl 0 siblings, 0 replies; 52+ messages in thread From: Alice Ryhl @ 2025-11-05 10:33 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Wed, Nov 05, 2025 at 02:47:31PM +0900, FUJITA Tomonori wrote: > Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once > macros, which print a message only once. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> (also applies if macro syntax is changed according to my suggestion) ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-05 5:47 ` [PATCH v1 0/2] Add support for print exactly once FUJITA Tomonori 2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori 2025-11-05 5:47 ` [PATCH v1 2/2] rust: Add pr_*_once macros FUJITA Tomonori @ 2025-11-05 20:59 ` Andreas Hindborg 2025-11-05 23:12 ` FUJITA Tomonori 2025-11-13 10:07 ` Alice Ryhl 3 siblings, 1 reply; 52+ messages in thread From: Andreas Hindborg @ 2025-11-05 20:59 UTC (permalink / raw) To: FUJITA Tomonori, ojeda Cc: aliceryhl, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > pr_*_once macros. > > A proposal for this feature was made in the past [1], but it didn't > reach consensus on the implementation and wasn't merged. After reading > the previous discussions, I implemented it using a different approach. > > In the previous proposal, a structure equivalent to std::sync::Once > was implemented to realize the DO_ONCE_LITE macro. The approach tried > to provide Once-like semantics by using two atomic values. As pointed > out in the previous review comments, I think this approach tries to > provide more functionality than needed, making it unnecessarily > complex. Also, because data structures in the .data..once section can > be cleared at any time (via debugfs clear_warn_once), an > implementation using two atomics wouldn't work correctly. > > Therefore, I decided to drop the idea of emulating Once and took a > minimal approach to implement DO_ONCE_LITE with only one atomic > variable. While it would be possible to implement the feature entirely > as a Rust macro, the functionality that can be implemented as regular > functions has been extracted and implemented as the OnceLite struct > for better code readability. > > Of course, unlike the previous proposal, this uses LKMM atomics. Please consider if it makes sense to base this on `SetOnce`. It is in linux-next now, but was on list here [1]. Best regards, Andreas Hindborg [1] https://lore.kernel.org/r/20250924-module-params-v3-v18-1-bf512c35d910@kernel.org ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-05 20:59 ` [PATCH v1 0/2] Add support for print exactly once Andreas Hindborg @ 2025-11-05 23:12 ` FUJITA Tomonori 2025-11-06 14:31 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-05 23:12 UTC (permalink / raw) To: a.hindborg Cc: fujita.tomonori, ojeda, aliceryhl, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross On Wed, 05 Nov 2025 21:59:06 +0100 Andreas Hindborg <a.hindborg@kernel.org> wrote: > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >> pr_*_once macros. >> >> A proposal for this feature was made in the past [1], but it didn't >> reach consensus on the implementation and wasn't merged. After reading >> the previous discussions, I implemented it using a different approach. >> >> In the previous proposal, a structure equivalent to std::sync::Once >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >> to provide Once-like semantics by using two atomic values. As pointed >> out in the previous review comments, I think this approach tries to >> provide more functionality than needed, making it unnecessarily >> complex. Also, because data structures in the .data..once section can >> be cleared at any time (via debugfs clear_warn_once), an >> implementation using two atomics wouldn't work correctly. >> >> Therefore, I decided to drop the idea of emulating Once and took a >> minimal approach to implement DO_ONCE_LITE with only one atomic >> variable. While it would be possible to implement the feature entirely >> as a Rust macro, the functionality that can be implemented as regular >> functions has been extracted and implemented as the OnceLite struct >> for better code readability. >> >> Of course, unlike the previous proposal, this uses LKMM atomics. > > Please consider if it makes sense to base this on `SetOnce`. It is in > linux-next now, but was on list here [1]. Data placed in the .data..once section can be zero-cleared when a user writes to debugfs clear_warn_once. In that case, would such data still be considered a valid SetOnce value? Even if it technically represents a valid state, using data structures whose zero-cleared state isn't clearly valid, unlike simple integer types, doesn't seem like a good idea. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-05 23:12 ` FUJITA Tomonori @ 2025-11-06 14:31 ` Boqun Feng 2025-11-10 12:16 ` Andreas Hindborg 0 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-06 14:31 UTC (permalink / raw) To: FUJITA Tomonori Cc: a.hindborg, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: > On Wed, 05 Nov 2025 21:59:06 +0100 > Andreas Hindborg <a.hindborg@kernel.org> wrote: > > > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > > > >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > >> pr_*_once macros. > >> > >> A proposal for this feature was made in the past [1], but it didn't > >> reach consensus on the implementation and wasn't merged. After reading > >> the previous discussions, I implemented it using a different approach. > >> > >> In the previous proposal, a structure equivalent to std::sync::Once > >> was implemented to realize the DO_ONCE_LITE macro. The approach tried > >> to provide Once-like semantics by using two atomic values. As pointed > >> out in the previous review comments, I think this approach tries to > >> provide more functionality than needed, making it unnecessarily > >> complex. Also, because data structures in the .data..once section can > >> be cleared at any time (via debugfs clear_warn_once), an > >> implementation using two atomics wouldn't work correctly. > >> > >> Therefore, I decided to drop the idea of emulating Once and took a > >> minimal approach to implement DO_ONCE_LITE with only one atomic > >> variable. While it would be possible to implement the feature entirely > >> as a Rust macro, the functionality that can be implemented as regular > >> functions has been extracted and implemented as the OnceLite struct > >> for better code readability. > >> > >> Of course, unlike the previous proposal, this uses LKMM atomics. > > > > Please consider if it makes sense to base this on `SetOnce`. It is in > > linux-next now, but was on list here [1]. > > Data placed in the .data..once section can be zero-cleared when a user > writes to debugfs clear_warn_once. In that case, would such data still > be considered a valid SetOnce value? > It's still a valid value I believe. In term of data races, Rust and C have no difference, so if writing to debugfs could cause issues in Rust, it would cause issues in C as well. Regards, Boqun > Even if it technically represents a valid state, using data structures > whose zero-cleared state isn't clearly valid, unlike simple integer > types, doesn't seem like a good idea. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-06 14:31 ` Boqun Feng @ 2025-11-10 12:16 ` Andreas Hindborg 2025-11-10 16:08 ` Boqun Feng 2025-11-11 1:28 ` FUJITA Tomonori 0 siblings, 2 replies; 52+ messages in thread From: Andreas Hindborg @ 2025-11-10 12:16 UTC (permalink / raw) To: Boqun Feng, FUJITA Tomonori Cc: ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross Boqun Feng <boqun.feng@gmail.com> writes: > On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: >> On Wed, 05 Nov 2025 21:59:06 +0100 >> Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: >> > >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >> >> pr_*_once macros. >> >> >> >> A proposal for this feature was made in the past [1], but it didn't >> >> reach consensus on the implementation and wasn't merged. After reading >> >> the previous discussions, I implemented it using a different approach. >> >> >> >> In the previous proposal, a structure equivalent to std::sync::Once >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >> >> to provide Once-like semantics by using two atomic values. As pointed >> >> out in the previous review comments, I think this approach tries to >> >> provide more functionality than needed, making it unnecessarily >> >> complex. Also, because data structures in the .data..once section can >> >> be cleared at any time (via debugfs clear_warn_once), an >> >> implementation using two atomics wouldn't work correctly. >> >> >> >> Therefore, I decided to drop the idea of emulating Once and took a >> >> minimal approach to implement DO_ONCE_LITE with only one atomic >> >> variable. While it would be possible to implement the feature entirely >> >> as a Rust macro, the functionality that can be implemented as regular >> >> functions has been extracted and implemented as the OnceLite struct >> >> for better code readability. >> >> >> >> Of course, unlike the previous proposal, this uses LKMM atomics. >> > >> > Please consider if it makes sense to base this on `SetOnce`. It is in >> > linux-next now, but was on list here [1]. >> >> Data placed in the .data..once section can be zero-cleared when a user >> writes to debugfs clear_warn_once. In that case, would such data still >> be considered a valid SetOnce value? >> > > It's still a valid value I believe. In term of data races, Rust and C > have no difference, so if writing to debugfs could cause issues in Rust, > it would cause issues in C as well. @Tomo you are right, `SetOnce` would not work with someone (debugfs) asynchronously modifying the state atomic variable. It requires exclusive access while writing the contained value. Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-10 12:16 ` Andreas Hindborg @ 2025-11-10 16:08 ` Boqun Feng 2025-11-11 9:02 ` Andreas Hindborg 2025-11-11 1:28 ` FUJITA Tomonori 1 sibling, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-10 16:08 UTC (permalink / raw) To: Andreas Hindborg Cc: FUJITA Tomonori, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, Nov 10, 2025 at 01:16:44PM +0100, Andreas Hindborg wrote: > Boqun Feng <boqun.feng@gmail.com> writes: > > > On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: > >> On Wed, 05 Nov 2025 21:59:06 +0100 > >> Andreas Hindborg <a.hindborg@kernel.org> wrote: > >> > >> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > >> > > >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > >> >> pr_*_once macros. > >> >> > >> >> A proposal for this feature was made in the past [1], but it didn't > >> >> reach consensus on the implementation and wasn't merged. After reading > >> >> the previous discussions, I implemented it using a different approach. > >> >> > >> >> In the previous proposal, a structure equivalent to std::sync::Once > >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried > >> >> to provide Once-like semantics by using two atomic values. As pointed > >> >> out in the previous review comments, I think this approach tries to > >> >> provide more functionality than needed, making it unnecessarily > >> >> complex. Also, because data structures in the .data..once section can > >> >> be cleared at any time (via debugfs clear_warn_once), an > >> >> implementation using two atomics wouldn't work correctly. > >> >> > >> >> Therefore, I decided to drop the idea of emulating Once and took a > >> >> minimal approach to implement DO_ONCE_LITE with only one atomic > >> >> variable. While it would be possible to implement the feature entirely > >> >> as a Rust macro, the functionality that can be implemented as regular > >> >> functions has been extracted and implemented as the OnceLite struct > >> >> for better code readability. > >> >> > >> >> Of course, unlike the previous proposal, this uses LKMM atomics. > >> > > >> > Please consider if it makes sense to base this on `SetOnce`. It is in > >> > linux-next now, but was on list here [1]. > >> > >> Data placed in the .data..once section can be zero-cleared when a user > >> writes to debugfs clear_warn_once. In that case, would such data still > >> be considered a valid SetOnce value? > >> > > > > It's still a valid value I believe. In term of data races, Rust and C > > have no difference, so if writing to debugfs could cause issues in Rust, > > it would cause issues in C as well. > > @Tomo you are right, `SetOnce` would not work with someone (debugfs) > asynchronously modifying the state atomic variable. It requires > exclusive access while writing the contained value. > I mean if we were to use `SetOnce` in pr_*_once(), we should just use `SetOnce<()>`, and the problem you mentioned doesn't exist in this case. Regards, Boqun > Best regards, > Andreas Hindborg > > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-10 16:08 ` Boqun Feng @ 2025-11-11 9:02 ` Andreas Hindborg 2025-11-12 0:45 ` FUJITA Tomonori 0 siblings, 1 reply; 52+ messages in thread From: Andreas Hindborg @ 2025-11-11 9:02 UTC (permalink / raw) To: Boqun Feng Cc: FUJITA Tomonori, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross Boqun Feng <boqun.feng@gmail.com> writes: > On Mon, Nov 10, 2025 at 01:16:44PM +0100, Andreas Hindborg wrote: >> Boqun Feng <boqun.feng@gmail.com> writes: >> >> > On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: >> >> On Wed, 05 Nov 2025 21:59:06 +0100 >> >> Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> >> >> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: >> >> > >> >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >> >> >> pr_*_once macros. >> >> >> >> >> >> A proposal for this feature was made in the past [1], but it didn't >> >> >> reach consensus on the implementation and wasn't merged. After reading >> >> >> the previous discussions, I implemented it using a different approach. >> >> >> >> >> >> In the previous proposal, a structure equivalent to std::sync::Once >> >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >> >> >> to provide Once-like semantics by using two atomic values. As pointed >> >> >> out in the previous review comments, I think this approach tries to >> >> >> provide more functionality than needed, making it unnecessarily >> >> >> complex. Also, because data structures in the .data..once section can >> >> >> be cleared at any time (via debugfs clear_warn_once), an >> >> >> implementation using two atomics wouldn't work correctly. >> >> >> >> >> >> Therefore, I decided to drop the idea of emulating Once and took a >> >> >> minimal approach to implement DO_ONCE_LITE with only one atomic >> >> >> variable. While it would be possible to implement the feature entirely >> >> >> as a Rust macro, the functionality that can be implemented as regular >> >> >> functions has been extracted and implemented as the OnceLite struct >> >> >> for better code readability. >> >> >> >> >> >> Of course, unlike the previous proposal, this uses LKMM atomics. >> >> > >> >> > Please consider if it makes sense to base this on `SetOnce`. It is in >> >> > linux-next now, but was on list here [1]. >> >> >> >> Data placed in the .data..once section can be zero-cleared when a user >> >> writes to debugfs clear_warn_once. In that case, would such data still >> >> be considered a valid SetOnce value? >> >> >> > >> > It's still a valid value I believe. In term of data races, Rust and C >> > have no difference, so if writing to debugfs could cause issues in Rust, >> > it would cause issues in C as well. >> >> @Tomo you are right, `SetOnce` would not work with someone (debugfs) >> asynchronously modifying the state atomic variable. It requires >> exclusive access while writing the contained value. >> > > I mean if we were to use `SetOnce` in pr_*_once(), we should just use > `SetOnce<()>`, and the problem you mentioned doesn't exist in this case. At the very least it would break the type invariants and assumptions of the `SetOnce` type. There a race condition between `SetOnce::as_ref` and `SetOnce::populate`. I don't think we are allowed to race like this, even if the type is `()`? At any rate if we do this, we should update the safety invariant of `SetOnce` to state that it is valid to revert the type back to the initial state for zero sized types that do not have a `Drop` implementation. Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-11 9:02 ` Andreas Hindborg @ 2025-11-12 0:45 ` FUJITA Tomonori 2025-11-12 1:04 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-12 0:45 UTC (permalink / raw) To: a.hindborg Cc: boqun.feng, fujita.tomonori, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, 11 Nov 2025 10:02:46 +0100 Andreas Hindborg <a.hindborg@kernel.org> wrote: > Boqun Feng <boqun.feng@gmail.com> writes: > >> On Mon, Nov 10, 2025 at 01:16:44PM +0100, Andreas Hindborg wrote: >>> Boqun Feng <boqun.feng@gmail.com> writes: >>> >>> > On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: >>> >> On Wed, 05 Nov 2025 21:59:06 +0100 >>> >> Andreas Hindborg <a.hindborg@kernel.org> wrote: >>> >> >>> >> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: >>> >> > >>> >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >>> >> >> pr_*_once macros. >>> >> >> >>> >> >> A proposal for this feature was made in the past [1], but it didn't >>> >> >> reach consensus on the implementation and wasn't merged. After reading >>> >> >> the previous discussions, I implemented it using a different approach. >>> >> >> >>> >> >> In the previous proposal, a structure equivalent to std::sync::Once >>> >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >>> >> >> to provide Once-like semantics by using two atomic values. As pointed >>> >> >> out in the previous review comments, I think this approach tries to >>> >> >> provide more functionality than needed, making it unnecessarily >>> >> >> complex. Also, because data structures in the .data..once section can >>> >> >> be cleared at any time (via debugfs clear_warn_once), an >>> >> >> implementation using two atomics wouldn't work correctly. >>> >> >> >>> >> >> Therefore, I decided to drop the idea of emulating Once and took a >>> >> >> minimal approach to implement DO_ONCE_LITE with only one atomic >>> >> >> variable. While it would be possible to implement the feature entirely >>> >> >> as a Rust macro, the functionality that can be implemented as regular >>> >> >> functions has been extracted and implemented as the OnceLite struct >>> >> >> for better code readability. >>> >> >> >>> >> >> Of course, unlike the previous proposal, this uses LKMM atomics. >>> >> > >>> >> > Please consider if it makes sense to base this on `SetOnce`. It is in >>> >> > linux-next now, but was on list here [1]. >>> >> >>> >> Data placed in the .data..once section can be zero-cleared when a user >>> >> writes to debugfs clear_warn_once. In that case, would such data still >>> >> be considered a valid SetOnce value? >>> >> >>> > >>> > It's still a valid value I believe. In term of data races, Rust and C >>> > have no difference, so if writing to debugfs could cause issues in Rust, >>> > it would cause issues in C as well. >>> >>> @Tomo you are right, `SetOnce` would not work with someone (debugfs) >>> asynchronously modifying the state atomic variable. It requires >>> exclusive access while writing the contained value. >>> >> >> I mean if we were to use `SetOnce` in pr_*_once(), we should just use >> `SetOnce<()>`, and the problem you mentioned doesn't exist in this case. > > At the very least it would break the type invariants and assumptions of > the `SetOnce` type. There a race condition between `SetOnce::as_ref` and > `SetOnce::populate`. I don't think we are allowed to race like this, > even if the type is `()`? > > At any rate if we do this, we should update the safety invariant of > `SetOnce` to state that it is valid to revert the type back to the > initial state for zero sized types that do not have a `Drop` > implementation. I agree. I think that even if the type is (), a race condition would still occur if the init member were reset to zero using non-atomic operations. For instance, the as_ref() method effectively checks whether a value has been set, so for the () type, it would return either Ok(()) or None. Although SetOnce is designed to atomically set a value, allowing the data to be modified by non-atomic operations at arbitrary times would, as I mentioned in another thread, unnecessarily complicate its implementation for a feature that would not be useful in other use cases. Could the root cause of the issue be that OnceLite, which isn't updated atomically, is implemented using atomic operations? It might be better to implement it with non-atomic operations, as in the C version, as that would make the update behavior clearer. What do you think? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-12 0:45 ` FUJITA Tomonori @ 2025-11-12 1:04 ` Boqun Feng 2025-11-12 1:18 ` FUJITA Tomonori 0 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-12 1:04 UTC (permalink / raw) To: FUJITA Tomonori Cc: a.hindborg, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Wed, Nov 12, 2025 at 09:45:08AM +0900, FUJITA Tomonori wrote: > On Tue, 11 Nov 2025 10:02:46 +0100 > Andreas Hindborg <a.hindborg@kernel.org> wrote: > > > Boqun Feng <boqun.feng@gmail.com> writes: > > > >> On Mon, Nov 10, 2025 at 01:16:44PM +0100, Andreas Hindborg wrote: > >>> Boqun Feng <boqun.feng@gmail.com> writes: > >>> > >>> > On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: > >>> >> On Wed, 05 Nov 2025 21:59:06 +0100 > >>> >> Andreas Hindborg <a.hindborg@kernel.org> wrote: > >>> >> > >>> >> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > >>> >> > > >>> >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > >>> >> >> pr_*_once macros. > >>> >> >> > >>> >> >> A proposal for this feature was made in the past [1], but it didn't > >>> >> >> reach consensus on the implementation and wasn't merged. After reading > >>> >> >> the previous discussions, I implemented it using a different approach. > >>> >> >> > >>> >> >> In the previous proposal, a structure equivalent to std::sync::Once > >>> >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried > >>> >> >> to provide Once-like semantics by using two atomic values. As pointed > >>> >> >> out in the previous review comments, I think this approach tries to > >>> >> >> provide more functionality than needed, making it unnecessarily > >>> >> >> complex. Also, because data structures in the .data..once section can > >>> >> >> be cleared at any time (via debugfs clear_warn_once), an > >>> >> >> implementation using two atomics wouldn't work correctly. > >>> >> >> > >>> >> >> Therefore, I decided to drop the idea of emulating Once and took a > >>> >> >> minimal approach to implement DO_ONCE_LITE with only one atomic > >>> >> >> variable. While it would be possible to implement the feature entirely > >>> >> >> as a Rust macro, the functionality that can be implemented as regular > >>> >> >> functions has been extracted and implemented as the OnceLite struct > >>> >> >> for better code readability. > >>> >> >> > >>> >> >> Of course, unlike the previous proposal, this uses LKMM atomics. > >>> >> > > >>> >> > Please consider if it makes sense to base this on `SetOnce`. It is in > >>> >> > linux-next now, but was on list here [1]. > >>> >> > >>> >> Data placed in the .data..once section can be zero-cleared when a user > >>> >> writes to debugfs clear_warn_once. In that case, would such data still > >>> >> be considered a valid SetOnce value? > >>> >> > >>> > > >>> > It's still a valid value I believe. In term of data races, Rust and C > >>> > have no difference, so if writing to debugfs could cause issues in Rust, > >>> > it would cause issues in C as well. > >>> > >>> @Tomo you are right, `SetOnce` would not work with someone (debugfs) > >>> asynchronously modifying the state atomic variable. It requires > >>> exclusive access while writing the contained value. > >>> > >> > >> I mean if we were to use `SetOnce` in pr_*_once(), we should just use > >> `SetOnce<()>`, and the problem you mentioned doesn't exist in this case. > > > > At the very least it would break the type invariants and assumptions of > > the `SetOnce` type. There a race condition between `SetOnce::as_ref` and > > `SetOnce::populate`. I don't think we are allowed to race like this, > > even if the type is `()`? > > > > At any rate if we do this, we should update the safety invariant of > > `SetOnce` to state that it is valid to revert the type back to the > > initial state for zero sized types that do not have a `Drop` > > implementation. > > I agree. I think that even if the type is (), a race condition would > still occur if the init member were reset to zero using non-atomic > operations. For instance, the as_ref() method effectively checks I already explained this, the debugfs writes have to be treated as per-byte atomic, otherwise it's data race, using "non-atomic" to reference that won't be accurate. > whether a value has been set, so for the () type, it would return > either Ok(()) or None. > > Although SetOnce is designed to atomically set a value, allowing the > data to be modified by non-atomic operations at arbitrary times would, > as I mentioned in another thread, unnecessarily complicate its > implementation for a feature that would not be useful in other use > cases. > > Could the root cause of the issue be that OnceLite, which isn't > updated atomically, is implemented using atomic operations? > > It might be better to implement it with non-atomic operations, as in > the C version, as that would make the update behavior clearer. > No, that's introducing more problems and inheriting bad things from C. I would say we can probably implement an OnceFlag type (similiar to OnceLite but also support cmpxchg so that SetOnce::populate() can use) and use it to implement SetOnce. Then you can avoid violating SetOnce's type invariants that Andreas mentioned. Regards, Boqun > What do you think? > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-12 1:04 ` Boqun Feng @ 2025-11-12 1:18 ` FUJITA Tomonori 2025-11-12 1:35 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-12 1:18 UTC (permalink / raw) To: boqun.feng Cc: fujita.tomonori, a.hindborg, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, 11 Nov 2025 17:04:26 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Wed, Nov 12, 2025 at 09:45:08AM +0900, FUJITA Tomonori wrote: >> On Tue, 11 Nov 2025 10:02:46 +0100 >> Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> > Boqun Feng <boqun.feng@gmail.com> writes: >> > >> >> On Mon, Nov 10, 2025 at 01:16:44PM +0100, Andreas Hindborg wrote: >> >>> Boqun Feng <boqun.feng@gmail.com> writes: >> >>> >> >>> > On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: >> >>> >> On Wed, 05 Nov 2025 21:59:06 +0100 >> >>> >> Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >>> >> >> >>> >> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: >> >>> >> > >> >>> >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >> >>> >> >> pr_*_once macros. >> >>> >> >> >> >>> >> >> A proposal for this feature was made in the past [1], but it didn't >> >>> >> >> reach consensus on the implementation and wasn't merged. After reading >> >>> >> >> the previous discussions, I implemented it using a different approach. >> >>> >> >> >> >>> >> >> In the previous proposal, a structure equivalent to std::sync::Once >> >>> >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >> >>> >> >> to provide Once-like semantics by using two atomic values. As pointed >> >>> >> >> out in the previous review comments, I think this approach tries to >> >>> >> >> provide more functionality than needed, making it unnecessarily >> >>> >> >> complex. Also, because data structures in the .data..once section can >> >>> >> >> be cleared at any time (via debugfs clear_warn_once), an >> >>> >> >> implementation using two atomics wouldn't work correctly. >> >>> >> >> >> >>> >> >> Therefore, I decided to drop the idea of emulating Once and took a >> >>> >> >> minimal approach to implement DO_ONCE_LITE with only one atomic >> >>> >> >> variable. While it would be possible to implement the feature entirely >> >>> >> >> as a Rust macro, the functionality that can be implemented as regular >> >>> >> >> functions has been extracted and implemented as the OnceLite struct >> >>> >> >> for better code readability. >> >>> >> >> >> >>> >> >> Of course, unlike the previous proposal, this uses LKMM atomics. >> >>> >> > >> >>> >> > Please consider if it makes sense to base this on `SetOnce`. It is in >> >>> >> > linux-next now, but was on list here [1]. >> >>> >> >> >>> >> Data placed in the .data..once section can be zero-cleared when a user >> >>> >> writes to debugfs clear_warn_once. In that case, would such data still >> >>> >> be considered a valid SetOnce value? >> >>> >> >> >>> > >> >>> > It's still a valid value I believe. In term of data races, Rust and C >> >>> > have no difference, so if writing to debugfs could cause issues in Rust, >> >>> > it would cause issues in C as well. >> >>> >> >>> @Tomo you are right, `SetOnce` would not work with someone (debugfs) >> >>> asynchronously modifying the state atomic variable. It requires >> >>> exclusive access while writing the contained value. >> >>> >> >> >> >> I mean if we were to use `SetOnce` in pr_*_once(), we should just use >> >> `SetOnce<()>`, and the problem you mentioned doesn't exist in this case. >> > >> > At the very least it would break the type invariants and assumptions of >> > the `SetOnce` type. There a race condition between `SetOnce::as_ref` and >> > `SetOnce::populate`. I don't think we are allowed to race like this, >> > even if the type is `()`? >> > >> > At any rate if we do this, we should update the safety invariant of >> > `SetOnce` to state that it is valid to revert the type back to the >> > initial state for zero sized types that do not have a `Drop` >> > implementation. >> >> I agree. I think that even if the type is (), a race condition would >> still occur if the init member were reset to zero using non-atomic >> operations. For instance, the as_ref() method effectively checks > > I already explained this, the debugfs writes have to be treated as > per-byte atomic, otherwise it's data race, using "non-atomic" to > reference that won't be accurate. Suppose the init member of SetOnce is set to 2 and then reset to 0 via debugfs. In that case, if two threads execute the as_ref() method at the same time, is it guaranteed that they will both see the same value? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-12 1:18 ` FUJITA Tomonori @ 2025-11-12 1:35 ` Boqun Feng 2025-11-13 9:55 ` FUJITA Tomonori 0 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-12 1:35 UTC (permalink / raw) To: FUJITA Tomonori Cc: a.hindborg, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Wed, Nov 12, 2025 at 10:18:53AM +0900, FUJITA Tomonori wrote: [..] > >> > >> I agree. I think that even if the type is (), a race condition would > >> still occur if the init member were reset to zero using non-atomic > >> operations. For instance, the as_ref() method effectively checks > > > > I already explained this, the debugfs writes have to be treated as > > per-byte atomic, otherwise it's data race, using "non-atomic" to > > reference that won't be accurate. > > Suppose the init member of SetOnce is set to 2 and then reset to 0 via > debugfs. In that case, if two threads execute the as_ref() method at > the same time, is it guaranteed that they will both see the same > value? No, but it's not because it's non-atomic, but because there is no synchronization in this case. It'll still be problem even if the reset is atomic. (I was distracted by the mentioning of "non-atomic" and didn't see the real idea here...) Regards, Boqun ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-12 1:35 ` Boqun Feng @ 2025-11-13 9:55 ` FUJITA Tomonori 0 siblings, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-13 9:55 UTC (permalink / raw) To: boqun.feng Cc: fujita.tomonori, a.hindborg, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Tue, 11 Nov 2025 17:35:04 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Wed, Nov 12, 2025 at 10:18:53AM +0900, FUJITA Tomonori wrote: > [..] >> >> >> >> I agree. I think that even if the type is (), a race condition would >> >> still occur if the init member were reset to zero using non-atomic >> >> operations. For instance, the as_ref() method effectively checks >> > >> > I already explained this, the debugfs writes have to be treated as >> > per-byte atomic, otherwise it's data race, using "non-atomic" to >> > reference that won't be accurate. >> >> Suppose the init member of SetOnce is set to 2 and then reset to 0 via >> debugfs. In that case, if two threads execute the as_ref() method at >> the same time, is it guaranteed that they will both see the same >> value? > > No, but it's not because it's non-atomic, but because there is no > synchronization in this case. It'll still be problem even if the reset > is atomic. Of source, I was using the term "atomic operations" in the way it's used in the kernel docs (for example, in atomic_t.txt), namely to refer to the set of functions that ultimately issue CPU atomic instructions. Naturally, those perform the appropriate synchronization. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-10 12:16 ` Andreas Hindborg 2025-11-10 16:08 ` Boqun Feng @ 2025-11-11 1:28 ` FUJITA Tomonori 1 sibling, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-11 1:28 UTC (permalink / raw) To: a.hindborg Cc: boqun.feng, fujita.tomonori, ojeda, aliceryhl, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross On Mon, 10 Nov 2025 13:16:44 +0100 Andreas Hindborg <a.hindborg@kernel.org> wrote: > Boqun Feng <boqun.feng@gmail.com> writes: > >> On Thu, Nov 06, 2025 at 08:12:31AM +0900, FUJITA Tomonori wrote: >>> On Wed, 05 Nov 2025 21:59:06 +0100 >>> Andreas Hindborg <a.hindborg@kernel.org> wrote: >>> >>> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: >>> > >>> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >>> >> pr_*_once macros. >>> >> >>> >> A proposal for this feature was made in the past [1], but it didn't >>> >> reach consensus on the implementation and wasn't merged. After reading >>> >> the previous discussions, I implemented it using a different approach. >>> >> >>> >> In the previous proposal, a structure equivalent to std::sync::Once >>> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >>> >> to provide Once-like semantics by using two atomic values. As pointed >>> >> out in the previous review comments, I think this approach tries to >>> >> provide more functionality than needed, making it unnecessarily >>> >> complex. Also, because data structures in the .data..once section can >>> >> be cleared at any time (via debugfs clear_warn_once), an >>> >> implementation using two atomics wouldn't work correctly. >>> >> >>> >> Therefore, I decided to drop the idea of emulating Once and took a >>> >> minimal approach to implement DO_ONCE_LITE with only one atomic >>> >> variable. While it would be possible to implement the feature entirely >>> >> as a Rust macro, the functionality that can be implemented as regular >>> >> functions has been extracted and implemented as the OnceLite struct >>> >> for better code readability. >>> >> >>> >> Of course, unlike the previous proposal, this uses LKMM atomics. >>> > >>> > Please consider if it makes sense to base this on `SetOnce`. It is in >>> > linux-next now, but was on list here [1]. >>> >>> Data placed in the .data..once section can be zero-cleared when a user >>> writes to debugfs clear_warn_once. In that case, would such data still >>> be considered a valid SetOnce value? >>> >> >> It's still a valid value I believe. In term of data races, Rust and C >> have no difference, so if writing to debugfs could cause issues in Rust, >> it would cause issues in C as well. > > @Tomo you are right, `SetOnce` would not work with someone (debugfs) > asynchronously modifying the state atomic variable. It requires > exclusive access while writing the contained value. Yeah, I believe that SetOnce assumes that both init and value fields are modified only through its methods, and it's not intended to handle external modifications such those done with memset. For this use-case, SetOnce would still work even if used that way, though it's not the intended or recommended usage, I think. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-05 5:47 ` [PATCH v1 0/2] Add support for print exactly once FUJITA Tomonori ` (2 preceding siblings ...) 2025-11-05 20:59 ` [PATCH v1 0/2] Add support for print exactly once Andreas Hindborg @ 2025-11-13 10:07 ` Alice Ryhl 2025-11-13 11:18 ` FUJITA Tomonori 3 siblings, 1 reply; 52+ messages in thread From: Alice Ryhl @ 2025-11-13 10:07 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Wed, Nov 05, 2025 at 02:47:29PM +0900, FUJITA Tomonori wrote: > This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > pr_*_once macros. > > A proposal for this feature was made in the past [1], but it didn't > reach consensus on the implementation and wasn't merged. After reading > the previous discussions, I implemented it using a different approach. > > In the previous proposal, a structure equivalent to std::sync::Once > was implemented to realize the DO_ONCE_LITE macro. The approach tried > to provide Once-like semantics by using two atomic values. As pointed > out in the previous review comments, I think this approach tries to > provide more functionality than needed, making it unnecessarily > complex. Also, because data structures in the .data..once section can > be cleared at any time (via debugfs clear_warn_once), an > implementation using two atomics wouldn't work correctly. > > Therefore, I decided to drop the idea of emulating Once and took a > minimal approach to implement DO_ONCE_LITE with only one atomic > variable. While it would be possible to implement the feature entirely > as a Rust macro, the functionality that can be implemented as regular > functions has been extracted and implemented as the OnceLite struct > for better code readability. > > Of course, unlike the previous proposal, this uses LKMM atomics. > > [1] https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/ There has been a fair bit of discussion below. Here is my take on the way forward: 1. OnceLite should be it's own special type, and it should be in a printing-related module, not kernel::sync. The reason for this is that do_once_lite! is hooked up to a special section that allows you to reset the once status, and if someone used it for anything not related to printing, they would end up with being incorrectly reset when someone resets pr_warn_once calls. 2. I would suggest just using a relaxed load followed by store instead of xchg. This is what the C-side does, and I think there is no strong reason to deviate. It allows for a single byte of data instead of i32. 3. If we *do* decide to use xchg, then the xchg needs to be guarded by a load so that we avoid a write operation when it's already cleared. Many read ops are much cheaper than many read/write ops. Alice ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-13 10:07 ` Alice Ryhl @ 2025-11-13 11:18 ` FUJITA Tomonori 2025-11-13 12:06 ` Alice Ryhl 2025-11-13 15:20 ` Boqun Feng 0 siblings, 2 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-13 11:18 UTC (permalink / raw) To: aliceryhl Cc: fujita.tomonori, ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, 13 Nov 2025 10:07:36 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > On Wed, Nov 05, 2025 at 02:47:29PM +0900, FUJITA Tomonori wrote: >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >> pr_*_once macros. >> >> A proposal for this feature was made in the past [1], but it didn't >> reach consensus on the implementation and wasn't merged. After reading >> the previous discussions, I implemented it using a different approach. >> >> In the previous proposal, a structure equivalent to std::sync::Once >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >> to provide Once-like semantics by using two atomic values. As pointed >> out in the previous review comments, I think this approach tries to >> provide more functionality than needed, making it unnecessarily >> complex. Also, because data structures in the .data..once section can >> be cleared at any time (via debugfs clear_warn_once), an >> implementation using two atomics wouldn't work correctly. >> >> Therefore, I decided to drop the idea of emulating Once and took a >> minimal approach to implement DO_ONCE_LITE with only one atomic >> variable. While it would be possible to implement the feature entirely >> as a Rust macro, the functionality that can be implemented as regular >> functions has been extracted and implemented as the OnceLite struct >> for better code readability. >> >> Of course, unlike the previous proposal, this uses LKMM atomics. >> >> [1] https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/ > > There has been a fair bit of discussion below. Here is my take on the > way forward: > > 1. OnceLite should be it's own special type, and it should be in a > printing-related module, not kernel::sync. The reason for this is > that do_once_lite! is hooked up to a special section that allows you > to reset the once status, and if someone used it for anything not > related to printing, they would end up with being incorrectly reset > when someone resets pr_warn_once calls. Do you mean that only the do_once_lite! macro, which places the data in .data..once section, should live in a printing-related module? Or should everything including OnceLite struct itself, also be moved there? > 2. I would suggest just using a relaxed load followed by store instead > of xchg. This is what the C-side does, and I think there is no strong > reason to deviate. It allows for a single byte of data instead of i32. You meant best-effort try-once logic like C? pub fn call_once<F>(&self, f: F) -> bool where F: FnOnce(), { if self.0.load(Relaxed) == State::Incomplete { self.0.store(State::Complete, Relaxed); f(); return true; } false } Using u8 as atomic type instead of i32 would require a fair amount of work, since at the moment only i32 and i64 are supported as atomic types, right? > 3. If we *do* decide to use xchg, then the xchg needs to be guarded by a > load so that we avoid a write operation when it's already cleared. > Many read ops are much cheaper than many read/write ops. Make sense. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-13 11:18 ` FUJITA Tomonori @ 2025-11-13 12:06 ` Alice Ryhl 2025-11-14 0:47 ` FUJITA Tomonori 2025-11-13 15:20 ` Boqun Feng 1 sibling, 1 reply; 52+ messages in thread From: Alice Ryhl @ 2025-11-13 12:06 UTC (permalink / raw) To: FUJITA Tomonori Cc: ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, Nov 13, 2025 at 08:18:44PM +0900, FUJITA Tomonori wrote: > On Thu, 13 Nov 2025 10:07:36 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: > > > On Wed, Nov 05, 2025 at 02:47:29PM +0900, FUJITA Tomonori wrote: > >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > >> pr_*_once macros. > >> > >> A proposal for this feature was made in the past [1], but it didn't > >> reach consensus on the implementation and wasn't merged. After reading > >> the previous discussions, I implemented it using a different approach. > >> > >> In the previous proposal, a structure equivalent to std::sync::Once > >> was implemented to realize the DO_ONCE_LITE macro. The approach tried > >> to provide Once-like semantics by using two atomic values. As pointed > >> out in the previous review comments, I think this approach tries to > >> provide more functionality than needed, making it unnecessarily > >> complex. Also, because data structures in the .data..once section can > >> be cleared at any time (via debugfs clear_warn_once), an > >> implementation using two atomics wouldn't work correctly. > >> > >> Therefore, I decided to drop the idea of emulating Once and took a > >> minimal approach to implement DO_ONCE_LITE with only one atomic > >> variable. While it would be possible to implement the feature entirely > >> as a Rust macro, the functionality that can be implemented as regular > >> functions has been extracted and implemented as the OnceLite struct > >> for better code readability. > >> > >> Of course, unlike the previous proposal, this uses LKMM atomics. > >> > >> [1] https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/ > > > > There has been a fair bit of discussion below. Here is my take on the > > way forward: > > > > 1. OnceLite should be it's own special type, and it should be in a > > printing-related module, not kernel::sync. The reason for this is > > that do_once_lite! is hooked up to a special section that allows you > > to reset the once status, and if someone used it for anything not > > related to printing, they would end up with being incorrectly reset > > when someone resets pr_warn_once calls. > > Do you mean that only the do_once_lite! macro, which places the data > in .data..once section, should live in a printing-related module? Or > should everything including OnceLite struct itself, also be moved > there? I think both should live in the same place, and not in kernel::sync. Whether you call the module once_lite or print is not a big deal to me. > > 2. I would suggest just using a relaxed load followed by store instead > > of xchg. This is what the C-side does, and I think there is no strong > > reason to deviate. It allows for a single byte of data instead of i32. > > You meant best-effort try-once logic like C? > > pub fn call_once<F>(&self, f: F) -> bool > where > F: FnOnce(), > { > if self.0.load(Relaxed) == State::Incomplete { > self.0.store(State::Complete, Relaxed); > f(); > return true; > } > false > } Yes that is what I meant. > Using u8 as atomic type instead of i32 would require a fair amount of > work, since at the moment only i32 and i64 are supported as atomic > types, right? Supporting u8 atomics in general is tricky, but I think *specifically* load/store should not be a problem. Alice ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-13 12:06 ` Alice Ryhl @ 2025-11-14 0:47 ` FUJITA Tomonori 2025-11-14 0:57 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-14 0:47 UTC (permalink / raw) To: aliceryhl Cc: fujita.tomonori, ojeda, a.hindborg, bjorn3_gh, boqun.feng, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, 13 Nov 2025 12:06:17 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > On Thu, Nov 13, 2025 at 08:18:44PM +0900, FUJITA Tomonori wrote: >> On Thu, 13 Nov 2025 10:07:36 +0000 >> Alice Ryhl <aliceryhl@google.com> wrote: >> >> > On Wed, Nov 05, 2025 at 02:47:29PM +0900, FUJITA Tomonori wrote: >> >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and >> >> pr_*_once macros. >> >> >> >> A proposal for this feature was made in the past [1], but it didn't >> >> reach consensus on the implementation and wasn't merged. After reading >> >> the previous discussions, I implemented it using a different approach. >> >> >> >> In the previous proposal, a structure equivalent to std::sync::Once >> >> was implemented to realize the DO_ONCE_LITE macro. The approach tried >> >> to provide Once-like semantics by using two atomic values. As pointed >> >> out in the previous review comments, I think this approach tries to >> >> provide more functionality than needed, making it unnecessarily >> >> complex. Also, because data structures in the .data..once section can >> >> be cleared at any time (via debugfs clear_warn_once), an >> >> implementation using two atomics wouldn't work correctly. >> >> >> >> Therefore, I decided to drop the idea of emulating Once and took a >> >> minimal approach to implement DO_ONCE_LITE with only one atomic >> >> variable. While it would be possible to implement the feature entirely >> >> as a Rust macro, the functionality that can be implemented as regular >> >> functions has been extracted and implemented as the OnceLite struct >> >> for better code readability. >> >> >> >> Of course, unlike the previous proposal, this uses LKMM atomics. >> >> >> >> [1] https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/ >> > >> > There has been a fair bit of discussion below. Here is my take on the >> > way forward: >> > >> > 1. OnceLite should be it's own special type, and it should be in a >> > printing-related module, not kernel::sync. The reason for this is >> > that do_once_lite! is hooked up to a special section that allows you >> > to reset the once status, and if someone used it for anything not >> > related to printing, they would end up with being incorrectly reset >> > when someone resets pr_warn_once calls. >> >> Do you mean that only the do_once_lite! macro, which places the data >> in .data..once section, should live in a printing-related module? Or >> should everything including OnceLite struct itself, also be moved >> there? > > I think both should live in the same place, and not in kernel::sync. > Whether you call the module once_lite or print is not a big deal to me. Understood. In that case, it seems to me that either adding everything to print.rs or creating a print/once_lite.rs would work well. >> > 2. I would suggest just using a relaxed load followed by store instead >> > of xchg. This is what the C-side does, and I think there is no strong >> > reason to deviate. It allows for a single byte of data instead of i32. >> >> You meant best-effort try-once logic like C? >> >> pub fn call_once<F>(&self, f: F) -> bool >> where >> F: FnOnce(), >> { >> if self.0.load(Relaxed) == State::Incomplete { >> self.0.store(State::Complete, Relaxed); >> f(); >> return true; >> } >> false >> } > > Yes that is what I meant. Sounds good to me. As I wrote in another mail, I think that it's fine to use the C-side logic here. >> Using u8 as atomic type instead of i32 would require a fair amount of >> work, since at the moment only i32 and i64 are supported as atomic >> types, right? > > Supporting u8 atomics in general is tricky, but I think *specifically* > load/store should not be a problem. You meant using read_volatile/write_volatile? Since we only need "Relaxed" order so something like the following? #[repr(transparent)] struct AtomicU8(UnsafeCell<u8>); impl AtomicU8 { #[inline(always)] fn load(&self) -> u8 { unsafe { core::ptr::read_volatile(self.0.get()) } } #[inline(always)] fn store(&self, v: u8) { unsafe { core::ptr::write_volatile(self.0.get(), v) } } } ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-14 0:47 ` FUJITA Tomonori @ 2025-11-14 0:57 ` Boqun Feng 2025-11-14 1:12 ` FUJITA Tomonori 0 siblings, 1 reply; 52+ messages in thread From: Boqun Feng @ 2025-11-14 0:57 UTC (permalink / raw) To: FUJITA Tomonori Cc: aliceryhl, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Fri, Nov 14, 2025 at 09:47:30AM +0900, FUJITA Tomonori wrote: [...] > >> Using u8 as atomic type instead of i32 would require a fair amount of > >> work, since at the moment only i32 and i64 are supported as atomic > >> types, right? > > > > Supporting u8 atomics in general is tricky, but I think *specifically* > > load/store should not be a problem. > > You meant using read_volatile/write_volatile? Since we only need > "Relaxed" order so something like the following? > > #[repr(transparent)] > struct AtomicU8(UnsafeCell<u8>); > Please don't. Please make it work with Atomic<T>, also store release and load acquire should be introduced as well. Regards, Boqun > impl AtomicU8 { > #[inline(always)] > fn load(&self) -> u8 { > unsafe { core::ptr::read_volatile(self.0.get()) } > } > > #[inline(always)] > fn store(&self, v: u8) { > unsafe { core::ptr::write_volatile(self.0.get(), v) } > } > } ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-14 0:57 ` Boqun Feng @ 2025-11-14 1:12 ` FUJITA Tomonori 2025-11-14 1:19 ` Boqun Feng 0 siblings, 1 reply; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-14 1:12 UTC (permalink / raw) To: boqun.feng Cc: fujita.tomonori, aliceryhl, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, 13 Nov 2025 16:57:18 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Fri, Nov 14, 2025 at 09:47:30AM +0900, FUJITA Tomonori wrote: > [...] >> >> Using u8 as atomic type instead of i32 would require a fair amount of >> >> work, since at the moment only i32 and i64 are supported as atomic >> >> types, right? >> > >> > Supporting u8 atomics in general is tricky, but I think *specifically* >> > load/store should not be a problem. >> >> You meant using read_volatile/write_volatile? Since we only need >> "Relaxed" order so something like the following? >> >> #[repr(transparent)] >> struct AtomicU8(UnsafeCell<u8>); >> > > Please don't. Please make it work with Atomic<T>, also store release and > load acquire should be introduced as well. I wanted to confirm what kind of u8 load/store implementation Alice has in mind, and whether it would be used only for OnceLite. Of course. If we add something like that to kernel/sync, I agree that it should be implemented in that way. If not, I'm not sure. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-14 1:12 ` FUJITA Tomonori @ 2025-11-14 1:19 ` Boqun Feng 2025-11-14 9:48 ` Alice Ryhl 2025-11-14 13:47 ` FUJITA Tomonori 0 siblings, 2 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-14 1:19 UTC (permalink / raw) To: FUJITA Tomonori Cc: aliceryhl, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Fri, Nov 14, 2025 at 10:12:08AM +0900, FUJITA Tomonori wrote: > On Thu, 13 Nov 2025 16:57:18 -0800 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Fri, Nov 14, 2025 at 09:47:30AM +0900, FUJITA Tomonori wrote: > > [...] > >> >> Using u8 as atomic type instead of i32 would require a fair amount of > >> >> work, since at the moment only i32 and i64 are supported as atomic > >> >> types, right? > >> > > >> > Supporting u8 atomics in general is tricky, but I think *specifically* > >> > load/store should not be a problem. > >> > >> You meant using read_volatile/write_volatile? Since we only need > >> "Relaxed" order so something like the following? > >> > >> #[repr(transparent)] > >> struct AtomicU8(UnsafeCell<u8>); > >> > > > > Please don't. Please make it work with Atomic<T>, also store release and > > load acquire should be introduced as well. > > I wanted to confirm what kind of u8 load/store implementation Alice > has in mind, and whether it would be used only for OnceLite. > > Of course. If we add something like that to kernel/sync, I agree that > it should be implemented in that way. If not, I'm not sure. > Yeah, I'm afraid that you want to implement something only for OneLite, and that'll introduce fragments for maintaining. For Atomic<T>, supporting load/store/xchg/cmpxchg is definitely the plan, so appreciate if you could help while working on this pr_*_once(). Alternatively, we can start with i32, and improve that later. Thanks! Regards, Boqun ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-14 1:19 ` Boqun Feng @ 2025-11-14 9:48 ` Alice Ryhl 2025-11-14 13:55 ` FUJITA Tomonori 2025-11-14 13:47 ` FUJITA Tomonori 1 sibling, 1 reply; 52+ messages in thread From: Alice Ryhl @ 2025-11-14 9:48 UTC (permalink / raw) To: Boqun Feng Cc: FUJITA Tomonori, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, Nov 13, 2025 at 05:19:10PM -0800, Boqun Feng wrote: > On Fri, Nov 14, 2025 at 10:12:08AM +0900, FUJITA Tomonori wrote: > > On Thu, 13 Nov 2025 16:57:18 -0800 > > Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > On Fri, Nov 14, 2025 at 09:47:30AM +0900, FUJITA Tomonori wrote: > > > [...] > > >> >> Using u8 as atomic type instead of i32 would require a fair amount of > > >> >> work, since at the moment only i32 and i64 are supported as atomic > > >> >> types, right? > > >> > > > >> > Supporting u8 atomics in general is tricky, but I think *specifically* > > >> > load/store should not be a problem. > > >> > > >> You meant using read_volatile/write_volatile? Since we only need > > >> "Relaxed" order so something like the following? > > >> > > >> #[repr(transparent)] > > >> struct AtomicU8(UnsafeCell<u8>); > > >> > > > > > > Please don't. Please make it work with Atomic<T>, also store release and > > > load acquire should be introduced as well. > > > > I wanted to confirm what kind of u8 load/store implementation Alice > > has in mind, and whether it would be used only for OnceLite. > > > > Of course. If we add something like that to kernel/sync, I agree that > > it should be implemented in that way. If not, I'm not sure. > > > > Yeah, I'm afraid that you want to implement something only for OneLite, > and that'll introduce fragments for maintaining. > > For Atomic<T>, supporting load/store/xchg/cmpxchg is definitely the > plan, so appreciate if you could help while working on this pr_*_once(). > > Alternatively, we can start with i32, and improve that later. I read over this discussion. Perhaps it's best that we go with i32 with xchg after all to avoid adding new atomic types for this feature. Still needs a load, though. /// Calls the provided function exactly once. pub fn call_once<F>(&self, f: F) -> bool where F: FnOnce(), { let old = self.0.load(Relaxed); if old == State::Complete { return false; } let old = self.0.xchg(State::Complete, Relaxed); if old == State::Complete { return false; } f(); true } This ensures that we do not perform a write operation when we don't need to. Alice ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-14 9:48 ` Alice Ryhl @ 2025-11-14 13:55 ` FUJITA Tomonori 0 siblings, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-14 13:55 UTC (permalink / raw) To: aliceryhl Cc: boqun.feng, fujita.tomonori, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Fri, 14 Nov 2025 09:48:46 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > On Thu, Nov 13, 2025 at 05:19:10PM -0800, Boqun Feng wrote: >> On Fri, Nov 14, 2025 at 10:12:08AM +0900, FUJITA Tomonori wrote: >> > On Thu, 13 Nov 2025 16:57:18 -0800 >> > Boqun Feng <boqun.feng@gmail.com> wrote: >> > >> > > On Fri, Nov 14, 2025 at 09:47:30AM +0900, FUJITA Tomonori wrote: >> > > [...] >> > >> >> Using u8 as atomic type instead of i32 would require a fair amount of >> > >> >> work, since at the moment only i32 and i64 are supported as atomic >> > >> >> types, right? >> > >> > >> > >> > Supporting u8 atomics in general is tricky, but I think *specifically* >> > >> > load/store should not be a problem. >> > >> >> > >> You meant using read_volatile/write_volatile? Since we only need >> > >> "Relaxed" order so something like the following? >> > >> >> > >> #[repr(transparent)] >> > >> struct AtomicU8(UnsafeCell<u8>); >> > >> >> > > >> > > Please don't. Please make it work with Atomic<T>, also store release and >> > > load acquire should be introduced as well. >> > >> > I wanted to confirm what kind of u8 load/store implementation Alice >> > has in mind, and whether it would be used only for OnceLite. >> > >> > Of course. If we add something like that to kernel/sync, I agree that >> > it should be implemented in that way. If not, I'm not sure. >> > >> >> Yeah, I'm afraid that you want to implement something only for OneLite, >> and that'll introduce fragments for maintaining. >> >> For Atomic<T>, supporting load/store/xchg/cmpxchg is definitely the >> plan, so appreciate if you could help while working on this pr_*_once(). >> >> Alternatively, we can start with i32, and improve that later. > > I read over this discussion. Perhaps it's best that we go with i32 with > xchg after all to avoid adding new atomic types for this feature. > > Still needs a load, though. > > /// Calls the provided function exactly once. > pub fn call_once<F>(&self, f: F) -> bool > where > F: FnOnce(), > { > let old = self.0.load(Relaxed); > if old == State::Complete { > return false; > } > > let old = self.0.xchg(State::Complete, Relaxed); > if old == State::Complete { > return false; > } > > f(); > true > } > > This ensures that we do not perform a write operation when we don't need > to. Yes, that makes sense. Let's proceed with this for now. Once support for Atomic<u8> load/store is available, we can revisit it. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-14 1:19 ` Boqun Feng 2025-11-14 9:48 ` Alice Ryhl @ 2025-11-14 13:47 ` FUJITA Tomonori 1 sibling, 0 replies; 52+ messages in thread From: FUJITA Tomonori @ 2025-11-14 13:47 UTC (permalink / raw) To: boqun.feng Cc: fujita.tomonori, aliceryhl, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io On Thu, 13 Nov 2025 17:19:10 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Fri, Nov 14, 2025 at 10:12:08AM +0900, FUJITA Tomonori wrote: >> On Thu, 13 Nov 2025 16:57:18 -0800 >> Boqun Feng <boqun.feng@gmail.com> wrote: >> >> > On Fri, Nov 14, 2025 at 09:47:30AM +0900, FUJITA Tomonori wrote: >> > [...] >> >> >> Using u8 as atomic type instead of i32 would require a fair amount of >> >> >> work, since at the moment only i32 and i64 are supported as atomic >> >> >> types, right? >> >> > >> >> > Supporting u8 atomics in general is tricky, but I think *specifically* >> >> > load/store should not be a problem. >> >> >> >> You meant using read_volatile/write_volatile? Since we only need >> >> "Relaxed" order so something like the following? >> >> >> >> #[repr(transparent)] >> >> struct AtomicU8(UnsafeCell<u8>); >> >> >> > >> > Please don't. Please make it work with Atomic<T>, also store release and >> > load acquire should be introduced as well. >> >> I wanted to confirm what kind of u8 load/store implementation Alice >> has in mind, and whether it would be used only for OnceLite. >> >> Of course. If we add something like that to kernel/sync, I agree that >> it should be implemented in that way. If not, I'm not sure. >> > > Yeah, I'm afraid that you want to implement something only for OneLite, > and that'll introduce fragments for maintaining. > > For Atomic<T>, supporting load/store/xchg/cmpxchg is definitely the > plan, so appreciate if you could help while working on this pr_*_once(). Ah, if that's the plan, then of course it makes sense to work on kernle/sync instead of adding something only for OnceLite. Let me give it a try. > Alternatively, we can start with i32, and improve that later. Sounds perfect. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v1 0/2] Add support for print exactly once 2025-11-13 11:18 ` FUJITA Tomonori 2025-11-13 12:06 ` Alice Ryhl @ 2025-11-13 15:20 ` Boqun Feng 1 sibling, 0 replies; 52+ messages in thread From: Boqun Feng @ 2025-11-13 15:20 UTC (permalink / raw) To: FUJITA Tomonori Cc: aliceryhl, ojeda, a.hindborg, bjorn3_gh, dakr, gary, lossin, rust-for-linux, tmgross, jens.korinth.tuta.io, Peter Zijlstra On Thu, Nov 13, 2025 at 08:18:44PM +0900, FUJITA Tomonori wrote: > On Thu, 13 Nov 2025 10:07:36 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: > > > On Wed, Nov 05, 2025 at 02:47:29PM +0900, FUJITA Tomonori wrote: > >> This adds the Rust equivalent of the kernel's DO_ONCE_LITE and > >> pr_*_once macros. > >> > >> A proposal for this feature was made in the past [1], but it didn't > >> reach consensus on the implementation and wasn't merged. After reading > >> the previous discussions, I implemented it using a different approach. > >> > >> In the previous proposal, a structure equivalent to std::sync::Once > >> was implemented to realize the DO_ONCE_LITE macro. The approach tried > >> to provide Once-like semantics by using two atomic values. As pointed > >> out in the previous review comments, I think this approach tries to > >> provide more functionality than needed, making it unnecessarily > >> complex. Also, because data structures in the .data..once section can > >> be cleared at any time (via debugfs clear_warn_once), an > >> implementation using two atomics wouldn't work correctly. > >> > >> Therefore, I decided to drop the idea of emulating Once and took a > >> minimal approach to implement DO_ONCE_LITE with only one atomic > >> variable. While it would be possible to implement the feature entirely > >> as a Rust macro, the functionality that can be implemented as regular > >> functions has been extracted and implemented as the OnceLite struct > >> for better code readability. > >> > >> Of course, unlike the previous proposal, this uses LKMM atomics. > >> > >> [1] https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/ > > > > There has been a fair bit of discussion below. Here is my take on the > > way forward: > > Thank Alice for putting together the summary of the discussion, add Peter Cced as well. For context, Peter, this is about having the pr_*_once() on Rust side. Also Alice has a reply to Tomo, but I'm copying this to you because it contains more context. > > 1. OnceLite should be it's own special type, and it should be in a > > printing-related module, not kernel::sync. The reason for this is > > that do_once_lite! is hooked up to a special section that allows you > > to reset the once status, and if someone used it for anything not > > related to printing, they would end up with being incorrectly reset > > when someone resets pr_warn_once calls. > > Do you mean that only the do_once_lite! macro, which places the data > in .data..once section, should live in a printing-related module? Or > should everything including OnceLite struct itself, also be moved > there? > I know Alice mentioned in a reply that she prefers to moving both, but I think with a reset() function [1] added, it's totally fine to keep OnceLite in kernel::sync and make do_once_lite!() an internal macro (i.e. not exposed) for pr_*_once!() only. But let's first focus on more important differences below. > > 2. I would suggest just using a relaxed load followed by store instead > > of xchg. This is what the C-side does, and I think there is no strong > > reason to deviate. It allows for a single byte of data instead of i32. > > You meant best-effort try-once logic like C? > > pub fn call_once<F>(&self, f: F) -> bool > where > F: FnOnce(), > { > if self.0.load(Relaxed) == State::Incomplete { > self.0.store(State::Complete, Relaxed); > f(); > return true; > } > false > } > I understand Alice's point, but personally, I think the C's DO_ONCE_LITE() was implemented wrongly, because it DOES NOT guarantee "do something once", hence I suggested we don't repeat that logic in the Rust version. Speak of personal experience, sometimes I do need to provide an educational guess of "what's going on" with only one copy of a kernel log, and I think I'm not alone. For that purpose, the uncertainty of "it may print more than once" does matter. My point is more about since it's a logging mechanism, it better be accurate than efficient because the logging itself would take more time and space. > Using u8 as atomic type instead of i32 would require a fair amount of > work, since at the moment only i32 and i64 are supported as atomic > types, right? > I agree with Alice in the other reply that u8 store and load (even xchg() and cmpxchg()) are not hard to add. However, depending on which logic we decide to use: 1. If we use the current C logic (i.e. no xchg() and it might do things more than once), then using u8 is fine and saves spaces. 2. If we use the correct "doing once" logic, that means we have to use xchg() and we need to notice that xchg() on certain architectures (RISC for example), will take more than 1 instruction on u8. Therefore although we save the 3 bytes by using u8 instead of u32, we will spend 4 more instructions. That's something to take into consideration. Maybe u8/u32 can be an architecture-specific thing. But we can push it as a future patch. > > > 3. If we *do* decide to use xchg, then the xchg needs to be guarded by a > > load so that we avoid a write operation when it's already cleared. > > Many read ops are much cheaper than many read/write ops. > > Make sense. Agreed. Regards, Boqun > [1]: https://lore.kernel.org/rust-for-linux/aRPvjQJLdzvxLUpr@tardis.local/ ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2025-11-15 13:37 UTC | newest]
Thread overview: 52+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <nsSZZk6z9Ia7Gl5JS9LVNDRjc-9eFvtSWyLI4SSjsHNouDkDV-GmXnixMYlIpGHryPfhLr8Z2W_CkWd6D2frYQ==@protonmail.internalid>
2025-11-05 5:47 ` [PATCH v1 0/2] Add support for print exactly once FUJITA Tomonori
2025-11-05 5:47 ` [PATCH v1 1/2] rust: Add support for calling a function " FUJITA Tomonori
2025-11-05 9:21 ` Onur Özkan
2025-11-05 10:35 ` Alice Ryhl
2025-11-05 10:32 ` Alice Ryhl
2025-11-06 0:34 ` FUJITA Tomonori
2025-11-05 16:19 ` Boqun Feng
2025-11-06 0:10 ` FUJITA Tomonori
2025-11-06 14:46 ` Boqun Feng
2025-11-07 9:03 ` Alice Ryhl
2025-11-10 9:21 ` FUJITA Tomonori
2025-11-10 16:14 ` Boqun Feng
2025-11-10 16:37 ` Miguel Ojeda
2025-11-10 16:55 ` Boqun Feng
2025-11-11 21:42 ` Miguel Ojeda
2025-11-11 3:09 ` FUJITA Tomonori
2025-11-11 5:17 ` Boqun Feng
2025-11-11 9:12 ` Andreas Hindborg
2025-11-11 23:38 ` FUJITA Tomonori
2025-11-12 9:04 ` Andreas Hindborg
2025-11-15 13:37 ` FUJITA Tomonori
2025-11-11 21:43 ` FUJITA Tomonori
2025-11-12 1:30 ` Boqun Feng
2025-11-12 2:23 ` Boqun Feng
2025-11-12 9:10 ` Andreas Hindborg
2025-11-14 15:03 ` Boqun Feng
2025-11-12 13:17 ` FUJITA Tomonori
2025-11-05 5:47 ` [PATCH v1 2/2] rust: Add pr_*_once macros FUJITA Tomonori
2025-11-05 10:33 ` Alice Ryhl
2025-11-05 20:59 ` [PATCH v1 0/2] Add support for print exactly once Andreas Hindborg
2025-11-05 23:12 ` FUJITA Tomonori
2025-11-06 14:31 ` Boqun Feng
2025-11-10 12:16 ` Andreas Hindborg
2025-11-10 16:08 ` Boqun Feng
2025-11-11 9:02 ` Andreas Hindborg
2025-11-12 0:45 ` FUJITA Tomonori
2025-11-12 1:04 ` Boqun Feng
2025-11-12 1:18 ` FUJITA Tomonori
2025-11-12 1:35 ` Boqun Feng
2025-11-13 9:55 ` FUJITA Tomonori
2025-11-11 1:28 ` FUJITA Tomonori
2025-11-13 10:07 ` Alice Ryhl
2025-11-13 11:18 ` FUJITA Tomonori
2025-11-13 12:06 ` Alice Ryhl
2025-11-14 0:47 ` FUJITA Tomonori
2025-11-14 0:57 ` Boqun Feng
2025-11-14 1:12 ` FUJITA Tomonori
2025-11-14 1:19 ` Boqun Feng
2025-11-14 9:48 ` Alice Ryhl
2025-11-14 13:55 ` FUJITA Tomonori
2025-11-14 13:47 ` FUJITA Tomonori
2025-11-13 15:20 ` Boqun Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).