linux-kernel - Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72kpohLie_23sMQm6h-Cq6wiqqvFfwSsAdi4x760kCisoA@mail.gmail.com>
Date: Tue, 26 Nov 2024 20:00:29 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: jens.korinth@...a.io
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org, 
	FUJITA Tomonori <fujita.tomonori@...il.com>, Dirk Behme <dirk.behme@...il.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once

On Tue, Nov 26, 2024 at 5:41 PM Jens Korinth via B4 Relay
<devnull+jens.korinth.tuta.io@...nel.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