[PATCH v1] rust: Add pr_*_once macros

[PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

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>
---
 rust/kernel/print.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index a28077a7cb30..57dbb45ba5c0 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -414,3 +414,95 @@ macro_rules! pr_cont (
         $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*)
     )
 );
+
+/// Executes code only once if the condition is true.
+///
+/// Public but hidden since it should only be used from public macros.
+#[doc(hidden)]
+#[macro_export]
+macro_rules! do_once_lite_if (
+    ($condition:expr, $e:expr) => (
+        {
+            #[link_section = ".data.once"]
+            static ALREADY_DONE: core::sync::atomic::AtomicBool =
+                core::sync::atomic::AtomicBool::new(false);
+            let mut ret = false;
+
+            if $condition && !ALREADY_DONE.swap(true, core::sync::atomic::Ordering::Relaxed) {
+                $e;
+                ret = true;
+            }
+            ret
+        }
+    )
+);
+
+/// 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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $crate::pr_info!($($arg)*))
+    )
+);

base-commit: ae7851c29747fa3765ecb722fe722117a346f988
-- 
2.43.0

Re: [PATCH v1] rust: Add pr_*_once macros

[Dirk Behme]

On 03.11.24 04:05, FUJITA Tomonori wrote:
> Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once
> functions, which print a message only once.

Maybe it should be mentioned why this is added? Who will use this?

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/print.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> index a28077a7cb30..57dbb45ba5c0 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -414,3 +414,95 @@ macro_rules! pr_cont (
>          $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*)
>      )
>  );
> +
> +/// Executes code only once if the condition is true.
> +///
> +/// Public but hidden since it should only be used from public macros.
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! do_once_lite_if (
> +    ($condition:expr, $e:expr) => (
> +        {
> +            #[link_section = ".data.once"]
> +            static ALREADY_DONE: core::sync::atomic::AtomicBool =
> +                core::sync::atomic::AtomicBool::new(false);
> +            let mut ret = false;
> +
> +            if $condition && !ALREADY_DONE.swap(true, core::sync::atomic::Ordering::Relaxed) {

What is '$condition'  good for? It looks to me that it is set just to
'true' everywhere below? Could it be dropped?

> +                $e;
> +                ret = true;
> +            }
> +            ret
> +        }
> +    )
> +);
> +
> +/// Prints an emergency-level message (level 0) only once.
> +///
> +/// Equivalent to the kernel's [`pr_emerg_once`] macro.

Here and below:

Would it make sense to use a similar header style like with the
existing pr_*()? And add '# Examples'?

Best regards

Dirk


> +#[macro_export]
> +macro_rules! pr_emerg_once (
> +    ($($arg:tt)*) => (
> +        $crate::do_once_lite_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $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_if!(true, $crate::pr_info!($($arg)*))
> +    )
> +);
> 
> base-commit: ae7851c29747fa3765ecb722fe722117a346f988

Re: [PATCH v1] rust: Add pr_*_once macros

[Miguel Ojeda]

On Sun, Nov 3, 2024 at 4:05 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once
> functions, which print a message only once.

Hmm... Isn't this derived from what Jens was working on in Zulip? (Cc'd)

Thanks!

Cheers,
Miguel

Re: [PATCH v1] rust: Add pr_*_once macros

[jens.korinth]

> Hmm... Isn't this derived from what Jens was working on in Zulip? (Cc'd)
>
Looks very similar. But more importantly it's missing the hotpath improvement suggested by Boqun Feng, which improves the perfomance significantly:

@_**Boqun Feng|411647** [said](https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/warn_once.2Fdo_once_lite_if/near/478155223):
````quote
FWIW, you can do this:
```diff
diff --git a/benches/once_cmp.rs b/benches/once_cmp.rs
index 322e420..1bfc9cd 100644
--- a/benches/once_cmp.rs
+++ b/benches/once_cmp.rs
@@ -124,7 +124,7 @@ macro_rules! do_once_lite_if_abool_swap {
     ($cond:expr, $e:expr) => {
         #[link_section = ".data.once"]
         static ALREADY_DONE: AtomicBool = AtomicBool::new(false);
-        if $cond && !ALREADY_DONE.swap(true, Ordering::Relaxed) {
+        if $cond && !ALREADY_DONE.load(Ordering::Relaxed) && !ALREADY_DONE.swap(true, Ordering::Relaxed) {
             $e;
         }
     }
```
to improve the hotpath, because you don't need to xchg() each time after you set the flag.
````

