* [PATCH v4 0/3] rust: Add pr_*_once macros @ 2024-11-26 16:40 ` Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay ` (4 more replies) 0 siblings, 5 replies; 27+ messages in thread From: Jens Korinth via B4 Relay @ 2024-11-26 16:40 UTC (permalink / raw) To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross Cc: rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel, Jens Korinth Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once functions, which print a message only once. Introduces a `OnceLite` abstraction similar to Rust's [`std::sync::Once`](https://doc.rust-lang.org/std/sync/struct.Once.html) but using the non-blocking mechanism from the kernel's `DO_ONCE_LITE` macro, which is used to define the `do_once_lite` Rust macro. First use case are an implementation of `pr_*_once` message macros to print a message only once. v4: - Move `do_once_lite` macro implementation to `OnceLite` abstraction - Use `OnceLite` in `do_once_lite` - More documentation, examples v3: https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/ - Fix rustdoc error, formatting issues - Fix missing Signed-off-by v2: https://lore.kernel.org/r/20241107-pr_once_macros-v2-0-dc0317ff301e@tuta.io - Split patch into do_once_lite part and pr_*_once macros - Add macro rule for call without condition => renamed to do_once_lite - Used condition-less call in pr_*_once macros - Added examples - Removed TODO in kernel/error.rs using pr_warn_once v1: https://lore.kernel.org/rust-for-linux/20241106.083113.356536037967804464.fujita.tomonori@gmail.com/ Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Co-developed-by: Boqun Feng <boqun.feng@gmail.com> Signed-off-by: Jens Korinth <jens.korinth@tuta.io> --- FUJITA Tomonori (1): rust: print: Add pr_*_once macros Jens Korinth (2): rust: Add `OnceLite` for executing code once rust: error: Replace pr_warn by pr_warn_once rust/kernel/error.rs | 3 +- rust/kernel/lib.rs | 1 + rust/kernel/once_lite.rs | 127 +++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/print.rs | 126 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 255 insertions(+), 2 deletions(-) --- base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb change-id: 20241107-pr_once_macros-6438e6f5b923 Best regards, -- Jens Korinth <jens.korinth@tuta.io> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-26 16:40 ` [PATCH v4 0/3] rust: Add pr_*_once macros Jens Korinth via B4 Relay @ 2024-11-26 16:40 ` Jens Korinth via B4 Relay 2024-11-26 18:57 ` Daniel Sedlak ` (3 more replies) 2024-11-26 16:40 ` [PATCH v4 2/3] rust: print: Add pr_*_once macros Jens Korinth via B4 Relay ` (3 subsequent siblings) 4 siblings, 4 replies; 27+ messages in thread From: Jens Korinth via B4 Relay @ 2024-11-26 16:40 UTC (permalink / raw) To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross Cc: rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel, Jens Korinth From: Jens Korinth <jens.korinth@tuta.io> Similar to `Once` in Rust's standard library, but with the same non-blocking behavior as the kernel's `DO_ONCE_LITE` macro. Abstraction allows easy replacement of the underlying sync mechanisms, see https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/. Suggested-by: Boqun Feng <boqun.feng@gmail.com> Signed-off-by: Jens Korinth <jens.korinth@tuta.io> --- rust/kernel/lib.rs | 1 + rust/kernel/once_lite.rs | 127 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index bf8d7f841f9425d19a24f3910929839cfe705c7f..2b0a80435d24f5e168679ec2e25bd68cd970dcdd 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -44,6 +44,7 @@ pub mod list; #[cfg(CONFIG_NET)] pub mod net; +pub mod once_lite; pub mod page; pub mod prelude; pub mod print; diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs new file mode 100644 index 0000000000000000000000000000000000000000..723c3244fc856fe974ddd33de5415e7ced37f315 --- /dev/null +++ b/rust/kernel/once_lite.rs @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! A one-time only global execution primitive. +//! +//! This primitive is meant to be used to execute code only once. It is +//! similar in design to Rust's +//! [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html), +//! but borrowing the non-blocking mechanism used in the kernel's +//! [`DO_ONCE_LITE`] macro. +//! +//! An example use case would be to print a message only the first time. +//! +//! [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h +//! +//! C header: [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) +//! +//! Reference: <https://doc.rust-lang.org/std/sync/struct.Once.html> + +use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; + +/// A low-level synchronization primitive for one-time global execution. +/// +/// Based on the +/// [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html) +/// interface, but internally equivalent the kernel's [`DO_ONCE_LITE`] +/// macro. The Rust macro `do_once_lite` replacing it uses `OnceLite` +/// internally. +/// +/// [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h +/// +/// # Examples +/// +/// ```rust +/// static START: kernel::once_lite::OnceLite = +/// kernel::once_lite::OnceLite::new(); +/// +/// let mut x: i32 = 0; +/// +/// START.call_once(|| { +/// // run initialization here +/// x = 42; +/// }); +/// while !START.is_completed() { /* busy wait */ } +/// assert_eq!(x, 42); +/// ``` +/// +pub struct OnceLite(AtomicBool, AtomicBool); + +impl OnceLite { + /// Creates a new `OnceLite` value. + #[inline(always)] + pub const fn new() -> Self { + Self(AtomicBool::new(false), AtomicBool::new(false)) + } + + /// Performs an initialization routine once and only once. The given + /// closure will be executed if this is the first time `call_once` has + /// been called, and otherwise the routine will not be invoked. + /// + /// This method will _not_ block the calling thread if another + /// initialization is currently running. It is _not_ guaranteed that the + /// initialization routine will have completed by the time the calling + /// thread continues execution. + /// + /// Note that this is different from the guarantees made by + /// [`std::sync::Once`], but identical to the way the implementation of + /// the kernel's [`DO_ONCE_LITE_IF`] macro is behaving at the time of + /// writing. + /// + /// [`std::sync::Once`]: https://doc.rust-lang.org/std/sync/struct.Once.html + /// [`DO_ONCE_LITE_IF`]: srctree/include/once_lite.h + #[inline(always)] + pub fn call_once<F: FnOnce()>(&self, f: F) { + if !self.0.load(Relaxed) && !self.0.swap(true, Relaxed) { + f() + }; + self.1.store(true, Relaxed); + } + + /// Returns `true` if some `call_once` call has completed successfully. + /// Specifically, `is_completed` will return `false` in the following + /// situations: + /// + /// 1. `call_once()` was not called at all, + /// 2. `call_once()` was called, but has not yet completed. + /// + /// # Examples + /// + /// ```rust + /// static INIT: kernel::once_lite::OnceLite = + /// kernel::once_lite::OnceLite::new(); + /// + /// assert_eq!(INIT.is_completed(), false); + /// INIT.call_once(|| { + /// assert_eq!(INIT.is_completed(), false); + /// }); + /// assert_eq!(INIT.is_completed(), true); + /// ``` + #[inline(always)] + pub fn is_completed(&self) -> bool { + self.1.load(Relaxed) + } +} + +/// Executes code only once. +/// +/// Equivalent to the kernel's [`DO_ONCE_LITE`] macro: Expression is +/// evaluated at most once by the first thread, other threads will not be +/// blocked while executing in first thread, nor are there any guarantees +/// regarding the visibility of side-effects of the called expression. +/// +/// [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h +/// +/// # Examples +/// +/// ``` +/// let mut x: i32 = 0; +/// kernel::do_once_lite!((||{x = 42;})()); +/// ``` +#[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.47.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay @ 2024-11-26 18:57 ` Daniel Sedlak 2024-11-27 19:46 ` jens.korinth 2024-11-26 19:00 ` Miguel Ojeda ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Daniel Sedlak @ 2024-11-26 18:57 UTC (permalink / raw) To: jens.korinth, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross Cc: rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel On 11/26/24 5:40 PM, Jens Korinth via B4 Relay wrote: > From: Jens Korinth <jens.korinth@tuta.io> > > Similar to `Once` in Rust's standard library, but with the same > non-blocking behavior as the kernel's `DO_ONCE_LITE` macro. Abstraction > allows easy replacement of the underlying sync mechanisms, see > > https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/. > > Suggested-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Jens Korinth <jens.korinth@tuta.io> > --- > rust/kernel/lib.rs | 1 + > rust/kernel/once_lite.rs | 127 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 128 insertions(+) > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index bf8d7f841f9425d19a24f3910929839cfe705c7f..2b0a80435d24f5e168679ec2e25bd68cd970dcdd 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -44,6 +44,7 @@ > pub mod list; > #[cfg(CONFIG_NET)] > pub mod net; > +pub mod once_lite; > pub mod page; > pub mod prelude; > pub mod print; > diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..723c3244fc856fe974ddd33de5415e7ced37f315 > --- /dev/null > +++ b/rust/kernel/once_lite.rs > @@ -0,0 +1,127 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! A one-time only global execution primitive. > +//! > +//! This primitive is meant to be used to execute code only once. It is > +//! similar in design to Rust's > +//! [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html), > +//! but borrowing the non-blocking mechanism used in the kernel's > +//! [`DO_ONCE_LITE`] macro. > +//! > +//! An example use case would be to print a message only the first time. > +//! > +//! [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h > +//! > +//! C header: [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) > +//! > +//! Reference: <https://doc.rust-lang.org/std/sync/struct.Once.html> > + > +use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; > + > +/// A low-level synchronization primitive for one-time global execution. > +/// > +/// Based on the > +/// [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html) > +/// interface, but internally equivalent the kernel's [`DO_ONCE_LITE`] > +/// macro. The Rust macro `do_once_lite` replacing it uses `OnceLite` > +/// internally. > +/// > +/// [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h > +/// > +/// # Examples > +/// > +/// ```rust The `rust` part should be default value for rustdoc tests, can we please omit that? > +/// static START: kernel::once_lite::OnceLite = > +/// kernel::once_lite::OnceLite::new(); > +/// > +/// let mut x: i32 = 0; > +/// > +/// START.call_once(|| { > +/// // run initialization here > +/// x = 42; > +/// }); > +/// while !START.is_completed() { /* busy wait */ } > +/// assert_eq!(x, 42); > +/// ``` > +/// > +pub struct OnceLite(AtomicBool, AtomicBool); Have you considered it to be implemented like `AtomicU32`? I think™ that one atomic variable is more than enough. > + > +impl OnceLite { > + /// Creates a new `OnceLite` value. > + #[inline(always)] > + pub const fn new() -> Self { > + Self(AtomicBool::new(false), AtomicBool::new(false)) > + } > + > + /// Performs an initialization routine once and only once. The given > + /// closure will be executed if this is the first time `call_once` has > + /// been called, and otherwise the routine will not be invoked. > + /// > + /// This method will _not_ block the calling thread if another > + /// initialization is currently running. It is _not_ guaranteed that the > + /// initialization routine will have completed by the time the calling > + /// thread continues execution. > + /// > + /// Note that this is different from the guarantees made by > + /// [`std::sync::Once`], but identical to the way the implementation of > + /// the kernel's [`DO_ONCE_LITE_IF`] macro is behaving at the time of > + /// writing. > + /// > + /// [`std::sync::Once`]: https://doc.rust-lang.org/std/sync/struct.Once.html > + /// [`DO_ONCE_LITE_IF`]: srctree/include/once_lite.h > + #[inline(always)] > + pub fn call_once<F: FnOnce()>(&self, f: F) { > + if !self.0.load(Relaxed) && !self.0.swap(true, Relaxed) { > + f() > + }; > + self.1.store(true, Relaxed); I think the memory ordering is wrong here. Since it is `Relaxed`, the `self.1` and `self.0` can get reordered during execution. So `self.0` can be false, even though `self.1` will be true. Not on x86, but on architectures with weaker memory models it can happen. Even the implementation in the Rust std, uses at least `Acquire`/`Release`, where the reordering shouldn't be possible see [1]. [1]: https://github.com/rust-lang/rust/blob/master/library/std/src/sys/sync/once/futex.rs > + } > + > + /// Returns `true` if some `call_once` call has completed successfully. > + /// Specifically, `is_completed` will return `false` in the following > + /// situations: > + /// > + /// 1. `call_once()` was not called at all, > + /// 2. `call_once()` was called, but has not yet completed. > + /// > + /// # Examples > + /// > + /// ```rust Also here the `rust` part should be the default value. > + /// static INIT: kernel::once_lite::OnceLite = > + /// kernel::once_lite::OnceLite::new(); > + /// > + /// assert_eq!(INIT.is_completed(), false); > + /// INIT.call_once(|| { > + /// assert_eq!(INIT.is_completed(), false); > + /// }); > + /// assert_eq!(INIT.is_completed(), true); > + /// ``` > + #[inline(always)] > + pub fn is_completed(&self) -> bool { > + self.1.load(Relaxed) > + } > +} > + > +/// Executes code only once. > +/// > +/// Equivalent to the kernel's [`DO_ONCE_LITE`] macro: Expression is > +/// evaluated at most once by the first thread, other threads will not be > +/// blocked while executing in first thread, nor are there any guarantees > +/// regarding the visibility of side-effects of the called expression. > +/// > +/// [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h > +/// > +/// # Examples > +/// > +/// ``` > +/// let mut x: i32 = 0; > +/// kernel::do_once_lite!((||{x = 42;})()); > +/// ``` > +#[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); > + }}; > +} > Thanks for working on that! Daniel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-26 18:57 ` Daniel Sedlak @ 2024-11-27 19:46 ` jens.korinth 2024-11-29 8:22 ` Daniel Sedlak 0 siblings, 1 reply; 27+ messages in thread From: jens.korinth @ 2024-11-27 19:46 UTC (permalink / raw) To: Daniel Sedlak Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel > Have you considered it to be implemented like `AtomicU32`? I think™ that > one atomic variable is more than enough. Just to clarify - you mean something like this? Nevermind the magic numbers, I'd replace them, of course. diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs index 723c3244fc85..0622ecbfced5 100644 --- a/rust/kernel/once_lite.rs +++ b/rust/kernel/once_lite.rs @@ -16,7 +16,7 @@ //! //! Reference: <https://doc.rust-lang.org/std/sync/struct.Once.html> -use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; +use core::sync::atomic::{AtomicU32, Ordering::Acquire, Ordering::Relaxed}; /// A low-level synchronization primitive for one-time global execution. /// @@ -44,13 +44,13 @@ /// assert_eq!(x, 42); /// ``` /// -pub struct OnceLite(AtomicBool, AtomicBool); +pub struct OnceLite(AtomicU32); impl OnceLite { /// Creates a new `OnceLite` value. #[inline(always)] pub const fn new() -> Self { - Self(AtomicBool::new(false), AtomicBool::new(false)) + Self(AtomicU32::new(0)) } /// Performs an initialization routine once and only once. The given @@ -71,10 +71,10 @@ pub const fn new() -> Self { /// [`DO_ONCE_LITE_IF`]: srctree/include/once_lite.h #[inline(always)] pub fn call_once<F: FnOnce()>(&self, f: F) { - if !self.0.load(Relaxed) && !self.0.swap(true, Relaxed) { - f() + if self.0.load(Relaxed) == 0 && self.0.compare_exchange(0, 1, Acquire, Relaxed) == Ok(0) { + f(); + self.0.store(2, Relaxed); }; - self.1.store(true, Relaxed); } /// Returns `true` if some `call_once` call has completed successfully. @@ -98,7 +98,7 @@ pub fn call_once<F: FnOnce()>(&self, f: F) { /// ``` #[inline(always)] pub fn is_completed(&self) -> bool { - self.1.load(Relaxed) + self.0.load(Relaxed) > 1 } } > The `rust` part should be default value for rustdoc tests, can we please > omit that? Will do! Jens ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-27 19:46 ` jens.korinth @ 2024-11-29 8:22 ` Daniel Sedlak 0 siblings, 0 replies; 27+ messages in thread From: Daniel Sedlak @ 2024-11-29 8:22 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On 11/27/24 8:46 PM, jens.korinth@tuta.io wrote: >> Have you considered it to be implemented like `AtomicU32`? I think™ that >> one atomic variable is more than enough. > > Just to clarify - you mean something like this? Nevermind the magic numbers, > I'd replace them, of course. Yes, that's what I meant. But as Alice pointed out, you _may_ be able to reduce it to the one AtomicBool. Daniel > > diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs > index 723c3244fc85..0622ecbfced5 100644 > --- a/rust/kernel/once_lite.rs > +++ b/rust/kernel/once_lite.rs > @@ -16,7 +16,7 @@ > //! > //! Reference: <https://doc.rust-lang.org/std/sync/struct.Once.html> > > -use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; > +use core::sync::atomic::{AtomicU32, Ordering::Acquire, Ordering::Relaxed}; > > /// A low-level synchronization primitive for one-time global execution. > /// > @@ -44,13 +44,13 @@ > /// assert_eq!(x, 42); > /// ``` > /// > -pub struct OnceLite(AtomicBool, AtomicBool); > +pub struct OnceLite(AtomicU32); > > impl OnceLite { > /// Creates a new `OnceLite` value. > #[inline(always)] > pub const fn new() -> Self { > - Self(AtomicBool::new(false), AtomicBool::new(false)) > + Self(AtomicU32::new(0)) > } > > /// Performs an initialization routine once and only once. The given > @@ -71,10 +71,10 @@ pub const fn new() -> Self { > /// [`DO_ONCE_LITE_IF`]: srctree/include/once_lite.h > #[inline(always)] > pub fn call_once<F: FnOnce()>(&self, f: F) { > - if !self.0.load(Relaxed) && !self.0.swap(true, Relaxed) { > - f() > + if self.0.load(Relaxed) == 0 && self.0.compare_exchange(0, 1, Acquire, Relaxed) == Ok(0) { > + f(); > + self.0.store(2, Relaxed); > }; > - self.1.store(true, Relaxed); > } > > /// Returns `true` if some `call_once` call has completed successfully. > @@ -98,7 +98,7 @@ pub fn call_once<F: FnOnce()>(&self, f: F) { > /// ``` > #[inline(always)] > pub fn is_completed(&self) -> bool { > - self.1.load(Relaxed) > + self.0.load(Relaxed) > 1 > } > } > >> The `rust` part should be default value for rustdoc tests, can we please >> omit that? > > Will do! > > Jens ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay 2024-11-26 18:57 ` Daniel Sedlak @ 2024-11-26 19:00 ` Miguel Ojeda 2024-11-27 12:26 ` Alice Ryhl 2025-02-04 11:11 ` Andreas Hindborg 3 siblings, 0 replies; 27+ messages in thread From: Miguel Ojeda @ 2024-11-26 19:00 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel On Tue, Nov 26, 2024 at 5:41 PM Jens Korinth via B4 Relay <devnull+jens.korinth.tuta.io@kernel.org> wrote: > > Similar to `Once` in Rust's standard library, but with the same > non-blocking behavior as the kernel's `DO_ONCE_LITE` macro. Abstraction > allows easy replacement of the underlying sync mechanisms, see > > https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/. Nit: you could use a Link tag for this, e.g. Link: https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/ [1] And then you can refer to it using [1], like "see [1].". > +//! This primitive is meant to be used to execute code only once. It is > +//! similar in design to Rust's > +//! [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html), In one case you used a shortcut reference link for this -- perhaps you could do that here and below. > +//! but borrowing the non-blocking mechanism used in the kernel's > +//! [`DO_ONCE_LITE`] macro. I would suggest mentioning C here, e.g. C macro, to reduce ambiguity (it is true we have just used "kernel's" in some cases). > +//! An example use case would be to print a message only the first time. Ideally we would mention one or two concrete use cases here, e.g. you could grep to see a couple common C use cases. Also, since we are going to have the `pr_..._once` macros, it may not be the best example use case, since the developer would use those instead, right? The docs look well-formatted etc., so thanks for taking the time writing them :) > +/// A low-level synchronization primitive for one-time global execution. I wonder if we should move part of the docs from the module level here to avoid duplication. > +/// macro. The Rust macro `do_once_lite` replacing it uses `OnceLite` Please link these with intra-doc links. > +/// ```rust You can remove `rust` from this one, like in the others. > +/// static START: kernel::once_lite::OnceLite = > +/// kernel::once_lite::OnceLite::new(); I would have a hidden `use` line to simplify the example -- since we are reading the example about this item, it is OK to shorten it here, e.g. static START: OnceLite = OnceLite::new(); > +/// // run initialization here Please use the usual comment style: "Run initialization here.". > +/// while !START.is_completed() { /* busy wait */ } Hmm... without threads this looks a bit strange. > + /// Creates a new `OnceLite` value. Please use intra-doc links wherever possible. > + /// This method will _not_ block the calling thread if another Should we save "does _not_ ... regardless ..."? i.e. it never blocks. > + /// [`std::sync::Once`], but identical to the way the implementation of > + /// the kernel's [`DO_ONCE_LITE_IF`] macro is behaving at the time of > + /// writing. "at the time of writing" is implicit, so we don't need to say it. (Of course, it would be nice to have someone making sure we don't get out of sync! > +/// Executes code only once. "an expression" or "a Rust expression" could perhaps be more precise and hints what the argument is (which may help those not accustomed to macro signatures). > +/// kernel::do_once_lite!((||{x = 42;})()); Please format the examples if possible. Not a big deal, but it is always nicer. Can we add an `assert` here too like in the other examples, so that this doubles as a test? By the way, does this need to be an immediately called closure? i.e. the macro takes an expression, can't we do e.g. do_once_lite!(x = 42); ? > + #[link_section = ".data.once"] > + static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); I let Boqun et al. review the sync parts, but a few questions: Do we want/need two `AtomicBool`s? Should the docs for `OnceLite` mention/suggest `link_section`? Should we have a macro to declare them instead? By the way, please test with `CLIPPY=1`, I got `new_without_default`. Cheers, Miguel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay 2024-11-26 18:57 ` Daniel Sedlak 2024-11-26 19:00 ` Miguel Ojeda @ 2024-11-27 12:26 ` Alice Ryhl 2024-11-27 20:12 ` jens.korinth 2025-02-04 11:11 ` Andreas Hindborg 3 siblings, 1 reply; 27+ messages in thread From: Alice Ryhl @ 2024-11-27 12:26 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Trevor Gross, rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel On Tue, Nov 26, 2024 at 5:41 PM Jens Korinth via B4 Relay <devnull+jens.korinth.tuta.io@kernel.org> wrote: > > From: Jens Korinth <jens.korinth@tuta.io> > > Similar to `Once` in Rust's standard library, but with the same > non-blocking behavior as the kernel's `DO_ONCE_LITE` macro. Abstraction > allows easy replacement of the underlying sync mechanisms, see > > https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/. > > Suggested-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Jens Korinth <jens.korinth@tuta.io> > + pub fn is_completed(&self) -> bool { > + self.1.load(Relaxed) > + } What is the use-case for this function? Why not just have one atomic? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-27 12:26 ` Alice Ryhl @ 2024-11-27 20:12 ` jens.korinth 2024-11-27 20:18 ` Alice Ryhl 0 siblings, 1 reply; 27+ messages in thread From: jens.korinth @ 2024-11-27 20:12 UTC (permalink / raw) To: Alice Ryhl Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel > What is the use-case for this function? `DO_ONCE_LITE` has very few uses in C; frankly, the only other use I could think of was initialization. But since `OnceLite` cannot block or guarantee visibility of the side-effects of the `call_once` expression in other threads, it can't be used for this case, either. _Unless_ there is some mechanism to wait voluntarily when this is required, hence `is_completed`. (And it also exists in `std::sync::Once`.) `DO_ONCE_LITE_IF` has more uses in C, but this is a bit harder to do well with `OnceLite`: A `do_once_lite_if` Rust macro can't short-circuit the condition to avoid the evaluation if the atomic load already shows that it has been done / is being done rn. Maybe a `pub fn call_once<C: FnOnce() -> bool, F: FnOnce()>(cond: C, f: F)` could be used to simulate the effect. Thoughts? > Why not just have one atomic? Do you also have an `AtomicU32` state var in mind, as Daniel suggested? Jens ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-27 20:12 ` jens.korinth @ 2024-11-27 20:18 ` Alice Ryhl 2024-12-05 6:49 ` jens.korinth 0 siblings, 1 reply; 27+ messages in thread From: Alice Ryhl @ 2024-11-27 20:18 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Wed, Nov 27, 2024 at 9:12 PM <jens.korinth@tuta.io> wrote: > > > What is the use-case for this function? > > `DO_ONCE_LITE` has very few uses in C; frankly, the only other use I could think > of was initialization. But since `OnceLite` cannot block or guarantee visibility > of the side-effects of the `call_once` expression in other threads, it can't be > used for this case, either. _Unless_ there is some mechanism to wait > voluntarily when this is required, hence `is_completed`. (And it also exists in > `std::sync::Once`.) > > `DO_ONCE_LITE_IF` has more uses in C, but this is a bit harder to do well with > `OnceLite`: A `do_once_lite_if` Rust macro can't short-circuit the condition to > avoid the evaluation if the atomic load already shows that it has been done / is > being done rn. Maybe a > `pub fn call_once<C: FnOnce() -> bool, F: FnOnce()>(cond: C, f: F)` could be > used to simulate the effect. Thoughts? > > > Why not just have one atomic? > > Do you also have an `AtomicU32` state var in mind, as Daniel suggested? What I had in mind was to use a single AtomicBool and get rid of the `is_completed` logic entirely. The purpose is to use this for printing once, and printing doesn't need `is_completed`. We can have another helper for other purposes. Alice ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-27 20:18 ` Alice Ryhl @ 2024-12-05 6:49 ` jens.korinth 2024-12-05 8:47 ` Alice Ryhl 0 siblings, 1 reply; 27+ messages in thread From: jens.korinth @ 2024-12-05 6:49 UTC (permalink / raw) To: Alice Ryhl Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel I'm afraid you lost me. You wrote: > Using a Once type for this seems like a good idea to me. and > One advantage of using a Once type is that we can use core::sync::atomic > until we have working LKMM atomics and then we just swap out the Once > type without having to modify the warn_once abstractions. That made sense to me, so I started in this direction. `std::sync::Once` has `is_completed` [1], which made particular sense to implement in my mind to increase the utility of `OnceLite`. [1]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed > The purpose is to use this for printing once, and printing doesn't need > `is_completed`. We can have another helper for other purposes. Why have `OnceLite` then at all, instead of the hidden Rust macro that was proposed initially? Jens ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-12-05 6:49 ` jens.korinth @ 2024-12-05 8:47 ` Alice Ryhl 0 siblings, 0 replies; 27+ messages in thread From: Alice Ryhl @ 2024-12-05 8:47 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Thu, Dec 5, 2024 at 7:49 AM <jens.korinth@tuta.io> wrote: > > I'm afraid you lost me. You wrote: > > > Using a Once type for this seems like a good idea to me. > > and > > > One advantage of using a Once type is that we can use core::sync::atomic > > until we have working LKMM atomics and then we just swap out the Once > > type without having to modify the warn_once abstractions. > > That made sense to me, so I started in this direction. `std::sync::Once` > has `is_completed` [1], which made particular sense to implement in my > mind to increase the utility of `OnceLite`. > > [1]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed The stdlib Once type guarantees that when call_once has returned, then the closure has finished running *even* if you are not the caller who is running the closure. It achieves this by having other callers go to sleep until the main caller completes. If we actually want to mirror Once, then we should have that logic too, and I really don't think we want such complicated logic in our pr_warn_once macro. > > The purpose is to use this for printing once, and printing doesn't need > > `is_completed`. We can have another helper for other purposes. > > Why have `OnceLite` then at all, instead of the hidden Rust macro that was > proposed initially? Well, one advantage is that when Boqun manages to add support for LKMM atomics, we can switch them out without having to modify macro code. Moving logic out of macro code is usually a good idea. It's also possible that there are other user-cases of an OnceLite that doesn't have is_completed. Alice ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay ` (2 preceding siblings ...) 2024-11-27 12:26 ` Alice Ryhl @ 2025-02-04 11:11 ` Andreas Hindborg 3 siblings, 0 replies; 27+ messages in thread From: Andreas Hindborg @ 2025-02-04 11:11 UTC (permalink / raw) To: Jens Korinth via B4 Relay Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, jens.korinth, rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel "Jens Korinth via B4 Relay" <devnull+jens.korinth.tuta.io@kernel.org> writes: > From: Jens Korinth <jens.korinth@tuta.io> > > Similar to `Once` in Rust's standard library, but with the same > non-blocking behavior as the kernel's `DO_ONCE_LITE` macro. Abstraction > allows easy replacement of the underlying sync mechanisms, see > > https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/. > > Suggested-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Jens Korinth <jens.korinth@tuta.io> Thanks for the patch! I was using the series for `pr_warn_once!` elsewhere, and clippy gave me this suggestion: CLIPPY L rust/kernel.o error: you should consider adding a `Default` implementation for `OnceLite` --> /home/aeh/src/linux-rust/module-params/rust/kernel/once_lite.rs:52:5 | 52 | / pub const fn new() -> Self { 53 | | Self(AtomicBool::new(false), AtomicBool::new(false)) 54 | | } | |_____^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default = note: `-D clippy::new-without-default` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::new_without_default)]` help: try adding this | 49 + impl Default for OnceLite { 50 + fn default() -> Self { 51 + Self::new() 52 + } 53 + } | Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 2/3] rust: print: Add pr_*_once macros 2024-11-26 16:40 ` [PATCH v4 0/3] rust: Add pr_*_once macros Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay @ 2024-11-26 16:40 ` Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once Jens Korinth via B4 Relay ` (2 subsequent siblings) 4 siblings, 0 replies; 27+ messages in thread From: Jens Korinth via B4 Relay @ 2024-11-26 16:40 UTC (permalink / raw) To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross Cc: rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel, Jens Korinth From: FUJITA Tomonori <fujita.tomonori@gmail.com> Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once functions, which print a message only once. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Signed-off-by: Jens Korinth <jens.korinth@tuta.io> --- rust/kernel/print.rs | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index a28077a7cb301193dcca293befb096b2dd153202..f354b32684a2b3c6c5d9aca6ef9f68e52c870bcf 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -414,3 +414,129 @@ 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. +/// +/// [`pr_emerg_once`]: srctree/include/linux/printk.h +/// +/// # Examples +/// +/// ``` +/// kernel::pr_emerg_once!("hello {}\n", "there"); +/// ``` +#[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. +/// +/// [`pr_alert_once`]: srctree/include/linux/printk.h +/// +/// # Examples +/// +/// ``` +/// kernel::pr_alert_once!("hello {}\n", "there"); +/// ``` +#[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. +/// +/// [`pr_crit_once`]: srctree/include/linux/printk.h +/// +/// # Examples +/// +/// ``` +/// kernel::pr_crit_once!("hello {}\n", "there"); +/// ``` +#[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. +/// +/// # Examples +/// +/// [`pr_err_once`]: srctree/include/linux/printk.h +/// +/// ``` +/// kernel::pr_err_once!("hello {}\n", "there"); +/// ``` +#[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. +/// +/// [`pr_warn_once`]: srctree/include/linux/printk.h +/// +/// # Examples +/// +/// ``` +/// kernel::pr_warn_once!("hello {}\n", "there"); +/// ``` +#[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. +/// +/// [`pr_notice_once`]: srctree/include/linux/printk.h +/// +/// # Examples +/// +/// ``` +/// kernel::pr_notice_once!("hello {}\n", "there"); +/// ``` +#[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. +/// +/// [`pr_info_once`]: srctree/include/linux/printk.h +/// +/// # Examples +/// +/// ``` +/// kernel::pr_info_once!("hello {}\n", "there"); +/// ``` +#[macro_export] +macro_rules! pr_info_once ( + ($($arg:tt)*) => ( + $crate::do_once_lite!($crate::pr_info!($($arg)*)) + ) +); -- 2.47.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-11-26 16:40 ` [PATCH v4 0/3] rust: Add pr_*_once macros Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 2/3] rust: print: Add pr_*_once macros Jens Korinth via B4 Relay @ 2024-11-26 16:40 ` Jens Korinth via B4 Relay 2024-11-26 17:07 ` Miguel Ojeda 2024-11-26 16:59 ` [PATCH v4 0/3] rust: Add pr_*_once macros Miguel Ojeda 2025-02-11 15:04 ` Andreas Hindborg 4 siblings, 1 reply; 27+ messages in thread From: Jens Korinth via B4 Relay @ 2024-11-26 16:40 UTC (permalink / raw) To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross Cc: rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel, Jens Korinth From: Jens Korinth <jens.korinth@tuta.io> Use new pr_warn_once macro to resolve TODO in error.rs. Signed-off-by: Jens Korinth <jens.korinth@tuta.io> --- rust/kernel/error.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 52c5024324474fc1306047f3fd7516f0023d0313..f6813dace1128b7ef91f64e79cd83bb64995bf97 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -102,8 +102,7 @@ impl Error { /// be returned in such a case. pub fn from_errno(errno: crate::ffi::c_int) -> Error { if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 { - // TODO: Make it a `WARN_ONCE` once available. - crate::pr_warn!( + crate::pr_warn_once!( "attempted to create `Error` with out of range `errno`: {}", errno ); -- 2.47.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-11-26 16:40 ` [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once Jens Korinth via B4 Relay @ 2024-11-26 17:07 ` Miguel Ojeda 2024-11-27 20:39 ` jens.korinth 0 siblings, 1 reply; 27+ messages in thread From: Miguel Ojeda @ 2024-11-26 17:07 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel On Tue, Nov 26, 2024 at 5:41 PM Jens Korinth via B4 Relay <devnull+jens.korinth.tuta.io@kernel.org> wrote: > > Use new pr_warn_once macro to resolve TODO in error.rs. Thanks for keeping the work on this! I would mention here the merits between `pr_warn_once` vs. `WARN_ONCE` and why the former was picked in this patch (especially since the `TODO` suggests the latter). Cheers, Miguel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-11-26 17:07 ` Miguel Ojeda @ 2024-11-27 20:39 ` jens.korinth 2024-11-30 18:18 ` Miguel Ojeda 0 siblings, 1 reply; 27+ messages in thread From: jens.korinth @ 2024-11-27 20:39 UTC (permalink / raw) To: Miguel Ojeda Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel > I would mention here the merits between `pr_warn_once` vs. `WARN_ONCE` > and why the former was picked in this patch (especially since the > `TODO` suggests the latter). Tbh, I am not 100% whether this should be here at all. The bug is not here, it's at the call site. It should probably be a `try_from` instead, to raise the error there? Jens ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-11-27 20:39 ` jens.korinth @ 2024-11-30 18:18 ` Miguel Ojeda 2024-12-05 6:30 ` jens.korinth 0 siblings, 1 reply; 27+ messages in thread From: Miguel Ojeda @ 2024-11-30 18:18 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Wed, Nov 27, 2024 at 9:39 PM <jens.korinth@tuta.io> wrote: > > Tbh, I am not 100% whether this should be here at all. The bug is not here, it's > at the call site. It should probably be a `try_from` instead, to raise the error > there? Do you mean removing the function altogether? i.e. migrating all callers to `try_from_errno`? Cheers, Miguel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-11-30 18:18 ` Miguel Ojeda @ 2024-12-05 6:30 ` jens.korinth 2024-12-05 11:55 ` Miguel Ojeda 0 siblings, 1 reply; 27+ messages in thread From: jens.korinth @ 2024-12-05 6:30 UTC (permalink / raw) To: Miguel Ojeda Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel Sorry for the late response, the usual madness at the end of the year is setting in. > Do you mean removing the function altogether? i.e. migrating all > callers to `try_from_errno`? I think it should be `TryFrom`. The `std::From` doc [1] says: Note: This trait must not fail. The From trait is intended for perfect conversions. If the conversion can fail or is not perfect, use TryFrom. [1]: https://doc.rust-lang.org/std/convert/trait.From.html Jens ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-12-05 6:30 ` jens.korinth @ 2024-12-05 11:55 ` Miguel Ojeda 2024-12-05 19:22 ` jens.korinth 0 siblings, 1 reply; 27+ messages in thread From: Miguel Ojeda @ 2024-12-05 11:55 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Thu, Dec 5, 2024 at 7:30 AM <jens.korinth@tuta.io> wrote: > > Sorry for the late response, the usual madness at the end of the year is > setting in. No worries at all! I know the feeling... :) > I think it should be `TryFrom`. The `std::From` doc [1] says: > > Note: This trait must not fail. The From trait is intended for perfect > conversions. If the conversion can fail or is not perfect, use TryFrom. > > [1]: https://doc.rust-lang.org/std/convert/trait.From.html Sorry, I am confused. This is not implementing the `From` trait, it is an inherent implementation. If what you mean is that this should not be an infallible operation, then we are back at my previous reply, i.e. are you suggesting to remove the method? Could you please clarify, perhaps with an example? Or are you talking about something completely different? Thanks! Cheers, Miguel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once 2024-12-05 11:55 ` Miguel Ojeda @ 2024-12-05 19:22 ` jens.korinth 0 siblings, 0 replies; 27+ messages in thread From: jens.korinth @ 2024-12-05 19:22 UTC (permalink / raw) To: Miguel Ojeda Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel > Sorry, I am confused. This is not implementing the `From` trait, it is > an inherent implementation. Hmm, you're right. I assume there is a reason why it doesn't implement `From<c_int>` or `TryFrom<c_int>`? > If what you mean is that this should not be an infallible operation, > then we are back at my previous reply, i.e. are you suggesting to > remove the method? Could you please clarify, perhaps with an example? Yes, I meant to say it should not be infallible, because, well, it isn't. :) I'd probably make `try_from_errno` public and remove `from_errno`. If there's no other disadvantage I'd remove `try_from_errno` as well and replace it by `TryFrom<c_int>`. Implementing `From<Error> for c_int` may also make sense for the other direction? Jens ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros 2024-11-26 16:40 ` [PATCH v4 0/3] rust: Add pr_*_once macros Jens Korinth via B4 Relay ` (2 preceding siblings ...) 2024-11-26 16:40 ` [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once Jens Korinth via B4 Relay @ 2024-11-26 16:59 ` Miguel Ojeda 2025-02-11 15:04 ` Andreas Hindborg 4 siblings, 0 replies; 27+ messages in thread From: Miguel Ojeda @ 2024-11-26 16:59 UTC (permalink / raw) To: jens.korinth Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel On Tue, Nov 26, 2024 at 5:41 PM Jens Korinth via B4 Relay <devnull+jens.korinth.tuta.io@kernel.org> wrote: > > Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Jens Korinth <jens.korinth@tuta.io> I guess these tags are meant to be informative, but just in case, I thought I would ask if they are here for some other reason, i.e. if you somehow want them to end in the kernel log. (It may be less confusing to simply mention in the letter who worked on what.) Cheers, Miguel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros 2024-11-26 16:40 ` [PATCH v4 0/3] rust: Add pr_*_once macros Jens Korinth via B4 Relay ` (3 preceding siblings ...) 2024-11-26 16:59 ` [PATCH v4 0/3] rust: Add pr_*_once macros Miguel Ojeda @ 2025-02-11 15:04 ` Andreas Hindborg 2025-02-11 15:29 ` jens.korinth 4 siblings, 1 reply; 27+ messages in thread From: Andreas Hindborg @ 2025-02-11 15:04 UTC (permalink / raw) To: Jens Korinth via B4 Relay Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, jens.korinth, rust-for-linux, FUJITA Tomonori, Dirk Behme, linux-kernel Hi Jens, "Jens Korinth via B4 Relay" <devnull+jens.korinth.tuta.io@kernel.org> writes: > Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once > functions, which print a message only once. > > Introduces a `OnceLite` abstraction similar to Rust's > [`std::sync::Once`](https://doc.rust-lang.org/std/sync/struct.Once.html) > but using the non-blocking mechanism from the kernel's `DO_ONCE_LITE` > macro, which is used to define the `do_once_lite` Rust macro. > > First use case are an implementation of `pr_*_once` message macros to > print a message only once. > Thanks for the patch! Do you plan on sending a new version? If not, do you mind if I send v5? Best regards, Andreas Hindborg ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros 2025-02-11 15:04 ` Andreas Hindborg @ 2025-02-11 15:29 ` jens.korinth 2025-02-11 15:42 ` Andreas Hindborg 0 siblings, 1 reply; 27+ messages in thread From: jens.korinth @ 2025-02-11 15:29 UTC (permalink / raw) To: Andreas Hindborg Cc: Jens Korinth via B4 Relay, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel Hi! > Thanks for the patch! Do you plan on sending a new version? If not, do you mind if I send v5? I think there is currently no consensus on how exactly it should be done (or at least I was confused about that). If you’re actively using the patch please go ahead! Active usage is always the best argument in such cases. Jens 11 Feb 2025 at 16:05 by a.hindborg@kernel.org: > Hi Jens, > > "Jens Korinth via B4 Relay" <devnull+jens.korinth.tuta.io@kernel.org> writes: > >> Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once >> functions, which print a message only once. >> >> Introduces a `OnceLite` abstraction similar to Rust's >> [`std::sync::Once`](https://doc.rust-lang.org/std/sync/struct.Once.html) >> but using the non-blocking mechanism from the kernel's `DO_ONCE_LITE` >> macro, which is used to define the `do_once_lite` Rust macro. >> >> First use case are an implementation of `pr_*_once` message macros to >> print a message only once. >> > > Thanks for the patch! Do you plan on sending a new version? If not, do > you mind if I send v5? > > > Best regards, > Andreas Hindborg > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros 2025-02-11 15:29 ` jens.korinth @ 2025-02-11 15:42 ` Andreas Hindborg 2025-07-17 16:07 ` [PATCH v4 0/3] rust: Add pr_*_once macros134 Onur Özkan 0 siblings, 1 reply; 27+ messages in thread From: Andreas Hindborg @ 2025-02-11 15:42 UTC (permalink / raw) To: jens.korinth Cc: Jens Korinth via B4 Relay, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel <jens.korinth@tuta.io> writes: > Hi! > >> Thanks for the patch! Do you plan on sending a new version? If not, do you mind if I send v5? > > I think there is currently no consensus on how exactly it should be done (or at least I was confused about that). If you’re actively using the patch please go ahead! Active usage is always the best argument in such cases. I was informed this patch set is the correct way to emit a warning in the module_params patch set [1]. I did not follow all the discussion so I am not sure either. But I'll look into the discussion then. Best regards, Andreas Hindborg [1] https://lore.kernel.org/rust-for-linux/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros134 2025-02-11 15:42 ` Andreas Hindborg @ 2025-07-17 16:07 ` Onur Özkan 2025-07-19 3:33 ` Boqun Feng 0 siblings, 1 reply; 27+ messages in thread From: Onur Özkan @ 2025-07-17 16:07 UTC (permalink / raw) To: Andreas Hindborg Cc: jens.korinth, Jens Korinth via B4 Relay, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Tue, 11 Feb 2025 16:42:25 +0100 Andreas Hindborg <a.hindborg@kernel.org> wrote: > <jens.korinth@tuta.io> writes: > > > Hi! > > > >> Thanks for the patch! Do you plan on sending a new version? If > >>not, do you mind if I send v5? > > > > I think there is currently no consensus on how exactly it should be > > done (or at least I was confused about that). If you’re actively > > using the patch please go ahead! Active usage is always the best > > argument in such cases. > > I was informed this patch set is the correct way to emit a warning in > the module_params patch set [1]. > > I did not follow all the discussion so I am not sure either. But I'll > look into the discussion then. > > > Best regards, > Andreas Hindborg > > > > [1] > https://lore.kernel.org/rust-for-linux/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org/ > > I have reviewed the patch series from start to finish. I am not using or depending the patch actively but I can send v5 sometime soon (I will have some space next week) if you would like. Regards, Onur ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros134 2025-07-17 16:07 ` [PATCH v4 0/3] rust: Add pr_*_once macros134 Onur Özkan @ 2025-07-19 3:33 ` Boqun Feng 2025-07-19 16:33 ` Onur 0 siblings, 1 reply; 27+ messages in thread From: Boqun Feng @ 2025-07-19 3:33 UTC (permalink / raw) To: Onur Özkan Cc: Andreas Hindborg, jens.korinth, Jens Korinth via B4 Relay, Miguel Ojeda, Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Thu, Jul 17, 2025 at 07:07:13PM +0300, Onur Özkan wrote: > On Tue, 11 Feb 2025 16:42:25 +0100 > Andreas Hindborg <a.hindborg@kernel.org> wrote: > > > <jens.korinth@tuta.io> writes: > > > > > Hi! > > > > > >> Thanks for the patch! Do you plan on sending a new version? If > > >>not, do you mind if I send v5? > > > > > > I think there is currently no consensus on how exactly it should be > > > done (or at least I was confused about that). If you´re actively > > > using the patch please go ahead! Active usage is always the best > > > argument in such cases. > > > > I was informed this patch set is the correct way to emit a warning in > > the module_params patch set [1]. > > > > I did not follow all the discussion so I am not sure either. But I'll > > look into the discussion then. > > > > > > Best regards, > > Andreas Hindborg > > > > > > > > [1] > > https://lore.kernel.org/rust-for-linux/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org/ > > > > > > I have reviewed the patch series from start to finish. I am not > using or depending the patch actively but I can send v5 sometime > soon (I will have some space next week) if you would like. > Note that you need to use LKMM atomics [1], and you should really use a 32bit atomic at the beginning. Thanks! There are a few explanations on why we want to avoid use Rust native atomics: https://lwn.net/Articles/993785/ https://lore.kernel.org/rust-for-linux/CAHk-=whkQk=zq5XiMcaU3xj4v69+jyoP-y6Sywhq-TvxSSvfEA@mail.gmail.com/ [1]: https://lore.kernel.org/rust-for-linux/20250719030827.61357-1-boqun.feng@gmail.com/ Regards, Boqun > Regards, > Onur ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] rust: Add pr_*_once macros134 2025-07-19 3:33 ` Boqun Feng @ 2025-07-19 16:33 ` Onur 0 siblings, 0 replies; 27+ messages in thread From: Onur @ 2025-07-19 16:33 UTC (permalink / raw) To: Boqun Feng Cc: Andreas Hindborg, jens.korinth, Jens Korinth via B4 Relay, Miguel Ojeda, Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross, Rust For Linux, FUJITA Tomonori, Dirk Behme, Linux Kernel On Fri, 18 Jul 2025 20:33:21 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > > > > I have reviewed the patch series from start to finish. I am not > > using or depending the patch actively but I can send v5 sometime > > soon (I will have some space next week) if you would like. > > > > Note that you need to use LKMM atomics [1], and you should really use > a 32bit atomic at the beginning. Thanks! > > There are a few explanations on why we want to avoid use Rust native > atomics: > > https://lwn.net/Articles/993785/ > https://lore.kernel.org/rust-for-linux/CAHk-=whkQk=zq5XiMcaU3xj4v69+jyoP-y6Sywhq-TvxSSvfEA@mail.gmail.com/ > These are excellent sources for digging into the details. Thanks a lot! :) Regards, Onur ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2025-07-19 16:41 UTC | newest] Thread overview: 27+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <8gSHb0iuAT_Wz0rR1-qsnFIVndSpW229fA0lq-fslngTt5k1ooiMw5eAIc82F26Mdma4nkyU4X7_1RLZcnQf-Q==@protonmail.internalid> 2024-11-26 16:40 ` [PATCH v4 0/3] rust: Add pr_*_once macros Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 1/3] rust: Add `OnceLite` for executing code once Jens Korinth via B4 Relay 2024-11-26 18:57 ` Daniel Sedlak 2024-11-27 19:46 ` jens.korinth 2024-11-29 8:22 ` Daniel Sedlak 2024-11-26 19:00 ` Miguel Ojeda 2024-11-27 12:26 ` Alice Ryhl 2024-11-27 20:12 ` jens.korinth 2024-11-27 20:18 ` Alice Ryhl 2024-12-05 6:49 ` jens.korinth 2024-12-05 8:47 ` Alice Ryhl 2025-02-04 11:11 ` Andreas Hindborg 2024-11-26 16:40 ` [PATCH v4 2/3] rust: print: Add pr_*_once macros Jens Korinth via B4 Relay 2024-11-26 16:40 ` [PATCH v4 3/3] rust: error: Replace pr_warn by pr_warn_once Jens Korinth via B4 Relay 2024-11-26 17:07 ` Miguel Ojeda 2024-11-27 20:39 ` jens.korinth 2024-11-30 18:18 ` Miguel Ojeda 2024-12-05 6:30 ` jens.korinth 2024-12-05 11:55 ` Miguel Ojeda 2024-12-05 19:22 ` jens.korinth 2024-11-26 16:59 ` [PATCH v4 0/3] rust: Add pr_*_once macros Miguel Ojeda 2025-02-11 15:04 ` Andreas Hindborg 2025-02-11 15:29 ` jens.korinth 2025-02-11 15:42 ` Andreas Hindborg 2025-07-17 16:07 ` [PATCH v4 0/3] rust: Add pr_*_once macros134 Onur Özkan 2025-07-19 3:33 ` Boqun Feng 2025-07-19 16:33 ` Onur
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).