[PATCH] rust: task: mark Task methods inline

[PATCH] rust: task: mark Task methods inline

[Panagiotis Foliadis]

When you build the kernel using the llvm-18.1.3-rust-1.85.0-x86_64
toolchain provided by kernel.org, the following symbols are generated:

$ nm vmlinux | grep ' _R'.*Task | rustfilt
ffffffff817b2d30 T <kernel::task::Task>::get_pid_ns
ffffffff817b2d50 T <kernel::task::Task>::tgid_nr_ns
ffffffff817b2c90 T <kernel::task::Task>::current_pid_ns
ffffffff817b2d00 T <kernel::task::Task>::signal_pending
ffffffff817b2cc0 T <kernel::task::Task>::uid
ffffffff817b2ce0 T <kernel::task::Task>::euid
ffffffff817b2c70 T <kernel::task::Task>::current
ffffffff817b2d70 T <kernel::task::Task>::wake_up
ffffffff817b2db0 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::dec_ref
ffffffff817b2d90 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::inc_ref

Most of these Rust symbols are trivial wrappers around the C functions
signal_pending, uid, euid, wake_up, dec_ref and inc_ref.It doesn't
make sense to go through a trivial wrapper for these functions, so
mark them inline.

After applying this patch, the above command will produce this output:

ffff8000805aa004 T <kernel::task::Task>::get_pid_ns
ffff8000805aa01c T <kernel::task::Task>::tgid_nr_ns
ffff8000805a9fe8 T <kernel::task::Task>::current_pid_ns
ffff8000805a9fd0 T <kernel::task::Task>::current

Signed-off-by: Panagiotis Foliadis <pfoliadis@posteo.net>
Link: https://github.com/Rust-for-Linux/linux/issues/1145
---
 rust/kernel/task.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 07bc22a7645c0c7d792a0a163dd55b8ff0fe5f92..996d7c96e48689a5752817f9ca196c021865291d 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -273,18 +273,21 @@ pub fn pid(&self) -> Pid {
     }
 
     /// Returns the UID of the given task.
+    #[inline]
     pub fn uid(&self) -> Kuid {
         // SAFETY: It's always safe to call `task_uid` on a valid task.
         Kuid::from_raw(unsafe { bindings::task_uid(self.as_ptr()) })
     }
 
     /// Returns the effective UID of the given task.
+    #[inline]
     pub fn euid(&self) -> Kuid {
         // SAFETY: It's always safe to call `task_euid` on a valid task.
         Kuid::from_raw(unsafe { bindings::task_euid(self.as_ptr()) })
     }
 
     /// Determines whether the given task has pending signals.