Jens

Re: [PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

On Sun, 3 Nov 2024 17:38:10 +0100
Dirk Behme <dirk.behme@gmail.com> wrote:

> On 03.11.24 04:05, FUJITA Tomonori wrote:
>> Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once
>> functions, which print a message only once.
> 
> Maybe it should be mentioned why this is added? Who will use this?

I'm thinking about using this for my timekeeping and timer patchset:

https://lore.kernel.org/lkml/87cyjj2vi1.ffs@tglx/

pr_warn_once() is mentioned in core-api/printk-index.rst and
process/coding-style.rst so it's kinda API that we had better to
support even without the users?

I think that WARN_ON_ONCE (or WARN_ONCE) are better for the patchset
but they need assembly code and it would take time to implement them.

>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  rust/kernel/print.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 92 insertions(+)
>> 
>> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
>> index a28077a7cb30..57dbb45ba5c0 100644
>> --- a/rust/kernel/print.rs
>> +++ b/rust/kernel/print.rs
>> @@ -414,3 +414,95 @@ macro_rules! pr_cont (
>>          $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*)
>>      )
>>  );
>> +
>> +/// Executes code only once if the condition is true.
>> +///
>> +/// Public but hidden since it should only be used from public macros.
>> +#[doc(hidden)]
>> +#[macro_export]
>> +macro_rules! do_once_lite_if (
>> +    ($condition:expr, $e:expr) => (
>> +        {
>> +            #[link_section = ".data.once"]
>> +            static ALREADY_DONE: core::sync::atomic::AtomicBool =
>> +                core::sync::atomic::AtomicBool::new(false);
>> +            let mut ret = false;
>> +
>> +            if $condition && !ALREADY_DONE.swap(true, core::sync::atomic::Ordering::Relaxed) {
> 
> What is '$condition'  good for? It looks to me that it is set just to
> 'true' everywhere below? Could it be dropped?

$condition is supposed to be used by WARN_ONCE() in the future but I
could drop for now.

>> +                $e;
>> +                ret = true;
>> +            }
>> +            ret
>> +        }
>> +    )
>> +);
>> +
>> +/// Prints an emergency-level message (level 0) only once.
>> +///
>> +/// Equivalent to the kernel's [`pr_emerg_once`] macro.
> 
> Here and below:
> 
> Would it make sense to use a similar header style like with the
> existing pr_*()? And add '# Examples'?

You meant that having the similar comment like the following?

/// Prints an info-level message (level 6) only once.
///
/// Use this level for informational messages.
///
/// Equivalent to the kernel's [`pr_info_once`] macro.
///
/// Mimics the interface of [`std::print!`]. See [`core::fmt`] and
/// `alloc::format!` for information about the formatting syntax.
///
/// # Examples
///
/// ```
/// pr_info_once!("hello {}\n", "there");
/// ```

It's verbose to duplicate the comments in pr_*?

Re: [PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

On Sun, 3 Nov 2024 17:41:54 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sun, Nov 3, 2024 at 4:05 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Add Rust version of pr_[emerg|alert|crit|err|warn|notic|info]_once
>> functions, which print a message only once.
> 
> Hmm... Isn't this derived from what Jens was working on in Zulip? (Cc'd)

The code to execute only once is very basic code like the kind found
in textbooks? I think that his focus is on optimizing that code. Once
the the basic code is merged, it's easier to send patches to optimize
it.

Jens, if you prefer, I'll split this patch; make you the author of
the do_once code.

Re: [PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

On Sun, 3 Nov 2024 23:29:29 +0100 (CET)
jens.korinth@tuta.io wrote:

>> Hmm... Isn't this derived from what Jens was working on in Zulip? (Cc'd)
>>
> Looks very similar. But more importantly it's missing the hotpath
> improvement suggested by Boqun Feng, which improves the perfomance
> significantly:

I think that it's better to start with the very basic code and then
optimize it with commit log that includes benchmark results.

Re: [PATCH v1] rust: Add pr_*_once macros

[Boqun Feng]

On Mon, Nov 04, 2024 at 11:15:12AM +0900, FUJITA Tomonori wrote:
> On Sun, 3 Nov 2024 23:29:29 +0100 (CET)
> jens.korinth@tuta.io wrote:
> 
> >> Hmm... Isn't this derived from what Jens was working on in Zulip? (Cc'd)
> >>
> > Looks very similar. But more importantly it's missing the hotpath
> > improvement suggested by Boqun Feng, which improves the perfomance
> > significantly:
> 
> I think that it's better to start with the very basic code and then
> optimize it with commit log that includes benchmark results.

