LinuxLists.cc - [PATCH 0/3] Tracepoints and static branch/call in Rust

[permalink] [raw]

Subject: [PATCH 2/3] rust: add static_key_false

Add just enough support for static key so that we can use it from
tracepoints. Tracepoints rely on `static_key_false` even though it is
deprecated, so we add the same functionality to Rust.

Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/lib.rs | 1 +
rust/kernel/static_key.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++++
scripts/Makefile.build | 2 +-
3 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index d534b1178955..22e1fedd0774 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -39,6 +39,7 @@
pub mod print;
mod static_assert;
pub mod static_call;
+pub mod static_key;
#[doc(hidden)]
pub mod std_vendor;
pub mod str;
diff --git a/rust/kernel/static_key.rs b/rust/kernel/static_key.rs
new file mode 100644
index 000000000000..6c3dbe14c98a
--- /dev/null
+++ b/rust/kernel/static_key.rs
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Logic for static keys.
+
+use crate::bindings::*;
+
+#[doc(hidden)]
+#[macro_export]
+#[cfg(target_arch = "x86_64")]
+macro_rules! _static_key_false {
+ ($key:path, $keytyp:ty, $field:ident) => {'my_label: {
+ core::arch::asm!(
+ r#"
+ 1: .byte 0x0f,0x1f,0x44,0x00,0x00
+
+ .pushsection __jump_table, "aw"
+ .balign 8
+ .long 1b - .
+ .long {0} - .
+ .quad {1} + {2} - .
+ .popsection
+ "#,
+ label {
+ break 'my_label true;
+ },
+ sym $key,
+ const ::core::mem::offset_of!($keytyp, $field),
+ );
+
+ break 'my_label false;
+ }};
+}
+
+#[doc(hidden)]
+#[macro_export]
+#[cfg(target_arch = "aarch64")]
+macro_rules! _static_key_false {
+ ($key:path, $keytyp:ty, $field:ident) => {'my_label: {
+ core::arch::asm!(
+ r#"
+ 1: nop
+
+ .pushsection __jump_table, "aw"
+ .align 3
+ .long 1b - ., {0} - .
+ .quad {1} + {2} - .
+ .popsection
+ "#,
+ label {
+ break 'my_label true;
+ },
+ sym $key,
+ const ::core::mem::offset_of!($keytyp, $field),
+ );
+
+ break 'my_label false;
+ }};
+}
+
+/// Branch based on a static key.
+///
+/// Takes three arguments:
+///
+/// * `key` - the path to the static variable containing the `static_key`.
+/// * `keytyp` - the type of `key`.
+/// * `field` - the name of the field of `key` that contains the `static_key`.
+#[macro_export]
+macro_rules! static_key_false {
+ // Forward to the real implementation. Separated like this so that we don't have to duplicate
+ // the documentation.
+ ($key:path, $keytyp:ty, $field:ident) => {{
+ // Assert that `$key` has type `$keytyp` and that `$key.$field` has type `static_key`.
+ //
+ // SAFETY: We know that `$key` is a static because otherwise the inline assembly will not
+ // compile. The raw pointers created in this block are in-bounds of `$key`.
+ static _TY_ASSERT: () = unsafe {
+ let key: *const $keytyp = ::core::ptr::addr_of!($key);
+ let _: *const $crate::bindings::static_key = ::core::ptr::addr_of!((*key).$field);
+ };
+
+ $crate::static_key::_static_key_false! { $key, $keytyp, $field }
+ }};
+}
+
+pub use {_static_key_false, static_key_false};
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index efacca63c897..60197c1c063f 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -263,7 +263,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
# Compile Rust sources (.rs)
# ---------------------------------------------------------------------------

-rust_allowed_features := new_uninit
+rust_allowed_features := asm_const,asm_goto,new_uninit

# `--out-dir` is required to avoid temporaries being created by `rustc` in the
# current working directory, which may be not accessible in the out-of-tree

--
2.45.2.505.gda0bf45e8d-goog

2024-06-06 15:19:47

[permalink] [raw]

Subject: [PATCH 3/3] rust: add tracepoint support

Make it possible to have Rust code call into tracepoints defined by C
code. It is still required that the tracepoint is declared in a C
header, and that this header is included in the input to bindgen.

Signed-off-by: Alice Ryhl <[email protected]>
---
rust/bindings/bindings_helper.h | 1 +
rust/bindings/lib.rs | 15 +++++++
rust/helpers.c | 24 +++++++++++
rust/kernel/lib.rs | 1 +
rust/kernel/tracepoint.rs | 92 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 133 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ddb5644d4fd9..d442f9ccfc2c 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -15,6 +15,7 @@
#include <linux/refcount.h>
#include <linux/sched.h>
#include <linux/slab.h>
+#include <linux/tracepoint.h>
#include <linux/wait.h>
#include <linux/workqueue.h>

diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
index 40ddaee50d8b..48856761d682 100644
--- a/rust/bindings/lib.rs
+++ b/rust/bindings/lib.rs
@@ -48,3 +48,18 @@ mod bindings_helper {
}

pub use bindings_raw::*;
+
+/// Rust version of the C macro `rcu_dereference_raw`.
+///
+/// The rust helper only works with void pointers, but this wrapper method makes it work with any
+/// pointer type using pointer casts.
+///
+/// # Safety
+///
+/// This method has the same safety requirements as the C macro of the same name.
+#[inline(always)]
+pub unsafe fn rcu_dereference_raw<T>(p: *const *mut T) -> *mut T {
+ // SAFETY: This helper calls into the C macro, so the caller promises to uphold the safety
+ // requirements.
+ unsafe { __rcu_dereference_raw(p as *mut *mut _) as *mut T }
+}
diff --git a/rust/helpers.c b/rust/helpers.c
index 2c37a0f5d7a8..0560cc2a512a 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
}
EXPORT_SYMBOL_GPL(rust_helper_krealloc);