+    #[inline]
     pub fn signal_pending(&self) -> bool {
         // SAFETY: It's always safe to call `signal_pending` on a valid task.
         unsafe { bindings::signal_pending(self.as_ptr()) != 0 }
@@ -319,6 +322,7 @@ pub fn tgid_nr_ns(&self, pidns: Option<&PidNamespace>) -> Pid {
     }
 
     /// Wakes up the task.
+    #[inline]
     pub fn wake_up(&self) {
         // SAFETY: It's always safe to call `signal_pending` on a valid task, even if the task
         // running.
@@ -328,11 +332,13 @@ pub fn wake_up(&self) {
 
 // SAFETY: The type invariants guarantee that `Task` is always refcounted.
 unsafe impl crate::types::AlwaysRefCounted for Task {
+    #[inline]
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
         unsafe { bindings::get_task_struct(self.as_ptr()) };
     }
 
+    #[inline]
     unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         // SAFETY: The safety requirements guarantee that the refcount is nonzero.
         unsafe { bindings::put_task_struct(obj.cast().as_ptr()) }

---
base-commit: 7f0e9ee5e44887272627d0fcde0b19a675daf597
change-id: 20250308-inline-c-wrappers-da83ec1c2a77

Best regards,
-- 
Panagiotis Foliadis <pfoliadis@posteo.net>

Re: [PATCH] rust: task: mark Task methods inline

[Alice Ryhl]

On Mon, Mar 10, 2025 at 10:40 AM Panagiotis Foliadis
<pfoliadis@posteo.net> wrote:
>
> When you build the kernel using the llvm-18.1.3-rust-1.85.0-x86_64
> toolchain provided by kernel.org, the following symbols are generated:
>
> $ nm vmlinux | grep ' _R'.*Task | rustfilt
> ffffffff817b2d30 T <kernel::task::Task>::get_pid_ns
> ffffffff817b2d50 T <kernel::task::Task>::tgid_nr_ns
> ffffffff817b2c90 T <kernel::task::Task>::current_pid_ns
> ffffffff817b2d00 T <kernel::task::Task>::signal_pending
> ffffffff817b2cc0 T <kernel::task::Task>::uid
> ffffffff817b2ce0 T <kernel::task::Task>::euid
> ffffffff817b2c70 T <kernel::task::Task>::current
> ffffffff817b2d70 T <kernel::task::Task>::wake_up
> ffffffff817b2db0 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::dec_ref
> ffffffff817b2d90 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::inc_ref
>
> Most of these Rust symbols are trivial wrappers around the C functions
> signal_pending, uid, euid, wake_up, dec_ref and inc_ref.It doesn't
> make sense to go through a trivial wrapper for these functions, so
> mark them inline.

There's no C function called dec_ref or inc_ref? Please use the C
function names instead of the Rust ones.

> After applying this patch, the above command will produce this output:
>
> ffff8000805aa004 T <kernel::task::Task>::get_pid_ns
> ffff8000805aa01c T <kernel::task::Task>::tgid_nr_ns
> ffff8000805a9fe8 T <kernel::task::Task>::current_pid_ns
> ffff8000805a9fd0 T <kernel::task::Task>::current

I think it'd be nice with an explanation of why you did not mark these
#[inline].

> Signed-off-by: Panagiotis Foliadis <pfoliadis@posteo.net>
> Link: https://github.com/Rust-for-Linux/linux/issues/1145

The SoB usually goes at the bottom of the tags.

>  rust/kernel/task.rs | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 07bc22a7645c0c7d792a0a163dd55b8ff0fe5f92..996d7c96e48689a5752817f9ca196c021865291d 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -273,18 +273,21 @@ pub fn pid(&self) -> Pid {
>      }
>
>      /// Returns the UID of the given task.
> +    #[inline]
>      pub fn uid(&self) -> Kuid {
>          // SAFETY: It's always safe to call `task_uid` on a valid task.
>          Kuid::from_raw(unsafe { bindings::task_uid(self.as_ptr()) })
>      }
>
>      /// Returns the effective UID of the given task.
> +    #[inline]
>      pub fn euid(&self) -> Kuid {
>          // SAFETY: It's always safe to call `task_euid` on a valid task.
>          Kuid::from_raw(unsafe { bindings::task_euid(self.as_ptr()) })
>      }
>
>      /// Determines whether the given task has pending signals.
> +    #[inline]
>      pub fn signal_pending(&self) -> bool {
>          // SAFETY: It's always safe to call `signal_pending` on a valid task.
>          unsafe { bindings::signal_pending(self.as_ptr()) != 0 }
> @@ -319,6 +322,7 @@ pub fn tgid_nr_ns(&self, pidns: Option<&PidNamespace>) -> Pid {
>      }
>
>      /// Wakes up the task.
> +    #[inline]
>      pub fn wake_up(&self) {
>          // SAFETY: It's always safe to call `signal_pending` on a valid task, even if the task
>          // running.
> @@ -328,11 +332,13 @@ pub fn wake_up(&self) {
>
>  // SAFETY: The type invariants guarantee that `Task` is always refcounted.
>  unsafe impl crate::types::AlwaysRefCounted for Task {
> +    #[inline]
>      fn inc_ref(&self) {
>          // SAFETY: The existence of a shared reference means that the refcount is nonzero.
>          unsafe { bindings::get_task_struct(self.as_ptr()) };
>      }
>
> +    #[inline]
>      unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          // SAFETY: The safety requirements guarantee that the refcount is nonzero.
>          unsafe { bindings::put_task_struct(obj.cast().as_ptr()) }
>
> ---
> base-commit: 7f0e9ee5e44887272627d0fcde0b19a675daf597
> change-id: 20250308-inline-c-wrappers-da83ec1c2a77
>
> Best regards,
> --
> Panagiotis Foliadis <pfoliadis@posteo.net>
>

Re: [PATCH] rust: task: mark Task methods inline

[Panagiotis Foliadis]

On 10/3/25 12:23, Alice Ryhl wrote:
> On Mon, Mar 10, 2025 at 10:40 AM Panagiotis Foliadis
> <pfoliadis@posteo.net> wrote:
>> When you build the kernel using the llvm-18.1.3-rust-1.85.0-x86_64
>> toolchain provided by kernel.org, the following symbols are generated:
>>
>> $ nm vmlinux | grep ' _R'.*Task | rustfilt
>> ffffffff817b2d30 T <kernel::task::Task>::get_pid_ns
>> ffffffff817b2d50 T <kernel::task::Task>::tgid_nr_ns
>> ffffffff817b2c90 T <kernel::task::Task>::current_pid_ns
>> ffffffff817b2d00 T <kernel::task::Task>::signal_pending
>> ffffffff817b2cc0 T <kernel::task::Task>::uid
>> ffffffff817b2ce0 T <kernel::task::Task>::euid
>> ffffffff817b2c70 T <kernel::task::Task>::current
>> ffffffff817b2d70 T <kernel::task::Task>::wake_up
>> ffffffff817b2db0 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::dec_ref
>> ffffffff817b2d90 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::inc_ref
>>
>> Most of these Rust symbols are trivial wrappers around the C functions
>> signal_pending, uid, euid, wake_up, dec_ref and inc_ref.It doesn't
>> make sense to go through a trivial wrapper for these functions, so
>> mark them inline.
> There's no C function called dec_ref or inc_ref? Please use the C
> function names instead of the Rust ones.
>
>> After applying this patch, the above command will produce this output:
>>
>> ffff8000805aa004 T <kernel::task::Task>::get_pid_ns
>> ffff8000805aa01c T <kernel::task::Task>::tgid_nr_ns
>> ffff8000805a9fe8 T <kernel::task::Task>::current_pid_ns
>> ffff8000805a9fd0 T <kernel::task::Task>::current
> I think it'd be nice with an explanation of why you did not mark these
> #[inline].

Since the issue focuses on the functions that are trivial wrappers around
c and `do nothing that call a C function` I thought i would leave this out
since there is some other functionality (albeit sometimes minimal) other
that being just a c-wrapper.

>
>> Signed-off-by: Panagiotis Foliadis <pfoliadis@posteo.net>
>> Link: https://github.com/Rust-for-Linux/linux/issues/1145
> The SoB usually goes at the bottom of the tags.
>
>>   rust/kernel/task.rs | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
>> index 07bc22a7645c0c7d792a0a163dd55b8ff0fe5f92..996d7c96e48689a5752817f9ca196c021865291d 100644
>> --- a/rust/kernel/task.rs
>> +++ b/rust/kernel/task.rs
>> @@ -273,18 +273,21 @@ pub fn pid(&self) -> Pid {
>>       }
>>
>>       /// Returns the UID of the given task.
>> +    #[inline]
>>       pub fn uid(&self) -> Kuid {
>>           // SAFETY: It's always safe to call `task_uid` on a valid task.
>>           Kuid::from_raw(unsafe { bindings::task_uid(self.as_ptr()) })
>>       }
>>
>>       /// Returns the effective UID of the given task.
>> +    #[inline]
>>       pub fn euid(&self) -> Kuid {
>>           // SAFETY: It's always safe to call `task_euid` on a valid task.
>>           Kuid::from_raw(unsafe { bindings::task_euid(self.as_ptr()) })
>>       }
>>
>>       /// Determines whether the given task has pending signals.
>> +    #[inline]
>>       pub fn signal_pending(&self) -> bool {
>>           // SAFETY: It's always safe to call `signal_pending` on a valid task.
>>           unsafe { bindings::signal_pending(self.as_ptr()) != 0 }
>> @@ -319,6 +322,7 @@ pub fn tgid_nr_ns(&self, pidns: Option<&PidNamespace>) -> Pid {
>>       }
>>
>>       /// Wakes up the task.
>> +    #[inline]
>>       pub fn wake_up(&self) {
>>           // SAFETY: It's always safe to call `signal_pending` on a valid task, even if the task
>>           // running.
>> @@ -328,11 +332,13 @@ pub fn wake_up(&self) {
>>
>>   // SAFETY: The type invariants guarantee that `Task` is always refcounted.
>>   unsafe impl crate::types::AlwaysRefCounted for Task {
>> +    #[inline]
>>       fn inc_ref(&self) {
>>           // SAFETY: The existence of a shared reference means that the refcount is nonzero.
>>           unsafe { bindings::get_task_struct(self.as_ptr()) };
>>       }
>>
>> +    #[inline]
>>       unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>           // SAFETY: The safety requirements guarantee that the refcount is nonzero.
>>           unsafe { bindings::put_task_struct(obj.cast().as_ptr()) }
>>
>> ---
>> base-commit: 7f0e9ee5e44887272627d0fcde0b19a675daf597
>> change-id: 20250308-inline-c-wrappers-da83ec1c2a77
>>
>> Best regards,
>> --
>> Panagiotis Foliadis <pfoliadis@posteo.net>
>>

Re: [PATCH] rust: task: mark Task methods inline

[Alice Ryhl]

On Mon, Mar 10, 2025 at 04:09:09PM +0000, Panagiotis Foliadis wrote:
> 
> 
> On 10/3/25 12:23, Alice Ryhl wrote:
> > On Mon, Mar 10, 2025 at 10:40 AM Panagiotis Foliadis
> > <pfoliadis@posteo.net> wrote:
> > > When you build the kernel using the llvm-18.1.3-rust-1.85.0-x86_64
> > > toolchain provided by kernel.org, the following symbols are generated:
> > > 
> > > $ nm vmlinux | grep ' _R'.*Task | rustfilt
> > > ffffffff817b2d30 T <kernel::task::Task>::get_pid_ns
> > > ffffffff817b2d50 T <kernel::task::Task>::tgid_nr_ns
> > > ffffffff817b2c90 T <kernel::task::Task>::current_pid_ns
> > > ffffffff817b2d00 T <kernel::task::Task>::signal_pending
> > > ffffffff817b2cc0 T <kernel::task::Task>::uid
> > > ffffffff817b2ce0 T <kernel::task::Task>::euid
> > > ffffffff817b2c70 T <kernel::task::Task>::current
> > > ffffffff817b2d70 T <kernel::task::Task>::wake_up
> > > ffffffff817b2db0 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::dec_ref
> > > ffffffff817b2d90 T <kernel::task::Task as kernel::types::AlwaysRefCounted>::inc_ref
> > > 
> > > Most of these Rust symbols are trivial wrappers around the C functions
> > > signal_pending, uid, euid, wake_up, dec_ref and inc_ref.It doesn't
> > > make sense to go through a trivial wrapper for these functions, so
> > > mark them inline.
> > There's no C function called dec_ref or inc_ref? Please use the C
> > function names instead of the Rust ones.
> > 
> > > After applying this patch, the above command will produce this output:
> > > 
> > > ffff8000805aa004 T <kernel::task::Task>::get_pid_ns
> > > ffff8000805aa01c T <kernel::task::Task>::tgid_nr_ns
> > > ffff8000805a9fe8 T <kernel::task::Task>::current_pid_ns
> > > ffff8000805a9fd0 T <kernel::task::Task>::current
> > I think it'd be nice with an explanation of why you did not mark these
> > #[inline].
> 
> Since the issue focuses on the functions that are trivial wrappers around
> c and `do nothing that call a C function` I thought i would leave this out
> since there is some other functionality (albeit sometimes minimal) other
> that being just a c-wrapper.

As far as I can tell, all four functions will compile down to a simple
call to a C function.

get_pid_ns, tgid_nr_ns: These functions have a pointer where they say
"if the pointer is null, replace it with null, otherwise just use the
pointer". The optimizer can optimize that to "just use the pointer", and
in fact I checked that this optimization happens when we wrote
tgid_nr_ns.

current_pid_ns: If you add #[inline] to PidNamespace::from_ptr, then
this is just a call to task_active_pid_ns.

current: This is just a call to Task::current_raw which in turn is just
a call to `get_current`.

Alice