Re: [PATCH v3 1/3] rust: print: Add do_once_lite macro

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

On Sun, Nov 10, 2024 at 08:45:49AM +0100, jens.korinth@tuta.io wrote:
> 
> 
> > With this `Once` type, we can do other things like waiting for some
> > initialization to finish. We can put the waiting as a future work, since
> > `pr_*_once!()` don't need it.
> >
> > Thoughts?
> >
> A `Once` implementation as the general approach of doing things once in
> the kernel would make a lot of sense to me and I think it would be the"rusty" way to go. I'd prefer that to `do_once_lite`, for sure.
> 
> However, I'm not sure if it can replace `do_once_lite`. Doesn't `Once` use
> a closure? I may be mistaken, but wouldn't that have at least the size of afunction pointer?
> 

No, because `Once` doesn't store the closure, it's just a atomic flag to
ensure that Once::call_once() can only execute once, as a result if you
have two threads calling the `call_once()` function in the same time,
only one will win, and the other won't get executed.

It's something like:

	https://godbolt.org/z/6PaWxnWxd

> My second concern would be with the methods of `Once` itself: We would
> have to rely on link-time optimization to inline the methods, which may bedeactivated. If they are not inlined, it could mean a far-jump on a potential

I don't think LTO is the one who inlines the methods, normal compiler
optimization could do the work, hint: `call_once()` is a generic
function over the closure.

> hot-path that could be triggered many times per second?
> 

and normally kernel will be compiled with -Copt-level=2, and the
compiler would "inline" the closures properly.

> > Another meta comment is the usage of `AtomicBool`, please see the LKMM
> > atomic patchset [2]. In short, we won't use core::sync atomics in
> > kernel, instead we can only use the atomics implemented by ourselves.
> > And we currently don't support `Atomic<u8>`, there requires some work to
> > get that done.
> >
> Ouch, I didn't know that. Sounds like an interesting topic! I would like to
> understand in more detail how the Rust part will imitate the inline assembly
> approaches of the arch-specific code in C. So far my mental model of that is
> "we'll wrap it with bindgen". ;)
> 

Feel free to review that patchset! ;-)

> > The other thing of using `AtomicBool` is that it's not the most
> > space-efficient way for `Once` (if xchg() is used) on all the archs that
> > support Rust in kernel. RISCV doesn't has a byte-wise swap, so the
> > implementation has to simulate with a 32-bit or 64-bit swap + mask,
> > that'll be totally 5 instructions, and each instruction take 4 bytes. As
> > a result, if the `swap()` is inlined (which most atomic operations would
> > be for performance reasons), you save 3 bytes by using `AtomicBool`
> > other than `AtomicI32` at data section, but you pay 4*4 extra bytes at
> > the instruction section compared to using `AtomicI32`.
> >
> Ok, wow, good point and explanation! But it does sound like an
> (arch-specific) implementation detail. I would expect the `AtomicBool`
> implementation to do the "smart" thing on every arch, no? I think on thesemantic level we want an `AtomicBool` here, regardless of how it is
> implemented on RISCV, or another arch?

The thing is `AtomicBool` is not the right layer for this, because it
has to be the same represention as a normal bool [1]. `Once` seems to be
a good layer to do it.

[1]: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicBool.html

Regards,
Boqun

Regards,
Boqun
> Jens