Re: [PATCH v1 1/2] rust: Add support for calling a function exactly once

[Boqun Feng <boqun.feng@gmail.com>]

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