rust/kernel/sync/condvar.rs | 3 +++ 1 file changed, 3 insertions(+)
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.0On 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
>
>
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
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)
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
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)
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
The following commit has been merged into the sched/core branch of tip:
Commit-ID: 3f9ebeba9878679bb43ee2db7d50a4691f55e3a5
Gitweb: https://git.kernel.org/tip/3f9ebeba9878679bb43ee2db7d50a4691f55e3a5
Author: Kunwu Chan <kunwu.chan@hotmail.com>
AuthorDate: Mon, 24 Mar 2025 14:18:34 +08:00
Committer: Boqun Feng <boqun.feng@gmail.com>
CommitterDate: Tue, 24 Jun 2025 10:23:48 -07:00
rust: sync: Mark CondVar::notify_*() inline
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
...
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.
[boqun: Reword the commit title for consistency and reformat the commit
log.]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1145
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>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://lore.kernel.org/r/20250324061835.1693125-1-kunwu.chan@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 caebf03..c6ec642 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -216,6 +216,7 @@ impl CondVar {
/// 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 @@ impl CondVar {
///
/// 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 @@ impl CondVar {
///
/// 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);
}© 2016 - 2025 Red Hat, Inc.