I don't think that is necessary, if you look at how DO_ONCE_LITE_IF is
implemented today in C, it already has the load for checking:

	if (unlikely(__ret_cond && !__already_done)) {

you will just need to use atomics to fix the data race issues in C, and
the xchg() to fix the "not exactly once" issue.

Regards,
Boqun

Re: [PATCH v1] rust: Add pr_*_once macros

[Miguel Ojeda]

On Mon, Nov 4, 2024 at 3:08 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> The code to execute only once is very basic code like the kind found
> in textbooks? I think that his focus is on optimizing that code. Once
> the the basic code is merged, it's easier to send patches to optimize
> it.

It doesn't matter that it is "very basic code". This was discussed 2
weeks ago in Zulip. Most importantly, it was a new contributor to the
kernel that was working on it (as far as I know).

The issue is that sending code without any indication about the
discussion, without Cc'ing the author, and so on, is the sort of thing
that can annoy people. And if they are new contributors, it can drive
them away.

We need to be careful about this.

Cheers,
Miguel

Re: [PATCH v1] rust: Add pr_*_once macros

[jens.korinth]

Nov 4, 2024, 16:45 by miguel.ojeda.sandonis@gmail.com:

> On Mon, Nov 4, 2024 at 3:08 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>
>>
>> The code to execute only once is very basic code like the kind found
>> in textbooks? I think that his focus is on optimizing that code. Once
>> the the basic code is merged, it's easier to send patches to optimize
>> it.
>>
>
> It doesn't matter that it is "very basic code". This was discussed 2
> weeks ago in Zulip. Most importantly, it was a new contributor to the
> kernel that was working on it (as far as I know).
>
> The issue is that sending code without any indication about the
> discussion, without Cc'ing the author, and so on, is the sort of thing
> that can annoy people. And if they are new contributors, it can drive
> them away.
>
> We need to be careful about this.
>
I think you both make good points. Fujita, you're right - it is very basic code with little technical complexity, so it may not be worth overthinking. At the same time, I really appreciate you including me in this conversation and acknowledging my contribution, Miguel; communities can be quite sensitive, and sometimes, attention to the small details really does make a difference. Perhaps we could simply add a "Suggested-by" or "Co-developed-by" acknowledgment and move forward?

FWIW, my motivation to look into it was the TODO kernel/error.rs:123. Might be a good first place to use the new macros.

Jens

Re: [PATCH v1] rust: Add pr_*_once macros

[Boqun Feng]

On Mon, Nov 04, 2024 at 11:08:21PM +0100, jens.korinth@tuta.io wrote:
> Nov 4, 2024, 16:45 by miguel.ojeda.sandonis@gmail.com:
> 
> > On Mon, Nov 4, 2024 at 3:08 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >
> >>
> >> The code to execute only once is very basic code like the kind found
> >> in textbooks? I think that his focus is on optimizing that code. Once
> >> the the basic code is merged, it's easier to send patches to optimize
> >> it.
> >>
> >
> > It doesn't matter that it is "very basic code". This was discussed 2
> > weeks ago in Zulip. Most importantly, it was a new contributor to the
> > kernel that was working on it (as far as I know).
> >
> > The issue is that sending code without any indication about the
> > discussion, without Cc'ing the author, and so on, is the sort of thing
> > that can annoy people. And if they are new contributors, it can drive
> > them away.
> >
> > We need to be careful about this.
> >
> I think you both make good points. Fujita, you're right - it is very
> basic code with little technical complexity, so it may not be worth
> overthinking. At the same time, I really appreciate you including me

I wouldn't call it (adding an extra load() before xchg()) "overthinking"
;-), because we are working in kernel, and we care about performance. We
actually use that pattern everywhere in the kernel. So thank you for
your work on this, the implementation and the benchmarking.

Regards,
Boqun

> in this conversation and acknowledging my contribution, Miguel;
> communities can be quite sensitive, and sometimes, attention to the
> small details really does make a difference. Perhaps we could simply
> add a "Suggested-by" or "Co-developed-by" acknowledgment and move
> forward?
> 
> FWIW, my motivation to look into it was the TODO kernel/error.rs:123.
> Might be a good first place to use the new macros.
> 
> Jens