+void rust_helper_preempt_enable_notrace(void)
+{
+ preempt_enable_notrace();
+}
+EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
+
+void rust_helper_preempt_disable_notrace(void)
+{
+ preempt_disable_notrace();
+}
+EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);
+
+bool rust_helper_current_cpu_online(void)
+{
+ return cpu_online(raw_smp_processor_id());
+}
+EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
+
+void *rust_helper___rcu_dereference_raw(void **p)
+{
+ return rcu_dereference_raw(p);
+}
+EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);
+
/*
* `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
* use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 22e1fedd0774..3f3b280bb437 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -46,6 +46,7 @@
pub mod sync;
pub mod task;
pub mod time;
+pub mod tracepoint;
pub mod types;
pub mod workqueue;

diff --git a/rust/kernel/tracepoint.rs b/rust/kernel/tracepoint.rs
new file mode 100644
index 000000000000..d628ae71fc58
--- /dev/null
+++ b/rust/kernel/tracepoint.rs
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Logic for tracepoints.
+
+/// Declare the Rust entry point for a tracepoint.
+#[macro_export]
+macro_rules! declare_trace {
+ ($($(#[$attr:meta])* $pub:vis fn $name:ident($($argname:ident : $argtyp:ty),* $(,)?);)*) => {$(
+ $( #[$attr] )*
+ #[inline(always)]
+ $pub unsafe fn $name($($argname : $argtyp),*) {
+ #[cfg(CONFIG_TRACEPOINTS)]
+ {
+ use $crate::bindings::*;
+
+ // SAFETY: This macro only compiles if $name is a real tracepoint, and if it is a
+ // real tracepoint, then it is okay to query the static key.
+ let should_trace = unsafe {
+ $crate::macros::paste! {
+ $crate::static_key::static_key_false!(
+ [< __tracepoint_ $name >],
+ $crate::bindings::tracepoint,
+ key
+ )
+ }
+ };
+
+ if should_trace {
+ $crate::tracepoint::do_trace!($name($($argname : $argtyp),*), cond);
+ }
+ }
+
+ #[cfg(not(CONFIG_TRACEPOINTS))]
+ {
+ // If tracepoints are disabled, insert a trivial use of each argument
+ // to avoid unused argument warnings.
+ $( let _unused = $argname; )*
+ }
+ }
+ )*}
+}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! do_trace {
+ ($name:ident($($argname:ident : $argtyp:ty),* $(,)?), $cond:expr) => {{
+ if !$crate::bindings::current_cpu_online() {
+ return;
+ }
+
+ // SAFETY: This call is balanced with the call below.
+ unsafe { $crate::bindings::preempt_disable_notrace() };
+
+ // SAFETY: This calls the tracepoint with the provided arguments. The caller of the Rust
+ // wrapper guarantees that this is okay.
+ #[cfg(CONFIG_HAVE_STATIC_CALL)]
+ unsafe {
+ let it_func_ptr: *mut $crate::bindings::tracepoint_func =
+ $crate::bindings::rcu_dereference_raw(
+ ::core::ptr::addr_of!(
+ $crate::macros::concat_idents!(__tracepoint_, $name).funcs
+ )
+ );
+
+ if !it_func_ptr.is_null() {
+ let __data = (*it_func_ptr).data;
+ $crate::macros::paste! {
+ $crate::static_call::static_call! {
+ [< tp_func_ $name >] (__data, $($argname),*)
+ };
+ }
+ }
+ }
+
+ // SAFETY: This calls the tracepoint with the provided arguments. The caller of the Rust
+ // wrapper guarantees that this is okay.
+ #[cfg(not(CONFIG_HAVE_STATIC_CALL))]
+ unsafe {
+ $crate::macros::concat_idents!(__traceiter_, $name)(
+ ::core::ptr::null_mut(),
+ $($argname),*
+ );
+ }
+
+ // SAFETY: This call is balanced with the call above.
+ unsafe { $crate::bindings::preempt_enable_notrace() };
+ }}
+}
+
+pub use {declare_trace, do_trace};

--
2.45.2.505.gda0bf45e8d-goog

2024-06-06 15:26:33

[permalink] [raw]

Subject: Re: [PATCH 0/3] Tracepoints and static branch/call in Rust

On 2024-06-06 11:05, Alice Ryhl wrote:
> An important part of a production ready Linux kernel driver is
> tracepoints. So to write production ready Linux kernel drivers in Rust,
> we must be able to call tracepoints from Rust code. This patch series
> adds support for calling tracepoints declared in C from Rust.

I'm glad to see progress on this front ! Please see feedback below.

>
> To use the tracepoint support, you must:
>
> 1. Declare the tracepoint in a C header file as usual.
> 2. Make sure that the header file is visible to bindgen so that Rust
> bindings are generated for the symbols that the tracepoint macro
> emits.
> 3. Use the declare_trace! macro in your Rust code to generate Rust
> functions that call into the tracepoint.
>
> For example, the kernel has a tracepoint called `sched_kthread_stop`. It
> is declared like this:
>
> TRACE_EVENT(sched_kthread_stop,
> TP_PROTO(struct task_struct *t),
> TP_ARGS(t),
> TP_STRUCT__entry(
> __array( char, comm, TASK_COMM_LEN )
> __field( pid_t, pid )
> ),
> TP_fast_assign(
> memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
> __entry->pid = t->pid;
> ),
> TP_printk("comm=%s pid=%d", __entry->comm, __entry->pid)
> );
>
> To call the above tracepoint from Rust code, you would add the relevant
> header file to rust/bindings/bindings_helper.h and add the following
> invocation somewhere in your Rust code:
>
> declare_trace! {
> fn sched_kthread_stop(task: *mut task_struct);
> }
>
> This will define a Rust function of the given name that you can call
> like any other Rust function. Since these tracepoints often take raw
> pointers as arguments, it may be convenient to wrap it in a safe
> wrapper:
>
> mod raw {
> declare_trace! {
> fn sched_kthread_stop(task: *mut task_struct);
> }
> }
>
> #[inline]
> pub fn trace_sched_kthread_stop(task: &Task) {
> // SAFETY: The pointer to `task` is valid.
> unsafe { raw::sched_kthread_stop(task.as_raw()) }
> }
>
> A future expansion of the tracepoint support could generate these safe
> versions automatically, but that is left as future work for now.
>
> This is intended for use in the Rust Binder driver, which was originally
> sent as an RFC [1]. The RFC did not include tracepoint support, but you
> can see how it will be used in Rust Binder at [2]. The author has
> verified that the tracepoint support works on Android devices.
>
> This implementation implements support for static keys in Rust so that
> the actual static branch will end up in the Rust object file. However,
> it would also be possible to just wrap the trace_##name generated by
> __DECLARE_TRACE in an extern C function and then call that from Rust.
> This will simplify the Rust code by removing the need for static
> branches and calls, but it places the static branch behind an external
> call, which has performance implications.

The tracepoints try very hard to minimize overhead of dormant tracepoints
so it is not frowned-upon to have them built into production binaries.
This is needed to make sure distribution vendors keep those tracepoints
in the kernel binaries that reach end-users.

Adding a function call before evaluation of the static branch goes against
this major goal.

>
> A possible middle ground would be to place just the __DO_TRACE body in
> an extern C function and to implement the Rust wrapper by doing the
> static branch in Rust, and then calling into C the code that contains
> __DO_TRACE when the tracepoint is active. However, this would need some
> changes to include/linux/tracepoint.h to generate and export a function
> containing the body of __DO_TRACE when the tracepoint should be callable
> from Rust.

This tradeoff is more acceptable than having a function call before
evaluation of the static branch, but I wonder what is the upside of
this tradeoff compared to inlining the whole __DO_TRACE in Rust ?

> So in general, there is a tradeoff between placing parts of the
> tracepoint (which is perf sensitive) behind an external call, and having
> code duplicated in both C and Rust (which must be kept in sync when
> changes are made). This is an important point that I would like feedback
> on from the C maintainers.

I don't see how the duplication happens there: __DO_TRACE is meant to be
inlined into each C tracepoint caller site, so the code is already meant
to be duplicated. Having an explicit function wrapping the tracepoint
for Rust would just create an extra instance of __DO_TRACE if it happens
to be also inlined into C code.

Or do you meant you would like to prevent having to duplicate the
implementation of __DO_TRACE in both C and Rust ?

I'm not sure if you mean to prevent source code duplication between
C and Rust or duplication of binary code (instructions).

Thanks,

Mathieu

>
> Link: https://lore.kernel.org/rust-for-linux/[email protected]/ [1]
> Link: https://r.android.com/3110088 [2]
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> Alice Ryhl (3):
> rust: add static_call support
> rust: add static_key_false
> rust: add tracepoint support
>
> rust/bindings/bindings_helper.h | 1 +
> rust/bindings/lib.rs | 15 +++++++
> rust/helpers.c | 24 +++++++++++
> rust/kernel/lib.rs | 3 ++
> rust/kernel/static_call.rs | 92 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/static_key.rs | 87 ++++++++++++++++++++++++++++++++++++++
> rust/kernel/tracepoint.rs | 92 +++++++++++++++++++++++++++++++++++++++++
> scripts/Makefile.build | 2 +-
> 8 files changed, 315 insertions(+), 1 deletion(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240606-tracepoint-31e15b90e471
>
> Best regards,

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

2024-06-06 15:29:13

[permalink] [raw]

Subject: [PATCH 1/3] rust: add static_call support

Add static_call support by mirroring how C does. When the platform does
not support static calls (right now, that means that it is not x86),
then the function pointer is loaded from a global and called. Otherwise,
we generate a call to a trampoline function, and objtool is used to make
these calls patchable at runtime.

Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/lib.rs | 1 +
rust/kernel/static_call.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 93 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fbd91a48ff8b..d534b1178955 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -38,6 +38,7 @@
pub mod prelude;
pub mod print;
mod static_assert;
+pub mod static_call;
#[doc(hidden)]
pub mod std_vendor;
pub mod str;
diff --git a/rust/kernel/static_call.rs b/rust/kernel/static_call.rs
new file mode 100644
index 000000000000..f7b8ba7bf1fb
--- /dev/null
+++ b/rust/kernel/static_call.rs
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Logic for static calls.
+
+#[macro_export]
+#[doc(hidden)]
+macro_rules! ty_underscore_for {
+ ($arg:expr) => {
+ _
+ };
+}
+
+#[doc(hidden)]
+#[repr(transparent)]
+pub struct AddressableStaticCallKey {
+ _ptr: *const bindings::static_call_key,
+}
+unsafe impl Sync for AddressableStaticCallKey {}
+impl AddressableStaticCallKey {
+ pub const fn new(ptr: *const bindings::static_call_key) -> Self {
+ Self { _ptr: ptr }
+ }
+}
+
+#[cfg(CONFIG_HAVE_STATIC_CALL)]
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _static_call {
+ ($name:ident($($args:expr),* $(,)?)) => {{
+ // Symbol mangling will give this symbol a unique name.
+ #[cfg(CONFIG_HAVE_STATIC_CALL_INLINE)]
+ #[link_section = ".discard.addressable"]
+ #[used]
+ static __ADDRESSABLE: $crate::static_call::AddressableStaticCallKey = unsafe {
+ $crate::static_call::AddressableStaticCallKey::new(::core::ptr::addr_of!(
+ $crate::macros::paste! { $crate::bindings:: [<__SCK__ $name >]; }
+ ))
+ };
+
+ let fn_ptr: unsafe extern "C" fn($($crate::static_call::ty_underscore_for!($args)),*) -> _ =
+ $crate::macros::paste! { $crate::bindings:: [<__SCT__ $name >]; };
+ (fn_ptr)($($args),*)
+ }};
+}
+
+#[cfg(not(CONFIG_HAVE_STATIC_CALL))]
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _static_call {
+ ($name:ident($($args:expr),* $(,)?)) => {{
+ let void_ptr_fn: *mut ::core::ffi::c_void =
+ $crate::macros::paste! { $crate::bindings:: [<__SCK__ $name >]; }.func;
+
+ let fn_ptr: unsafe extern "C" fn($($crate::static_call::ty_underscore_for!($args)),*) -> _ =
+ if true {
+ ::core::mem::transmute(void_ptr_fn)
+ } else {
+ // This is dead code, but it influences type inference on `fn_ptr` so that we
+ // transmute the function pointer to the right type.
+ $crate::macros::paste! { $crate::bindings:: [<__SCT__ $name >]; }
+ };
+
+ (fn_ptr)($($args),*)
+ }};
+}
+
+/// Statically call a global function.
+///
+/// # Safety
+///
+/// This macro will call the provided function. It is up to the caller to uphold the safety
+/// guarantees of the function.
+///
+/// # Examples
+///
+/// ```ignore
+/// fn call_static() {
+/// unsafe {
+/// static_call! { your_static_call() };
+/// }
+/// }
+/// ```
+#[macro_export]
+macro_rules! static_call {
+ // Forward to the real implementation. Separated like this so that we don't have to duplicate
+ // the documentation.
+ ($($args:tt)*) => { $crate::static_call::_static_call! { $($args)* } };
+}
+
+pub use {_static_call, static_call, ty_underscore_for};

--
2.45.2.505.gda0bf45e8d-goog

2024-06-06 15:47:33

[permalink] [raw]

Subject: Re: [PATCH 0/3] Tracepoints and static branch/call in Rust

On Thu, Jun 6, 2024 at 5:25 PM Mathieu Desnoyers
<[email protected]> wrote:
>
> On 2024-06-06 11:05, Alice Ryhl wrote:
> > This implementation implements support for static keys in Rust so that
> > the actual static branch will end up in the Rust object file. However,
> > it would also be possible to just wrap the trace_##name generated by
> > __DECLARE_TRACE in an extern C function and then call that from Rust.
> > This will simplify the Rust code by removing the need for static
> > branches and calls, but it places the static branch behind an external
> > call, which has performance implications.
>
> The tracepoints try very hard to minimize overhead of dormant tracepoints
> so it is not frowned-upon to have them built into production binaries.
> This is needed to make sure distribution vendors keep those tracepoints
> in the kernel binaries that reach end-users.
>
> Adding a function call before evaluation of the static branch goes against
> this major goal.
>
> >
> > A possible middle ground would be to place just the __DO_TRACE body in
> > an extern C function and to implement the Rust wrapper by doing the
> > static branch in Rust, and then calling into C the code that contains
> > __DO_TRACE when the tracepoint is active. However, this would need some
> > changes to include/linux/tracepoint.h to generate and export a function
> > containing the body of __DO_TRACE when the tracepoint should be callable
> > from Rust.
>
> This tradeoff is more acceptable than having a function call before
> evaluation of the static branch, but I wonder what is the upside of
> this tradeoff compared to inlining the whole __DO_TRACE in Rust ?
>
> > So in general, there is a tradeoff between placing parts of the
> > tracepoint (which is perf sensitive) behind an external call, and having
> > code duplicated in both C and Rust (which must be kept in sync when
> > changes are made). This is an important point that I would like feedback
> > on from the C maintainers.
>
> I don't see how the duplication happens there: __DO_TRACE is meant to be
> inlined into each C tracepoint caller site, so the code is already meant
> to be duplicated. Having an explicit function wrapping the tracepoint
> for Rust would just create an extra instance of __DO_TRACE if it happens
> to be also inlined into C code.
>
> Or do you meant you would like to prevent having to duplicate the
> implementation of __DO_TRACE in both C and Rust ?
>
> I'm not sure if you mean to prevent source code duplication between
> C and Rust or duplication of binary code (instructions).

It's a question of maintenance burden. If you change how __DO_TRACE is
implemented, then those changes must also be reflected in the Rust
version. There's no issue in the binary code.

Alice

2024-06-06 15:52:29

by Boqun Feng

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 06, 2024 at 11:30:03AM -0400, Mathieu Desnoyers wrote:
> On 2024-06-06 11:05, Alice Ryhl wrote:
> > Make it possible to have Rust code call into tracepoints defined by C
> > code. It is still required that the tracepoint is declared in a C
> > header, and that this header is included in the input to bindgen.
> >
> > Signed-off-by: Alice Ryhl <[email protected]>
>
> [...]
>
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 2c37a0f5d7a8..0560cc2a512a 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
> > }
> > EXPORT_SYMBOL_GPL(rust_helper_krealloc);
> > +void rust_helper_preempt_enable_notrace(void)
> > +{
> > + preempt_enable_notrace();
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
> > +
> > +void rust_helper_preempt_disable_notrace(void)
> > +{
> > + preempt_disable_notrace();
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);
> > +
> > +bool rust_helper_current_cpu_online(void)
> > +{
> > + return cpu_online(raw_smp_processor_id());
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
> > +
> > +void *rust_helper___rcu_dereference_raw(void **p)
> > +{
> > + return rcu_dereference_raw(p);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);
>
> Ouch. Doing a function call for each of those small operations will
> have a rather large performance impact when tracing is active. If it is

Long-term plan is to 1) compile the C helpers in some IR and 2) inline
the helpers with Rust in IR-level, as what Gary has:

https://lore.kernel.org/rust-for-linux/[email protected]/

and I use it for the upcoming atomic API support:

https://github.com/fbq/linux/tree/dev/rust/atomic-rfc

and it works very well.

Regards,
Boqun

> not possible to inline those in Rust, then implementing __DO_TRACE in
> a C function would at least allow Rust to only do a single call to C
> rather than go back and forth between Rust and C.
>
> What prevents inlining those helpers in Rust ?
>
> Thanks,
>
> Mathieu
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
>

2024-06-06 16:09:55

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On 2024-06-06 11:05, Alice Ryhl wrote:
> Make it possible to have Rust code call into tracepoints defined by C
> code. It is still required that the tracepoint is declared in a C
> header, and that this header is included in the input to bindgen.
>
> Signed-off-by: Alice Ryhl <[email protected]>

[...]

> diff --git a/rust/helpers.c b/rust/helpers.c
> index 2c37a0f5d7a8..0560cc2a512a 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
> }
> EXPORT_SYMBOL_GPL(rust_helper_krealloc);
>
> +void rust_helper_preempt_enable_notrace(void)
> +{
> + preempt_enable_notrace();
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
> +
> +void rust_helper_preempt_disable_notrace(void)
> +{
> + preempt_disable_notrace();
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);
> +
> +bool rust_helper_current_cpu_online(void)
> +{
> + return cpu_online(raw_smp_processor_id());
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
> +
> +void *rust_helper___rcu_dereference_raw(void **p)
> +{
> + return rcu_dereference_raw(p);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);

Ouch. Doing a function call for each of those small operations will
have a rather large performance impact when tracing is active. If it is
not possible to inline those in Rust, then implementing __DO_TRACE in
a C function would at least allow Rust to only do a single call to C
rather than go back and forth between Rust and C.

What prevents inlining those helpers in Rust ?

Thanks,

Mathieu

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

2024-06-06 16:15:14

[permalink] [raw]

Subject: Re: [PATCH 2/3] rust: add static_key_false

On 2024-06-06 11:05, Alice Ryhl wrote:
> Add just enough support for static key so that we can use it from
> tracepoints. Tracepoints rely on `static_key_false` even though it is
> deprecated, so we add the same functionality to Rust.
>
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/static_key.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++++
> scripts/Makefile.build | 2 +-
> 3 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index d534b1178955..22e1fedd0774 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -39,6 +39,7 @@
> pub mod print;
> mod static_assert;
> pub mod static_call;
> +pub mod static_key;
> #[doc(hidden)]
> pub mod std_vendor;
> pub mod str;
> diff --git a/rust/kernel/static_key.rs b/rust/kernel/static_key.rs
> new file mode 100644
> index 000000000000..6c3dbe14c98a
> --- /dev/null
> +++ b/rust/kernel/static_key.rs
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0

This static key code is something that can be generally useful
both in kernel and user-space. Is there anything that would prevent
licensing this under MIT right away instead so it could eventually be
re-used more broadly in userspace as well ?

Thanks,

Mathieu

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

2024-06-06 16:22:35

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 6, 2024 at 5:29 PM Mathieu Desnoyers
<[email protected]> wrote:
>
> On 2024-06-06 11:05, Alice Ryhl wrote:
> > Make it possible to have Rust code call into tracepoints defined by C
> > code. It is still required that the tracepoint is declared in a C
> > header, and that this header is included in the input to bindgen.
> >
> > Signed-off-by: Alice Ryhl <[email protected]>
>
> [...]
>
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 2c37a0f5d7a8..0560cc2a512a 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
> > }
> > EXPORT_SYMBOL_GPL(rust_helper_krealloc);
> >
> > +void rust_helper_preempt_enable_notrace(void)
> > +{
> > + preempt_enable_notrace();
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
> > +
> > +void rust_helper_preempt_disable_notrace(void)
> > +{
> > + preempt_disable_notrace();
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);
> > +
> > +bool rust_helper_current_cpu_online(void)
> > +{
> > + return cpu_online(raw_smp_processor_id());
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
> > +
> > +void *rust_helper___rcu_dereference_raw(void **p)
> > +{
> > + return rcu_dereference_raw(p);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);
>
> Ouch. Doing a function call for each of those small operations will
> have a rather large performance impact when tracing is active. If it is
> not possible to inline those in Rust, then implementing __DO_TRACE in
> a C function would at least allow Rust to only do a single call to C
> rather than go back and forth between Rust and C.
>
> What prevents inlining those helpers in Rust ?

There's nothing fundamental that prevents it. But they contain a large
amount of architecture specific code, which makes them a significant
amount of work to reimplement in Rust.

For example, rcu_dereference_raw calls into READ_ONCE. READ_ONCE is
usually a volatile load, but under arm64+LTO, you get a bunch of
inline assembly that relies on ALTERNATIVE for feature detection:
https://elixir.bootlin.com/linux/v6.9/source/arch/arm64/include/asm/rwonce.h#L36

And preempt_enable_notrace has a similar story.

The solution that Boqun mentions is nice, but it relies on rustc and
clang using the same version of LLVM. You are unlikely to have
compilers with matching LLVMs unless you intentionally take steps to
make that happen.

But yes, perhaps these helpers are an argument to have a single call
for __DO_TRACE instead.

Alice

2024-06-06 16:23:45

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On 2024-06-06 12:16, Alice Ryhl wrote:
> On Thu, Jun 6, 2024 at 5:29 PM Mathieu Desnoyers
> <[email protected]> wrote:
>>
>> On 2024-06-06 11:05, Alice Ryhl wrote:
>>> Make it possible to have Rust code call into tracepoints defined by C
>>> code. It is still required that the tracepoint is declared in a C
>>> header, and that this header is included in the input to bindgen.
>>>
>>> Signed-off-by: Alice Ryhl <[email protected]>
>>
>> [...]
>>
>>> diff --git a/rust/helpers.c b/rust/helpers.c
>>> index 2c37a0f5d7a8..0560cc2a512a 100644
>>> --- a/rust/helpers.c
>>> +++ b/rust/helpers.c
>>> @@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
>>> }
>>> EXPORT_SYMBOL_GPL(rust_helper_krealloc);
>>>
>>> +void rust_helper_preempt_enable_notrace(void)
>>> +{
>>> + preempt_enable_notrace();
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
>>> +
>>> +void rust_helper_preempt_disable_notrace(void)
>>> +{
>>> + preempt_disable_notrace();
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);
>>> +
>>> +bool rust_helper_current_cpu_online(void)
>>> +{
>>> + return cpu_online(raw_smp_processor_id());
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
>>> +
>>> +void *rust_helper___rcu_dereference_raw(void **p)
>>> +{
>>> + return rcu_dereference_raw(p);
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);
>>
>> Ouch. Doing a function call for each of those small operations will
>> have a rather large performance impact when tracing is active. If it is
>> not possible to inline those in Rust, then implementing __DO_TRACE in
>> a C function would at least allow Rust to only do a single call to C
>> rather than go back and forth between Rust and C.
>>
>> What prevents inlining those helpers in Rust ?
>
> There's nothing fundamental that prevents it. But they contain a large
> amount of architecture specific code, which makes them a significant
> amount of work to reimplement in Rust.
>
> For example, rcu_dereference_raw calls into READ_ONCE. READ_ONCE is
> usually a volatile load, but under arm64+LTO, you get a bunch of
> inline assembly that relies on ALTERNATIVE for feature detection:
> https://elixir.bootlin.com/linux/v6.9/source/arch/arm64/include/asm/rwonce.h#L36
>
> And preempt_enable_notrace has a similar story.
>
> The solution that Boqun mentions is nice, but it relies on rustc and
> clang using the same version of LLVM. You are unlikely to have
> compilers with matching LLVMs unless you intentionally take steps to
> make that happen.
>
> But yes, perhaps these helpers are an argument to have a single call
> for __DO_TRACE instead.

If those helpers end up being inlined into Rust with the solution
pointed to by Boqun, then it makes sense to implement __DO_TRACE
in Rust. Otherwise doing a single call to C would be more efficient
than calling each of the helpers individually.

Thanks,

Mathieu

>
> Alice

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

2024-06-06 16:24:00

[permalink] [raw]

Subject: Re: [PATCH 2/3] rust: add static_key_false

On Thu, Jun 6, 2024 at 5:37 PM Mathieu Desnoyers
<[email protected]> wrote:
>
> On 2024-06-06 11:05, Alice Ryhl wrote:
> > Add just enough support for static key so that we can use it from
> > tracepoints. Tracepoints rely on `static_key_false` even though it is
> > deprecated, so we add the same functionality to Rust.
> >
> > Signed-off-by: Alice Ryhl <[email protected]>
> > ---
> > rust/kernel/lib.rs | 1 +
> > rust/kernel/static_key.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++++
> > scripts/Makefile.build | 2 +-
> > 3 files changed, 89 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index d534b1178955..22e1fedd0774 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -39,6 +39,7 @@
> > pub mod print;
> > mod static_assert;
> > pub mod static_call;
> > +pub mod static_key;
> > #[doc(hidden)]
> > pub mod std_vendor;
> > pub mod str;
> > diff --git a/rust/kernel/static_key.rs b/rust/kernel/static_key.rs
> > new file mode 100644
> > index 000000000000..6c3dbe14c98a
> > --- /dev/null
> > +++ b/rust/kernel/static_key.rs
> > @@ -0,0 +1,87 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> This static key code is something that can be generally useful
> both in kernel and user-space. Is there anything that would prevent
> licensing this under MIT right away instead so it could eventually be
> re-used more broadly in userspace as well ?

I would not mind.

Alice

2024-06-06 16:25:46

[permalink] [raw]

Subject: Re: [PATCH 0/3] Tracepoints and static branch/call in Rust

On 2024-06-06 11:46, Alice Ryhl wrote:
> On Thu, Jun 6, 2024 at 5:25 PM Mathieu Desnoyers
> <[email protected]> wrote:
>>
>> On 2024-06-06 11:05, Alice Ryhl wrote:
>>> This implementation implements support for static keys in Rust so that
>>> the actual static branch will end up in the Rust object file. However,
>>> it would also be possible to just wrap the trace_##name generated by
>>> __DECLARE_TRACE in an extern C function and then call that from Rust.
>>> This will simplify the Rust code by removing the need for static
>>> branches and calls, but it places the static branch behind an external
>>> call, which has performance implications.
>>
>> The tracepoints try very hard to minimize overhead of dormant tracepoints
>> so it is not frowned-upon to have them built into production binaries.
>> This is needed to make sure distribution vendors keep those tracepoints
>> in the kernel binaries that reach end-users.
>>
>> Adding a function call before evaluation of the static branch goes against
>> this major goal.
>>
>>>
>>> A possible middle ground would be to place just the __DO_TRACE body in
>>> an extern C function and to implement the Rust wrapper by doing the
>>> static branch in Rust, and then calling into C the code that contains
>>> __DO_TRACE when the tracepoint is active. However, this would need some
>>> changes to include/linux/tracepoint.h to generate and export a function
>>> containing the body of __DO_TRACE when the tracepoint should be callable
>>> from Rust.
>>
>> This tradeoff is more acceptable than having a function call before
>> evaluation of the static branch, but I wonder what is the upside of
>> this tradeoff compared to inlining the whole __DO_TRACE in Rust ?
>>
>>> So in general, there is a tradeoff between placing parts of the
>>> tracepoint (which is perf sensitive) behind an external call, and having
>>> code duplicated in both C and Rust (which must be kept in sync when
>>> changes are made). This is an important point that I would like feedback
>>> on from the C maintainers.
>>
>> I don't see how the duplication happens there: __DO_TRACE is meant to be
>> inlined into each C tracepoint caller site, so the code is already meant
>> to be duplicated. Having an explicit function wrapping the tracepoint
>> for Rust would just create an extra instance of __DO_TRACE if it happens
>> to be also inlined into C code.
>>
>> Or do you meant you would like to prevent having to duplicate the
>> implementation of __DO_TRACE in both C and Rust ?
>>
>> I'm not sure if you mean to prevent source code duplication between
>> C and Rust or duplication of binary code (instructions).
>
> It's a question of maintenance burden. If you change how __DO_TRACE is
> implemented, then those changes must also be reflected in the Rust
> version. There's no issue in the binary code.

As long as it is only __DO_TRACE that is duplicated between C and Rust,
I don't see it as a large burden.

Thanks,

Mathieu

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

2024-06-06 17:12:48

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On 2024-06-06 11:49, Boqun Feng wrote:
> On Thu, Jun 06, 2024 at 11:30:03AM -0400, Mathieu Desnoyers wrote:
>> On 2024-06-06 11:05, Alice Ryhl wrote:
>>> Make it possible to have Rust code call into tracepoints defined by C
>>> code. It is still required that the tracepoint is declared in a C
>>> header, and that this header is included in the input to bindgen.
>>>
>>> Signed-off-by: Alice Ryhl <[email protected]>
>>
>> [...]
>>
>>> diff --git a/rust/helpers.c b/rust/helpers.c
>>> index 2c37a0f5d7a8..0560cc2a512a 100644
>>> --- a/rust/helpers.c
>>> +++ b/rust/helpers.c
>>> @@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
>>> }
>>> EXPORT_SYMBOL_GPL(rust_helper_krealloc);
>>> +void rust_helper_preempt_enable_notrace(void)
>>> +{
>>> + preempt_enable_notrace();
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
>>> +
>>> +void rust_helper_preempt_disable_notrace(void)
>>> +{
>>> + preempt_disable_notrace();
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);
>>> +
>>> +bool rust_helper_current_cpu_online(void)
>>> +{
>>> + return cpu_online(raw_smp_processor_id());
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
>>> +
>>> +void *rust_helper___rcu_dereference_raw(void **p)
>>> +{
>>> + return rcu_dereference_raw(p);
>>> +}
>>> +EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);
>>
>> Ouch. Doing a function call for each of those small operations will
>> have a rather large performance impact when tracing is active. If it is
>
> Long-term plan is to 1) compile the C helpers in some IR and 2) inline
> the helpers with Rust in IR-level, as what Gary has:
>
> https://lore.kernel.org/rust-for-linux/[email protected]/
>
> and I use it for the upcoming atomic API support:
>
> https://github.com/fbq/linux/tree/dev/rust/atomic-rfc
>
> and it works very well.

Thanks for the pointers, it makes sense.

Thanks,

Mathieu

>
> Regards,
> Boqun
>
>> not possible to inline those in Rust, then implementing __DO_TRACE in
>> a C function would at least allow Rust to only do a single call to C
>> rather than go back and forth between Rust and C.
>>
>> What prevents inlining those helpers in Rust ?
>>
>> Thanks,
>>
>> Mathieu
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> https://www.efficios.com
>>

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

2024-06-06 17:19:16

[permalink] [raw]

Subject: Re: [PATCH 1/3] rust: add static_call support

On Thu, Jun 06, 2024 at 03:05:24PM +0000, Alice Ryhl wrote:
> Add static_call support by mirroring how C does. When the platform does
> not support static calls (right now, that means that it is not x86),
> then the function pointer is loaded from a global and called. Otherwise,
> we generate a call to a trampoline function, and objtool is used to make
> these calls patchable at runtime.

This is absolutely unreadable gibberish -- how am I supposed to keep
this in sync with the rest of the static_call infrastructure?

> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/static_call.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 93 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fbd91a48ff8b..d534b1178955 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -38,6 +38,7 @@
> pub mod prelude;
> pub mod print;
> mod static_assert;
> +pub mod static_call;
> #[doc(hidden)]
> pub mod std_vendor;
> pub mod str;
> diff --git a/rust/kernel/static_call.rs b/rust/kernel/static_call.rs
> new file mode 100644
> index 000000000000..f7b8ba7bf1fb
> --- /dev/null
> +++ b/rust/kernel/static_call.rs
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Logic for static calls.
> +
> +#[macro_export]
> +#[doc(hidden)]
> +macro_rules! ty_underscore_for {
> + ($arg:expr) => {
> + _
> + };
> +}
> +
> +#[doc(hidden)]
> +#[repr(transparent)]
> +pub struct AddressableStaticCallKey {
> + _ptr: *const bindings::static_call_key,
> +}
> +unsafe impl Sync for AddressableStaticCallKey {}
> +impl AddressableStaticCallKey {
> + pub const fn new(ptr: *const bindings::static_call_key) -> Self {
> + Self { _ptr: ptr }
> + }
> +}
> +
> +#[cfg(CONFIG_HAVE_STATIC_CALL)]
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! _static_call {
> + ($name:ident($($args:expr),* $(,)?)) => {{
> + // Symbol mangling will give this symbol a unique name.
> + #[cfg(CONFIG_HAVE_STATIC_CALL_INLINE)]
> + #[link_section = ".discard.addressable"]
> + #[used]
> + static __ADDRESSABLE: $crate::static_call::AddressableStaticCallKey = unsafe {
> + $crate::static_call::AddressableStaticCallKey::new(::core::ptr::addr_of!(
> + $crate::macros::paste! { $crate::bindings:: [<__SCK__ $name >]; }
> + ))
> + };
> +
> + let fn_ptr: unsafe extern "C" fn($($crate::static_call::ty_underscore_for!($args)),*) -> _ =
> + $crate::macros::paste! { $crate::bindings:: [<__SCT__ $name >]; };
> + (fn_ptr)($($args),*)
> + }};
> +}
> +
> +#[cfg(not(CONFIG_HAVE_STATIC_CALL))]
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! _static_call {
> + ($name:ident($($args:expr),* $(,)?)) => {{
> + let void_ptr_fn: *mut ::core::ffi::c_void =
> + $crate::macros::paste! { $crate::bindings:: [<__SCK__ $name >]; }.func;
> +
> + let fn_ptr: unsafe extern "C" fn($($crate::static_call::ty_underscore_for!($args)),*) -> _ =
> + if true {
> + ::core::mem::transmute(void_ptr_fn)
> + } else {
> + // This is dead code, but it influences type inference on `fn_ptr` so that we
> + // transmute the function pointer to the right type.
> + $crate::macros::paste! { $crate::bindings:: [<__SCT__ $name >]; }
> + };
> +
> + (fn_ptr)($($args),*)
> + }};
> +}
> +
> +/// Statically call a global function.
> +///
> +/// # Safety
> +///
> +/// This macro will call the provided function. It is up to the caller to uphold the safety
> +/// guarantees of the function.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// fn call_static() {
> +/// unsafe {
> +/// static_call! { your_static_call() };
> +/// }
> +/// }
> +/// ```
> +#[macro_export]
> +macro_rules! static_call {
> + // Forward to the real implementation. Separated like this so that we don't have to duplicate
> + // the documentation.
> + ($($args:tt)*) => { $crate::static_call::_static_call! { $($args)* } };
> +}
> +
> +pub use {_static_call, static_call, ty_underscore_for};
>
> --
> 2.45.2.505.gda0bf45e8d-goog
>

2024-06-06 17:26:39

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 06, 2024 at 03:05:26PM +0000, Alice Ryhl wrote:
> Make it possible to have Rust code call into tracepoints defined by C
> code. It is still required that the tracepoint is declared in a C
> header, and that this header is included in the input to bindgen.
>
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/bindings/lib.rs | 15 +++++++
> rust/helpers.c | 24 +++++++++++
> rust/kernel/lib.rs | 1 +
> rust/kernel/tracepoint.rs | 92 +++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 133 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index ddb5644d4fd9..d442f9ccfc2c 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,6 +15,7 @@
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> +#include <linux/tracepoint.h>
> #include <linux/wait.h>
> #include <linux/workqueue.h>
>
> diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
> index 40ddaee50d8b..48856761d682 100644
> --- a/rust/bindings/lib.rs
> +++ b/rust/bindings/lib.rs
> @@ -48,3 +48,18 @@ mod bindings_helper {
> }
>
> pub use bindings_raw::*;
> +
> +/// Rust version of the C macro `rcu_dereference_raw`.
> +///
> +/// The rust helper only works with void pointers, but this wrapper method makes it work with any
> +/// pointer type using pointer casts.
> +///
> +/// # Safety
> +///
> +/// This method has the same safety requirements as the C macro of the same name.
> +#[inline(always)]
> +pub unsafe fn rcu_dereference_raw<T>(p: *const *mut T) -> *mut T {
> + // SAFETY: This helper calls into the C macro, so the caller promises to uphold the safety
> + // requirements.
> + unsafe { __rcu_dereference_raw(p as *mut *mut _) as *mut T }
> +}
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 2c37a0f5d7a8..0560cc2a512a 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -165,6 +165,30 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
> }
> EXPORT_SYMBOL_GPL(rust_helper_krealloc);
>
> +void rust_helper_preempt_enable_notrace(void)
> +{
> + preempt_enable_notrace();
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_preempt_enable_notrace);
> +
> +void rust_helper_preempt_disable_notrace(void)
> +{
> + preempt_disable_notrace();
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_preempt_disable_notrace);

A notrace wrapper that is tracable, lol.

> +bool rust_helper_current_cpu_online(void)
> +{
> + return cpu_online(raw_smp_processor_id());
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_current_cpu_online);
> +
> +void *rust_helper___rcu_dereference_raw(void **p)
> +{
> + return rcu_dereference_raw(p);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper___rcu_dereference_raw);

I'm going to keep yelling and objecting to these wrappers.

Fix bindgen already. Or whatever is needed to get it to interoperate
with C inline / asm.

NAK NAK NAK

2024-06-06 18:41:28

[permalink] [raw]

Subject: Re: [PATCH 2/3] rust: add static_key_false

On Thu, Jun 06, 2024 at 03:05:25PM +0000, Alice Ryhl wrote:
> Add just enough support for static key so that we can use it from
> tracepoints. Tracepoints rely on `static_key_false` even though it is
> deprecated, so we add the same functionality to Rust.

Urgh, more unreadable gibberish :-(

Can we please get bindgen to translate asm/jump_table.h instead of
randomly duplicating random bits.

I really think that whoever created rust was an esoteric language freak.
Hideous crap :-(.

2024-06-06 19:09:31

by Miguel Ojeda

[permalink] [raw]

Subject: Re: [PATCH 1/3] rust: add static_call support

On Thu, Jun 6, 2024 at 7:19 PM Peter Zijlstra <[email protected]> wrote:
>
> This is absolutely unreadable gibberish -- how am I supposed to keep
> this in sync with the rest of the static_call infrastructure?

Yeah, they are macros, which look different from "normal" Rust code.

Is there something we could do to help here? I think Alice and others
would be happy to explain how it works and/or help maintain it in the
future if you prefer.

Cheers,
Miguel

2024-06-06 19:20:46

by Boqun Feng

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 06, 2024 at 07:35:44PM +0200, Peter Zijlstra wrote:
> On Thu, Jun 06, 2024 at 08:49:06AM -0700, Boqun Feng wrote:
>
> > Long-term plan is to 1) compile the C helpers in some IR and 2) inline
> > the helpers with Rust in IR-level, as what Gary has:
> >
> > https://lore.kernel.org/rust-for-linux/[email protected]/
>
> Urgh, that still needs us to maintain that silly list of helpers :-/
>

But it's an improvement from the current stage, right? ;-)

> Can't we pretty please have clang parse the actual header files into IR
> and munge that into rust? So that we don't get to manually duplicate
> everything+dog.

That won't always work, because some of our kernel APIs are defined as
macros, and I don't think it's a trivial job to generate a macro
definition to a function definition so that it can be translated to
something in IR. We will have to do the macro -> function mapping
ourselves somewhere, if we want to inline the API across languages.

Regards,
Boqun

2024-06-06 19:30:14

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 06, 2024 at 12:00:36PM -0700, Boqun Feng wrote:
> On Thu, Jun 06, 2024 at 07:35:44PM +0200, Peter Zijlstra wrote:
> > On Thu, Jun 06, 2024 at 08:49:06AM -0700, Boqun Feng wrote:
> >
> > > Long-term plan is to 1) compile the C helpers in some IR and 2) inline
> > > the helpers with Rust in IR-level, as what Gary has:
> > >
> > > https://lore.kernel.org/rust-for-linux/[email protected]/
> >
> > Urgh, that still needs us to maintain that silly list of helpers :-/
> >
>
> But it's an improvement from the current stage, right? ;-)

Somewhat, but only marginal.

> > Can't we pretty please have clang parse the actual header files into IR
> > and munge that into rust? So that we don't get to manually duplicate
> > everything+dog.
>
> That won't always work, because some of our kernel APIs are defined as
> macros, and I don't think it's a trivial job to generate a macro
> definition to a function definition so that it can be translated to
> something in IR. We will have to do the macro -> function mapping
> ourselves somewhere, if we want to inline the API across languages.

We can try and see how far we can get with moving a bunch of stuff into
inlines. There's quite a bit of simple CPP that could be inlines or
const objects I suppose.

Things like the tracepoints are of course glorious CPP abuse and are
never going to work.

But perhaps you can have an explicit 'eval-CPP on this here' construct
or whatnot. If I squit I see this paste! thingy (WTF's up with that !
operator?) to munge function names in the static_call thing. So
something like apply CPP from over there on this here can also be done
:-)

2024-06-06 21:13:12

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 06, 2024 at 08:49:06AM -0700, Boqun Feng wrote:

> Long-term plan is to 1) compile the C helpers in some IR and 2) inline
> the helpers with Rust in IR-level, as what Gary has:
>
> https://lore.kernel.org/rust-for-linux/[email protected]/

Urgh, that still needs us to maintain that silly list of helpers :-/

Can't we pretty please have clang parse the actual header files into IR
and munge that into rust? So that we don't get to manually duplicate
everything+dog.

2024-06-06 22:33:28

[permalink] [raw]

Subject: Re: [PATCH 1/3] rust: add static_call support

On Thu, Jun 06, 2024 at 09:09:00PM +0200, Miguel Ojeda wrote:
> On Thu, Jun 6, 2024 at 7:19 PM Peter Zijlstra <[email protected]> wrote:
> >
> > This is absolutely unreadable gibberish -- how am I supposed to keep
> > this in sync with the rest of the static_call infrastructure?
>
> Yeah, they are macros, which look different from "normal" Rust code.

Macros like CPP ?

> Is there something we could do to help here? I think Alice and others
> would be happy to explain how it works and/or help maintain it in the
> future if you prefer.

Write a new language that looks more like C -- pretty please ? :-)

Mostly I would just really like you to just use arm/jump_label.h,
they're inline functions and should be doable with IR, no weirdo CPP
involved in this case.

2024-06-06 23:51:18

by Boqun Feng

[permalink] [raw]

Subject: Re: [PATCH 3/3] rust: add tracepoint support

On Thu, Jun 06, 2024 at 09:29:51PM +0200, Peter Zijlstra wrote:
> On Thu, Jun 06, 2024 at 12:00:36PM -0700, Boqun Feng wrote:
> > On Thu, Jun 06, 2024 at 07:35:44PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jun 06, 2024 at 08:49:06AM -0700, Boqun Feng wrote:
> > >
> > > > Long-term plan is to 1) compile the C helpers in some IR and 2) inline
> > > > the helpers with Rust in IR-level, as what Gary has:
> > > >
> > > > https://lore.kernel.org/rust-for-linux/[email protected]/
> > >
> > > Urgh, that still needs us to maintain that silly list of helpers :-/
> > >
> >
> > But it's an improvement from the current stage, right? ;-)
>
> Somewhat, but only marginal.
>
> > > Can't we pretty please have clang parse the actual header files into IR
> > > and munge that into rust? So that we don't get to manually duplicate
> > > everything+dog.
> >
> > That won't always work, because some of our kernel APIs are defined as
> > macros, and I don't think it's a trivial job to generate a macro
> > definition to a function definition so that it can be translated to
> > something in IR. We will have to do the macro -> function mapping
> > ourselves somewhere, if we want to inline the API across languages.
>
> We can try and see how far we can get with moving a bunch of stuff into
> inlines. There's quite a bit of simple CPP that could be inlines or
> const objects I suppose.
>

We can, but I'd first stick with what we have, improve it and make it
stable until we go to the next stage. Plus, there's benefit of keeping
an explicit helper list: it's clear what APIs are called by Rust, and
moreover, it's easier to modify the helpers if you were to change an
API, other than chasing where Rust code calls it. (Don't make me wrong,
I'm happy if you want to do that ;-))

Regards,
Boqun

> Things like the tracepoints are of course glorious CPP abuse and are
> never going to work.
>
> But perhaps you can have an explicit 'eval-CPP on this here' construct
> or whatnot. If I squit I see this paste! thingy (WTF's up with that !
> operator?) to munge function names in the static_call thing. So
> something like apply CPP from over there on this here can also be done
> :-)

2024-06-07 09:43:47

[permalink] [raw]

Subject: Re: [PATCH 1/3] rust: add static_call support

Peter Zijlstra <[email protected]> wrote:
> On Thu, Jun 06, 2024 at 09:09:00PM +0200, Miguel Ojeda wrote:
> > On Thu, Jun 6, 2024 at 7:19 PM Peter Zijlstra <[email protected]> wrote:
> > >
> > > This is absolutely unreadable gibberish -- how am I supposed to keep
> > > this in sync with the rest of the static_call infrastructure?
> >
> > Yeah, they are macros, which look different from "normal" Rust code.
>
> Macros like CPP ?

Yes, this patch series uses declarative macros, which are the closest
that Rust has to the C preprocessor. They are powerful, but just like
CPP, they can become pretty complicated and hard to read if you are
doing something non-trivial.

The macro_rules! block is how you define a new declarative macro.

The ($name:ident($($args:expr),* $(,)?)) part defines the arguments to
the declarative macro. This syntax means:

1. The input starts with any identifier, which we call $name.
2. Then there must be a ( token.
3. Then the $(),* part defines a repetition group. The contents inside
the parenthesises are what is being repeated. The comma means that
repetitions are separated by commas. The asterisk means that the
contents may be repeated any number of times, including zero times.
(Alternatives are + which means repeated at least once, and ? which
means that the contents may be repeated zero or one time.)
4. The contents of the repetition group will be an expression, which we
call $args.
5. After the last expression, we have $(,)? which means that you can
have zero or one commas after the last expression. Rust usually
allows trailing commas in lists, which is why I included it.
6. And finally, you must close the input with a ) token.

So for example, you might invoke the macro like this:

static_call!(tp_func_my_tracepoint(__data, &mut my_task_struct));

Here, $name will be tp_func_my_tracepoint, and the repetition group is
repeated twice, with $args first corresponding to `__data` and then to
`&mut my_task_struct` when the macro is expanded. The $(,)? group is
repeated zero times.

Inside the macro, you will see things such as:
$crate::macros::paste! { $crate::bindings:: [<__SCK__ $name >]; }

The Rust syntax for invoking a macro has an exclamation mark after the
name, so you know that $crate::macros::paste is a macro. The `paste`
macro just emits its input unchanged, except that any identifiers
between [< and >] are concatenated into a single identifier. So if $name
is my_static_key, then the above invocation of paste! emits:

$crate::bindings::__SCK__my_static_key;

The $crate::bindings module is where the output of bindgen goes, so this
should correspond to the C symbol called __SCK__my_static_key.

> > Is there something we could do to help here? I think Alice and others
> > would be happy to explain how it works and/or help maintain it in the
> > future if you prefer.
>
> Write a new language that looks more like C -- pretty please ? :-)
>
> Mostly I would just really like you to just use arm/jump_label.h,
> they're inline functions and should be doable with IR, no weirdo CPP
> involved in this case.

I assume that you're referring to static_key_false here? I don't think
that function can be exposed using IR because it passes the function
argument called key as an "i" argument to an inline assembly block. Any
attempt to compile static_key_false without knowing the value of key at
compile time will surely fail to compile with the

invalid operand for inline asm constraint 'i'

error.

Alice

2024-06-07 10:53:13

[permalink] [raw]

Subject: Re: [PATCH 1/3] rust: add static_call support

On Fri, Jun 07, 2024 at 09:43:29AM +0000, Alice Ryhl wrote:
> Peter Zijlstra <[email protected]> wrote:
> > On Thu, Jun 06, 2024 at 09:09:00PM +0200, Miguel Ojeda wrote:
> > > On Thu, Jun 6, 2024 at 7:19 PM Peter Zijlstra <[email protected]> wrote:
> > > >
> > > > This is absolutely unreadable gibberish -- how am I supposed to keep
> > > > this in sync with the rest of the static_call infrastructure?
> > >
> > > Yeah, they are macros, which look different from "normal" Rust code.
> >
> > Macros like CPP ?
>
> Yes, this patch series uses declarative macros, which are the closest
> that Rust has to the C preprocessor. They are powerful, but just like
> CPP, they can become pretty complicated and hard to read if you are
> doing something non-trivial.
>
> The macro_rules! block is how you define a new declarative macro.

I'm sorry, but 30+ years of reading ! as NOT (or factorial) isn't going
to go away. So I'm reading your macros do NOT rule.

> The ($name:ident($($args:expr),* $(,)?)) part defines the arguments to
> the declarative macro. This syntax means:
>
> 1. The input starts with any identifier, which we call $name.
> 2. Then there must be a ( token.

The above exaple fails, because the next token is :ident, whatever the
heck that might be. Also, extra points for line-noise due to lack of
whitespace.

> So for example, you might invoke the macro like this:
>
> static_call!(tp_func_my_tracepoint(__data, &mut my_task_struct));

static_call NOT (blah dog blah);

> Inside the macro, you will see things such as:
> $crate::macros::paste! { $crate::bindings:: [<__SCK__ $name >]; }
>
> The Rust syntax for invoking a macro has an exclamation mark after the

Like I said before, the creator of Rust must've been an esoteric
language freak and must've wanted to make this unreadable on purpose :/

Also, why the white space beteen the :: scope operator and the [< thing?
that's just weird. I would then expect the output to be:

...::bindings:: __SCK__my_static_key

> name, so you know that $crate::macros::paste is a macro. The `paste`
> macro just emits its input unchanged, except that any identifiers
> between [< and >] are concatenated into a single identifier. So if $name
> is my_static_key, then the above invocation of paste! emits:
>
> $crate::bindings::__SCK__my_static_key;

But it doesn't, so it isn't unmodified, it seems to strip whitespace.

> The $crate::bindings module is where the output of bindgen goes, so this
> should correspond to the C symbol called __SCK__my_static_key.
>
> > > Is there something we could do to help here? I think Alice and others
> > > would be happy to explain how it works and/or help maintain it in the
> > > future if you prefer.
> >
> > Write a new language that looks more like C -- pretty please ? :-)
> >
> > Mostly I would just really like you to just use arm/jump_label.h,
> > they're inline functions and should be doable with IR, no weirdo CPP
> > involved in this case.
>
> I assume that you're referring to static_key_false here? I don't think
> that function can be exposed using IR because it passes the function
> argument called key as an "i" argument to an inline assembly block. Any
> attempt to compile static_key_false without knowing the value of key at
> compile time will surely fail to compile with the
>
> invalid operand for inline asm constraint 'i'
>
> error.

You can have clang read the header files and compile them into
Intermediate-Representation, and have it splice the lot into the Rust
crap's IR and voila, compile time.

You just need to extend the rust thing to be able to consume C header
files.

2024-06-07 11:09:16