rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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
  0 siblings, 0 replies; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ 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; 51+ 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] 51+ messages in thread

end of thread, other threads:[~2025-11-14 15:03 UTC | newest]

Thread overview: 51+ 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-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).