Re: [PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

On Mon, 4 Nov 2024 23:08:21 +0100 (CET)
jens.korinth@tuta.io wrote:

> FWIW, my motivation to look into it was the TODO
> kernel/error.rs:123. Might be a good first place to use the new
> macros.

I think that WARN_ONCE needs much work than the code to execute once.

The following your warn_once implementation (from Zulip) isn't what C's
WARN_ONCE does.

macro_rules! warn_once {
    ($($arg:tt)*) => (
        do_once_lite_if!(true, crate::pr_warn!($($arg)*))
    )
}

I think that all the architectures that support Rust implement WARN_ONCE
in assembly. The simple wrapper for them doesn't work.

I assume that we simply can't cut and past the assembly in the C side
so it will be complicated.

I would recommend you to work on pr_*_once than WARN_ONCE; just a
wrapper for pr_* function.

Re: [PATCH v1] rust: Add pr_*_once macros

[Miguel Ojeda]

On Tue, Nov 5, 2024 at 12:33 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I think that all the architectures that support Rust implement WARN_ONCE
> in assembly. The simple wrapper for them doesn't work.

As far as I can see, `WARN_ONCE` uses the once lite support, not the
jump label one.

Which is why I assume Jens picked that name.

But even if that wasn't true...

> I assume that we simply can't cut and past the assembly in the C side
> so it will be complicated.

...we will have (going into -next as we speak -- Steven just picked it
up) a `RSCPP` rule that we use to expand C macros that end up in an
inline assembly string and then paste into a Rust file (cutting the
part of the file that is not part of that string).

It was added for jump label for tracepoints, precisely to avoid
duplicating the inline assembly.

Cheers,
Miguel

Re: [PATCH v1] rust: Add pr_*_once macros

[Miguel Ojeda]

On Mon, Nov 4, 2024 at 11:08 PM <jens.korinth@tuta.io> wrote:
>
> I think you both make good points. Fujita, you're right - it is very basic code with little technical complexity, so it may not be worth overthinking. At the same time, I really appreciate you including me in this conversation and acknowledging my contribution, Miguel; communities can be quite sensitive, and sometimes, attention to the small details really does make a difference. Perhaps we could simply add a "Suggested-by" or "Co-developed-by" acknowledgment and move forward?

You're welcome!

"Co-developed-by" sounds good to me, but since Tomonori is OK with the
split in two patches, I think that is even better, since you would be
the author and you would have your first commit in the kernel, which
is deserved.

By the way, perhaps I worded my reply above badly, because I don't
consider this "very basic code" (it is why I quoted it, but I should
have used "if" rather than "that") -- the fact that we had a Zulip
discussion about it, plus this email thread, plus the `once` vs
`once_lite` confusion, is already proof enough ;)

Moreover, on the textbook bit that Tomonori mentioned: it would be
amazing if Rust macros for the Linux kernel already appeared in
textbooks that we can copy, but I think it may be way, way too early
for that :)

> FWIW, my motivation to look into it was the TODO kernel/error.rs:123. Might be a good first place to use the new macros.

Indeed, if you want to fix that one, then it would be a nice first
user in case you want to send the patch -- I suggested that `TODO`
line long ago, so I am glad seeing it go! :)

Cheers,
Miguel

Re: [PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

On Tue, 5 Nov 2024 21:35:01 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> "Co-developed-by" sounds good to me, but since Tomonori is OK with the
> split in two patches, I think that is even better, since you would be
> the author and you would have your first commit in the kernel, which
> is deserved.

Jens, I'll leave submitting a patch to you. You can use my patch
however you like.


> Moreover, on the textbook bit that Tomonori mentioned: it would be
> amazing if Rust macros for the Linux kernel already appeared in
> textbooks that we can copy, but I think it may be way, way too early
> for that :)

I was taking more generally. Not limited to Linux kernel or Rust.

An apporach to use atomic bool type for "excute only once" like this
patch could be found in textbook about concurrent programming in a
programing language.

Re: [PATCH v1] rust: Add pr_*_once macros

[FUJITA Tomonori]

On Tue, 5 Nov 2024 21:20:48 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

>> I assume that we simply can't cut and past the assembly in the C side
>> so it will be complicated.
> 
> ...we will have (going into -next as we speak -- Steven just picked it
> up) a `RSCPP` rule that we use to expand C macros that end up in an
> inline assembly string and then paste into a Rust file (cutting the
> part of the file that is not part of that string).
> 
> It was added for jump label for tracepoints, precisely to avoid
> duplicating the inline assembly.

Yeah, I know. I was thinking about a way to use that approach for
assebly used for WARN_*.