| Message ID | 20241001-tracepoint-v9-1-1ad3b7d78acb@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Tracepoints and static branch in Rust | expand |
On Tue, 01 Oct 2024 13:29:58 +0000 Alice Ryhl <aliceryhl@google.com> 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. > > This patch only provides a generic implementation without code patching > (matching the one used when CONFIG_JUMP_LABEL is disabled). Later > patches add support for inline asm implementations that use runtime > patching. > > When CONFIG_JUMP_LABEL is unset, `static_key_count` is a static inline > function, so a Rust helper is defined for `static_key_count` in this > case. If Rust is compiled with LTO, this call should get inlined. The > helper can be eliminated once we have the necessary inline asm to make > atomic operations from Rust. > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/helpers/helpers.c | 1 + > rust/helpers/jump_label.c | 15 +++++++++++++++ > rust/kernel/jump_label.rs | 29 +++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 5 files changed, 47 insertions(+) > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index ae82e9c941af..e0846e7e93e6 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -14,6 +14,7 @@ > #include <linux/ethtool.h> > #include <linux/firmware.h> > #include <linux/jiffies.h> > +#include <linux/jump_label.h> > #include <linux/mdio.h> > #include <linux/phy.h> > #include <linux/refcount.h> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c > index 30f40149f3a9..17e1b60d178f 100644 > --- a/rust/helpers/helpers.c > +++ b/rust/helpers/helpers.c > @@ -12,6 +12,7 @@ > #include "build_assert.c" > #include "build_bug.c" > #include "err.c" > +#include "jump_label.c" > #include "kunit.c" > #include "mutex.c" > #include "page.c" > diff --git a/rust/helpers/jump_label.c b/rust/helpers/jump_label.c > new file mode 100644 > index 000000000000..0e9ed15903f6 > --- /dev/null > +++ b/rust/helpers/jump_label.c > @@ -0,0 +1,15 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Copyright (C) 2024 Google LLC. > + */ > + > +#include <linux/jump_label.h> > + Nit, the above line has a spurious space. -- Steve > +#ifndef CONFIG_JUMP_LABEL > +int rust_helper_static_key_count(struct static_key *key) > +{ > + return static_key_count(key); > +} > +EXPORT_SYMBOL_GPL(rust_helper_static_key_count); > +#endif > diff --git a/rust/kernel/jump_label.rs b/rust/kernel/jump_label.rs > new file mode 100644
On Tue, Oct 1, 2024 at 4:11 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 01 Oct 2024 13:29:58 +0000 > Alice Ryhl <aliceryhl@google.com> 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. > > > > This patch only provides a generic implementation without code patching > > (matching the one used when CONFIG_JUMP_LABEL is disabled). Later > > patches add support for inline asm implementations that use runtime > > patching. > > > > When CONFIG_JUMP_LABEL is unset, `static_key_count` is a static inline > > function, so a Rust helper is defined for `static_key_count` in this > > case. If Rust is compiled with LTO, this call should get inlined. The > > helper can be eliminated once we have the necessary inline asm to make > > atomic operations from Rust. > > > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > --- > > rust/bindings/bindings_helper.h | 1 + > > rust/helpers/helpers.c | 1 + > > rust/helpers/jump_label.c | 15 +++++++++++++++ > > rust/kernel/jump_label.rs | 29 +++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 1 + > > 5 files changed, 47 insertions(+) > > > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > > index ae82e9c941af..e0846e7e93e6 100644 > > --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -14,6 +14,7 @@ > > #include <linux/ethtool.h> > > #include <linux/firmware.h> > > #include <linux/jiffies.h> > > +#include <linux/jump_label.h> > > #include <linux/mdio.h> > > #include <linux/phy.h> > > #include <linux/refcount.h> > > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c > > index 30f40149f3a9..17e1b60d178f 100644 > > --- a/rust/helpers/helpers.c > > +++ b/rust/helpers/helpers.c > > @@ -12,6 +12,7 @@ > > #include "build_assert.c" > > #include "build_bug.c" > > #include "err.c" > > +#include "jump_label.c" > > #include "kunit.c" > > #include "mutex.c" > > #include "page.c" > > diff --git a/rust/helpers/jump_label.c b/rust/helpers/jump_label.c > > new file mode 100644 > > index 000000000000..0e9ed15903f6 > > --- /dev/null > > +++ b/rust/helpers/jump_label.c > > @@ -0,0 +1,15 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Copyright (C) 2024 Google LLC. > > + */ > > + > > +#include <linux/jump_label.h> > > + > > Nit, the above line has a spurious space. Thanks. Looks like the function body also uses 7 spaces instead of a tab. I'll fix it. Alice
On Tue, Oct 01, 2024 at 01:29:58PM +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. Instead of extending the old deprecated static key interface into Rust, can we just change tracepoints to use the new one? /me makes a note to go convert the other users... From: Josh Poimboeuf <jpoimboe@kernel.org> Subject: [PATCH] tracepoints: Use new static branch API The old static key API based on 'struct static_key' is deprecated. Convert tracepoints to use the new API. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- include/linux/tracepoint-defs.h | 4 ++-- include/linux/tracepoint.h | 8 ++++---- kernel/tracepoint.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h index 4dc4955f0fbf..60a6e8314d4c 100644 --- a/include/linux/tracepoint-defs.h +++ b/include/linux/tracepoint-defs.h @@ -31,7 +31,7 @@ struct tracepoint_func { struct tracepoint { const char *name; /* Tracepoint name */ - struct static_key key; + struct static_key_false key; struct static_call_key *static_call_key; void *static_call_tramp; void *iterator; @@ -83,7 +83,7 @@ struct bpf_raw_event_map { #ifdef CONFIG_TRACEPOINTS # define tracepoint_enabled(tp) \ - static_key_false(&(__tracepoint_##tp).key) + static_branch_unlikely(&(__tracepoint_##tp).key) #else # define tracepoint_enabled(tracepoint) false #endif diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index 6be396bb4297..ab5162fc3e4a 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -228,7 +228,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) #define __DECLARE_TRACE_RCU(name, proto, args, cond) \ static inline void trace_##name##_rcuidle(proto) \ { \ - if (static_key_false(&__tracepoint_##name.key)) \ + if (static_branch_unlikely(&__tracepoint_##name.key)) \ __DO_TRACE(name, \ TP_ARGS(args), \ TP_CONDITION(cond), 1); \ @@ -254,7 +254,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) extern struct tracepoint __tracepoint_##name; \ static inline void trace_##name(proto) \ { \ - if (static_key_false(&__tracepoint_##name.key)) \ + if (static_branch_unlikely(&__tracepoint_##name.key)) \ __DO_TRACE(name, \ TP_ARGS(args), \ TP_CONDITION(cond), 0); \ @@ -291,7 +291,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) static inline bool \ trace_##name##_enabled(void) \ { \ - return static_key_false(&__tracepoint_##name.key); \ + return static_branch_unlikely(&__tracepoint_##name.key);\ } /* @@ -308,7 +308,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) struct tracepoint __tracepoint_##_name __used \ __section("__tracepoints") = { \ .name = __tpstrtab_##_name, \ - .key = STATIC_KEY_INIT_FALSE, \ + .key = STATIC_KEY_FALSE_INIT, \ .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \ .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \ .iterator = &__traceiter_##_name, \ diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 8d1507dd0724..dc160cc0b616 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -358,7 +358,7 @@ static int tracepoint_add_func(struct tracepoint *tp, tracepoint_update_call(tp, tp_funcs); /* Both iterator and static call handle NULL tp->funcs */ rcu_assign_pointer(tp->funcs, tp_funcs); - static_key_enable(&tp->key); + static_branch_enable(&tp->key); break; case TP_FUNC_2: /* 1->2 */ /* Set iterator static call */ @@ -414,7 +414,7 @@ static int tracepoint_remove_func(struct tracepoint *tp, if (tp->unregfunc && static_key_enabled(&tp->key)) tp->unregfunc(); - static_key_disable(&tp->key); + static_branch_disable(&tp->key); /* Set iterator static call */ tracepoint_update_call(tp, tp_funcs); /* Both iterator and static call handle NULL tp->funcs */
On Tue, 1 Oct 2024 14:15:43 -0700 Josh Poimboeuf <jpoimboe@kernel.org> wrote: > On Tue, Oct 01, 2024 at 01:29:58PM +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. > > Instead of extending the old deprecated static key interface into Rust, > can we just change tracepoints to use the new one? Sure, care to send the patch properly, and I can add it to my queue. -- Steve > > /me makes a note to go convert the other users... > > From: Josh Poimboeuf <jpoimboe@kernel.org> > Subject: [PATCH] tracepoints: Use new static branch API > > The old static key API based on 'struct static_key' is deprecated. > Convert tracepoints to use the new API. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > include/linux/tracepoint-defs.h | 4 ++-- > include/linux/tracepoint.h | 8 ++++---- > kernel/tracepoint.c | 4 ++-- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h > index 4dc4955f0fbf..60a6e8314d4c 100644 > --- a/include/linux/tracepoint-defs.h > +++ b/include/linux/tracepoint-defs.h > @@ -31,7 +31,7 @@ struct tracepoint_func { > > struct tracepoint { > const char *name; /* Tracepoint name */ > - struct static_key key; > + struct static_key_false key; > struct static_call_key *static_call_key; > void *static_call_tramp; > void *iterator; > @@ -83,7 +83,7 @@ struct bpf_raw_event_map { > > #ifdef CONFIG_TRACEPOINTS > # define tracepoint_enabled(tp) \ > - static_key_false(&(__tracepoint_##tp).key) > + static_branch_unlikely(&(__tracepoint_##tp).key) > #else > # define tracepoint_enabled(tracepoint) false > #endif > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index 6be396bb4297..ab5162fc3e4a 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -228,7 +228,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > #define __DECLARE_TRACE_RCU(name, proto, args, cond) \ > static inline void trace_##name##_rcuidle(proto) \ > { \ > - if (static_key_false(&__tracepoint_##name.key)) \ > + if (static_branch_unlikely(&__tracepoint_##name.key)) \ > __DO_TRACE(name, \ > TP_ARGS(args), \ > TP_CONDITION(cond), 1); \ > @@ -254,7 +254,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > extern struct tracepoint __tracepoint_##name; \ > static inline void trace_##name(proto) \ > { \ > - if (static_key_false(&__tracepoint_##name.key)) \ > + if (static_branch_unlikely(&__tracepoint_##name.key)) \ > __DO_TRACE(name, \ > TP_ARGS(args), \ > TP_CONDITION(cond), 0); \ > @@ -291,7 +291,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > static inline bool \ > trace_##name##_enabled(void) \ > { \ > - return static_key_false(&__tracepoint_##name.key); \ > + return static_branch_unlikely(&__tracepoint_##name.key);\ > } > > /* > @@ -308,7 +308,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > struct tracepoint __tracepoint_##_name __used \ > __section("__tracepoints") = { \ > .name = __tpstrtab_##_name, \ > - .key = STATIC_KEY_INIT_FALSE, \ > + .key = STATIC_KEY_FALSE_INIT, \ > .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \ > .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \ > .iterator = &__traceiter_##_name, \ > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 8d1507dd0724..dc160cc0b616 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -358,7 +358,7 @@ static int tracepoint_add_func(struct tracepoint *tp, > tracepoint_update_call(tp, tp_funcs); > /* Both iterator and static call handle NULL tp->funcs */ > rcu_assign_pointer(tp->funcs, tp_funcs); > - static_key_enable(&tp->key); > + static_branch_enable(&tp->key); > break; > case TP_FUNC_2: /* 1->2 */ > /* Set iterator static call */ > @@ -414,7 +414,7 @@ static int tracepoint_remove_func(struct tracepoint *tp, > if (tp->unregfunc && static_key_enabled(&tp->key)) > tp->unregfunc(); > > - static_key_disable(&tp->key); > + static_branch_disable(&tp->key); > /* Set iterator static call */ > tracepoint_update_call(tp, tp_funcs); > /* Both iterator and static call handle NULL tp->funcs */
On Tue, Oct 01, 2024 at 05:46:06PM -0400, Steven Rostedt wrote: > On Tue, 1 Oct 2024 14:15:43 -0700 > Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > On Tue, Oct 01, 2024 at 01:29:58PM +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. > > > > Instead of extending the old deprecated static key interface into Rust, > > can we just change tracepoints to use the new one? > > Sure, care to send the patch properly, and I can add it to my queue. Done: https://lore.kernel.org/03da49e81c50eb2a9f47918409e4c590c0f28060.1727817249.git.jpoimboe@kernel.org
On Tue, 1 Oct 2024 14:15:43 -0700 Josh Poimboeuf <jpoimboe@kernel.org> wrote: > On Tue, Oct 01, 2024 at 01:29:58PM +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. > > Instead of extending the old deprecated static key interface into Rust, > can we just change tracepoints to use the new one? > > /me makes a note to go convert the other users... > > From: Josh Poimboeuf <jpoimboe@kernel.org> > Subject: [PATCH] tracepoints: Use new static branch API > > The old static key API based on 'struct static_key' is deprecated. > Convert tracepoints to use the new API. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> Alice, Can you send a v10 with the added acks and whitespace fixes as well as using static_branch_unlikely(), and I'll pull it into my tree. Base it off of v6.12-rc2. Thanks, -- Steve
On Thu, Oct 10, 2024 at 9:43 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 1 Oct 2024 14:15:43 -0700 > Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > On Tue, Oct 01, 2024 at 01:29:58PM +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. > > > > Instead of extending the old deprecated static key interface into Rust, > > can we just change tracepoints to use the new one? > > > > /me makes a note to go convert the other users... > > > > From: Josh Poimboeuf <jpoimboe@kernel.org> > > Subject: [PATCH] tracepoints: Use new static branch API > > > > The old static key API based on 'struct static_key' is deprecated. > > Convert tracepoints to use the new API. > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > Alice, > > Can you send a v10 with the added acks and whitespace fixes as well as > using static_branch_unlikely(), and I'll pull it into my tree. > > Base it off of v6.12-rc2. Here you go: https://lore.kernel.org/all/20241011-tracepoint-v10-0-7fbde4d6b525@google.com/ Alice
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index ae82e9c941af..e0846e7e93e6 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -14,6 +14,7 @@ #include <linux/ethtool.h> #include <linux/firmware.h> #include <linux/jiffies.h> +#include <linux/jump_label.h> #include <linux/mdio.h> #include <linux/phy.h> #include <linux/refcount.h> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index 30f40149f3a9..17e1b60d178f 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -12,6 +12,7 @@ #include "build_assert.c" #include "build_bug.c" #include "err.c" +#include "jump_label.c" #include "kunit.c" #include "mutex.c" #include "page.c" diff --git a/rust/helpers/jump_label.c b/rust/helpers/jump_label.c new file mode 100644 index 000000000000..0e9ed15903f6 --- /dev/null +++ b/rust/helpers/jump_label.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2024 Google LLC. + */ + +#include <linux/jump_label.h> + +#ifndef CONFIG_JUMP_LABEL +int rust_helper_static_key_count(struct static_key *key) +{ + return static_key_count(key); +} +EXPORT_SYMBOL_GPL(rust_helper_static_key_count); +#endif diff --git a/rust/kernel/jump_label.rs b/rust/kernel/jump_label.rs new file mode 100644 index 000000000000..011e1fc1d19a --- /dev/null +++ b/rust/kernel/jump_label.rs @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2024 Google LLC. + +//! Logic for static keys. +//! +//! C header: [`include/linux/jump_label.h`](srctree/include/linux/jump_label.h). + +/// 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`. +/// +/// # Safety +/// +/// The macro must be used with a real static key defined by C. +#[macro_export] +macro_rules! static_key_false { + ($key:path, $keytyp:ty, $field:ident) => {{ + let _key: *const $keytyp = ::core::ptr::addr_of!($key); + let _key: *const $crate::bindings::static_key = ::core::ptr::addr_of!((*_key).$field); + + $crate::bindings::static_key_count(_key.cast_mut()) > 0 + }}; +} +pub use static_key_false; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 22a3bfa5a9e9..6dfafa69a84e 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -36,6 +36,7 @@ pub mod firmware; pub mod init; pub mod ioctl; +pub mod jump_label; #[cfg(CONFIG_KUNIT)] pub mod kunit; pub mod list;