[PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Kunwu Chan]

From: Kunwu Chan <kunwu.chan@hotmail.com>

When build the kernel using the llvm-18.1.3-rust-1.85.0-x86_64
with ARCH=arm64, the following symbols are generated:

$nm vmlinux | grep ' _R'.*CondVar | rustfilt
... T <kernel::sync::condvar::CondVar>::notify_all
... T <kernel::sync::condvar::CondVar>::notify_one
... T <kernel::sync::condvar::CondVar>::notify_sync
... T <kernel::sync::condvar::CondVar>::new::{closure#0}::{closure#0}::panic_cold_explicit
... T <kernel::sync::condvar::CondVar>::new::{closure#0}::{closure#0}::panic_cold_explicit
... T <kernel::sync::poll::PollCondVar>::new::{closure#0}::{closure#0}::panic_cold_explicit
... T <kernel::sync::poll::PollCondVar as core::ops::drop::Drop>::drop

These notify_* symbols are trivial wrappers around the C functions
__wake_up and __wake_up_sync. It doesn't make sense to go through
a trivial wrapper for these functions, so mark them inline.

Link: https://github.com/Rust-for-Linux/linux/issues/1145
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Co-developed-by: Grace Deng <Grace.Deng006@Gmail.com>
Signed-off-by: Grace Deng <Grace.Deng006@Gmail.com>
Signed-off-by: Kunwu Chan <kunwu.chan@hotmail.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

---
Changes in v2:
- Remove '#[inline]' for notify()
- Reword commit msg
- v1 link: https://lore.kernel.org/rust-for-linux/01c67d96-6477-4851-81ae-0cbee3b9e893@linux.dev
---
 rust/kernel/sync/condvar.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index caebf03f553b..c6ec64295c9f 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -216,6 +216,7 @@ fn notify(&self, count: c_int) {
     /// This method behaves like `notify_one`, except that it hints to the scheduler that the
     /// current thread is about to go to sleep, so it should schedule the target thread on the same
     /// CPU.
+    #[inline]
     pub fn notify_sync(&self) {
         // SAFETY: `wait_queue_head` points to valid memory.
         unsafe { bindings::__wake_up_sync(self.wait_queue_head.get(), TASK_NORMAL) };
@@ -225,6 +226,7 @@ pub fn notify_sync(&self) {
     ///
     /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
     /// completely (as opposed to automatically waking up the next waiter).
+    #[inline]
     pub fn notify_one(&self) {
         self.notify(1);
     }
@@ -233,6 +235,7 @@ pub fn notify_one(&self) {
     ///
     /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
     /// completely (as opposed to automatically waking up the next waiter).
+    #[inline]
     pub fn notify_all(&self) {
         self.notify(0);
     }
-- 
2.43.0

Re: [PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Miguel Ojeda]

On Mon, Mar 24, 2025 at 7:19 AM Kunwu Chan <kunwu.chan@linux.dev> wrote:
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1145
> Suggested-by: Alice Ryhl <aliceryhl@google.com>

Nit: I typically use Link after Suggested-by, to mimic what the docs
require about Reported-by with Link. (No need to resend a new version
just for this :)

> - Remove '#[inline]' for notify()

It is good that the commit matches the log now, though I wonder if in
the future we may want the `#[inline]` for `notify` anyway, even if
LLVM does that one on its own today.

Cheers,
Miguel

Re: [PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Boqun Feng]

On Mon, Mar 24, 2025 at 10:37:38AM +0100, Miguel Ojeda wrote:
> On Mon, Mar 24, 2025 at 7:19 AM Kunwu Chan <kunwu.chan@linux.dev> wrote:
> >
> > Link: https://github.com/Rust-for-Linux/linux/issues/1145
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> 
> Nit: I typically use Link after Suggested-by, to mimic what the docs
> require about Reported-by with Link. (No need to resend a new version
> just for this :)
> 
> > - Remove '#[inline]' for notify()
> 
> It is good that the commit matches the log now, though I wonder if in
> the future we may want the `#[inline]` for `notify` anyway, even if
> LLVM does that one on its own today.
> 

IMO, inlining should be mostly decided by compilers instead of
programmers, so if we don't have evidences that compilers need some
guidance, we shouldn't introduce the `#[inline]` attribute.

Maybe Kunwu has an example saying otherwise?

Regards,
Boqun

> Cheers,
> Miguel

Re: [PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Kunwu Chan]

On 2025/3/24 17:37, Miguel Ojeda wrote:
> On Mon, Mar 24, 2025 at 7:19 AM Kunwu Chan<kunwu.chan@linux.dev> wrote:
>> Link:https://github.com/Rust-for-Linux/linux/issues/1145
>> Suggested-by: Alice Ryhl<aliceryhl@google.com>
> Nit: I typically use Link after Suggested-by, to mimic what the docs
> require about Reported-by with Link. (No need to resend a new version
> just for this :)
Thanks for the kind reminder, I'll follow this next time
>> - Remove '#[inline]' for notify()
> It is good that the commit matches the log now, though I wonder if in
> the future we may want the `#[inline]` for `notify` anyway, even if
> LLVM does that one on its own today.
Now, the default '-Copt-level' is 2 when define 
'CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE',
and in  KBUILD_RUSTFLAGS the  '-Ccodegen-units' is 1.

So if the config not change, the result should be the same.

And the actualy compile steps for rust/kernel.o is as following:

make -f ./scripts/Makefile.build obj=rust
# RUSTC L rust/kernel.o
OBJTREE=/media/kunwuchan/68d922f3-5dde-45b0-af93-c45ba81d85ef/linux-next 
rustc --edition=2021 -Zbinary_dep_depinfo=y -Astable_features 
-Dnon_ascii_idents -Dunsafe_op_in_unsafe_fn -Wmissing_docs 
-Wrust_2018_idioms -Wunreachable_pub -Wclippy::all 
-Wclippy::ignored_unit_patterns -Wclippy::mut_mut 
-Wclippy::needless_bitwise_bool -Wclippy::needless_continue 
-Aclippy::needless_lifetimes -Wclippy::no_mangle_with_rust_abi 
-Wclippy::undocumented_unsafe_blocks 
-Wclippy::unnecessary_safety_comment -Wclippy::unnecessary_safety_doc 
-Wrustdoc::missing_crate_level_docs -Wrustdoc::unescaped_backticks 
-Cpanic=abort -Cembed-bitcode=n -Clto=n -Cforce-unwind-tables=n 
-Ccodegen-units=1 -Csymbol-mangling-version=v0 -Crelocation-model=static 
-Zfunction-sections=n -Wclippy::float_arithmetic 
--target=aarch64-unknown-none-softfloat -Cforce-unwind-tables=n 
-Zbranch-protection=bti,pac-ret -Copt-level=2 -Cdebug-assertions=y 
-Coverflow-checks=y -Cforce-frame-pointers=y -Cdebuginfo=1 
@./include/generated/rustc_cfg --extern ffi --extern pin_init --extern 
build_error --extern macros --extern bindings --extern uapi 
--emit=dep-info=rust/.kernel.o.d --emit=obj=rust/kernel.o 
--emit=metadata=rust/libkernel.rmeta --crate-type rlib -L./rust 
--crate-name kernel rust/kernel/lib.rs --sysroot=/dev/null
# EXPORTS rust/exports_kernel_generated.h
   llvm-nm -p --defined-only rust/kernel.o | awk '$2~/(T|R|D|B)/ && 
$3!~/__cfi/ && $3!~/__odr_asan/ { printf "EXPORT_SYMBOL_RUST_GPL(%s);
",$3 }' > rust/exports_kernel_generated.h

> Cheers,
> Miguel

-- 
Thanks,
   Kunwu.Chan(Tao.Chan)

Re: [PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Miguel Ojeda]

On Tue, Mar 25, 2025 at 4:02 AM Kunwu Chan <kunwu.chan@linux.dev> wrote:
>
> Thanks for the kind reminder, I'll follow this next time

You're welcome!

> Now, the default '-Copt-level' is 2 when define
> 'CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE',
> and in  KBUILD_RUSTFLAGS the  '-Ccodegen-units' is 1.
>
> So if the config not change, the result should be the same.

I don't think that is true, because there are a few `rustc` versions
as well as LLVM ones that we support, so how inlining happens (at both
`rustc` and LLVM levels) may change even if the configuration is
essentially the same.

And, of course, then there are other non-compiler-related kernel
config options (i.e. not compiler flags, but other stuff, like `cfg`s)
that influence which and how gets emitted and thus the inlining
decisions.

Eventually we should have pure GCC builds too, which is yet another factor...

To be clear, I agree with Boqun that having to write `#[inline]`
everywhere is not great. Rust 1.75 already started to tag small
functions as `#[inline]` to try to prevent the proliferation of the
attribute, which is good (i.e. which is what triggered the message in
Compiler Explorer).

By the way, one difference is whether it is `pub` -- the `notify()`
isn't (unlike the others), but if it were, then we would have needed
the `#[inline]`, from a quick test.

Thanks!

Cheers,
Miguel

Re: [PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Kunwu Chan]

On 2025/3/25 18:12, Miguel Ojeda wrote:
> On Tue, Mar 25, 2025 at 4:02 AM Kunwu Chan <kunwu.chan@linux.dev> wrote:
>> Thanks for the kind reminder, I'll follow this next time
> You're welcome!
>
>> Now, the default '-Copt-level' is 2 when define
>> 'CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE',
>> and in  KBUILD_RUSTFLAGS the  '-Ccodegen-units' is 1.
>>
>> So if the config not change, the result should be the same.
> I don't think that is true, because there are a few `rustc` versions
> as well as LLVM ones that we support, so how inlining happens (at both
> `rustc` and LLVM levels) may change even if the configuration is
> essentially the same.
>
> And, of course, then there are other non-compiler-related kernel
> config options (i.e. not compiler flags, but other stuff, like `cfg`s)
> that influence which and how gets emitted and thus the inlining
> decisions.
> Eventually we should have pure GCC builds too, which is yet another factor...
>
> To be clear, I agree with Boqun that having to write `#[inline]`
> everywhere is not great. Rust 1.75 already started to tag small
> functions as `#[inline]` to try to prevent the proliferation of the
> attribute, which is good (i.e. which is what triggered the message in
> Compiler Explorer).

Thanks for the detailed reply.

Sure on the one hand, the decisions is a result of many factors,

on the other hand the rustc and llvm is rapidly developing.

It's a complicated thing.

> By the way, one difference is whether it is `pub` -- the `notify()`
> isn't (unlike the others), but if it were, then we would have needed
> the `#[inline]`, from a quick test.
That is a easy way to judge. I learned.
> Thanks!
>
> Cheers,
> Miguel

-- 
Thanks,
   Kunwu.Chan(Tao.Chan)

Re: [PATCH v2] rust: sync: optimize rust symbol generation for CondVar

[Boqun Feng]

On Mon, Mar 24, 2025 at 02:18:34PM +0800, Kunwu Chan wrote:
> From: Kunwu Chan <kunwu.chan@hotmail.com>
> 
> When build the kernel using the llvm-18.1.3-rust-1.85.0-x86_64
> with ARCH=arm64, the following symbols are generated:
> 
> $nm vmlinux | grep ' _R'.*CondVar | rustfilt
> ... T <kernel::sync::condvar::CondVar>::notify_all
> ... T <kernel::sync::condvar::CondVar>::notify_one
> ... T <kernel::sync::condvar::CondVar>::notify_sync
> ... T <kernel::sync::condvar::CondVar>::new::{closure#0}::{closure#0}::panic_cold_explicit
> ... T <kernel::sync::condvar::CondVar>::new::{closure#0}::{closure#0}::panic_cold_explicit
> ... T <kernel::sync::poll::PollCondVar>::new::{closure#0}::{closure#0}::panic_cold_explicit
> ... T <kernel::sync::poll::PollCondVar as core::ops::drop::Drop>::drop
> 
> These notify_* symbols are trivial wrappers around the C functions
> __wake_up and __wake_up_sync. It doesn't make sense to go through
> a trivial wrapper for these functions, so mark them inline.
> 
> Link: https://github.com/Rust-for-Linux/linux/issues/1145
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Co-developed-by: Grace Deng <Grace.Deng006@Gmail.com>
> Signed-off-by: Grace Deng <Grace.Deng006@Gmail.com>
> Signed-off-by: Kunwu Chan <kunwu.chan@hotmail.com>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 

I'm taking this via tip with the version as it is (i.e. no #[inline] for
notify()), but will reorder the tags. Thanks!

Regards,
Boqun

> ---
> Changes in v2:
> - Remove '#[inline]' for notify()
> - Reword commit msg
> - v1 link: https://lore.kernel.org/rust-for-linux/01c67d96-6477-4851-81ae-0cbee3b9e893@linux.dev
> ---
>  rust/kernel/sync/condvar.rs | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
> index caebf03f553b..c6ec64295c9f 100644
> --- a/rust/kernel/sync/condvar.rs
> +++ b/rust/kernel/sync/condvar.rs
> @@ -216,6 +216,7 @@ fn notify(&self, count: c_int) {
>      /// This method behaves like `notify_one`, except that it hints to the scheduler that the
>      /// current thread is about to go to sleep, so it should schedule the target thread on the same
>      /// CPU.
> +    #[inline]
>      pub fn notify_sync(&self) {
>          // SAFETY: `wait_queue_head` points to valid memory.
>          unsafe { bindings::__wake_up_sync(self.wait_queue_head.get(), TASK_NORMAL) };
> @@ -225,6 +226,7 @@ pub fn notify_sync(&self) {
>      ///
>      /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
>      /// completely (as opposed to automatically waking up the next waiter).
> +    #[inline]
>      pub fn notify_one(&self) {
>          self.notify(1);
>      }
> @@ -233,6 +235,7 @@ pub fn notify_one(&self) {
>      ///
>      /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
>      /// completely (as opposed to automatically waking up the next waiter).
> +    #[inline]
>      pub fn notify_all(&self) {
>          self.notify(0);
>      }
> -- 
> 2.43.0
> 
>