| Message ID | 20241011-tracepoint-v10-1-7fbde4d6b525@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Tracepoints and static branch in Rust | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-for-next-PR | fail | merge-conflict |
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Fri, 11 Oct 2024 10:13:34 +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_branch_unlikely` with a `struct > static_key_false`, 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 | 30 ++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 5 files changed, 48 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..6948cae5738f > --- /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); ^ Explicit export should be removed. This only works because we didn't remove export.h from all helpers.c yet, but there's a patch to do that and this will stop working. You can add a Reviewed-by from me with this fixed. > +#endif > diff --git a/rust/kernel/jump_label.rs b/rust/kernel/jump_label.rs > new file mode 100644 > index 000000000000..4b7655b2a022 > --- /dev/null > +++ b/rust/kernel/jump_label.rs > @@ -0,0 +1,30 @@ > +// 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_branch_unlikely { > + ($key:path, $keytyp:ty, $field:ident) => {{ > + let _key: *const $keytyp = ::core::ptr::addr_of!($key); > + let _key: *const $crate::bindings::static_key_false = ::core::ptr::addr_of!((*_key).$field); > + let _key: *const $crate::bindings::static_key = _key.cast(); > + > + $crate::bindings::static_key_count(_key.cast_mut()) > 0 > + }}; > +} > +pub use static_branch_unlikely; > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index b5f4b3ce6b48..708ff817ccc3 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; >
On Fri, Oct 11, 2024 at 5:13 AM Gary Guo <gary@garyguo.net> wrote: > > On Fri, 11 Oct 2024 10:13:34 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: > > > +#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); > > ^ Explicit export should be removed. This only works because we didn't > remove export.h from all helpers.c yet, but there's a patch to do > that and this will stop working. What's the benefit of removing explicit exports from the Rust helper C code? It requires special casing things like modversions for these files, so I assume there's a reason for this. I asked about it here, but never got a response: https://lore.kernel.org/rust-for-linux/CABCJKudqAEvLcdqTqyfE2+iW+jeqBpnTGgYJvrZ0by6hGdfevQ@mail.gmail.com/ Sami
On Fri, 11 Oct 2024 08:23:18 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > On Fri, Oct 11, 2024 at 5:13 AM Gary Guo <gary@garyguo.net> wrote: > > > > On Fri, 11 Oct 2024 10:13:34 +0000 > > Alice Ryhl <aliceryhl@google.com> wrote: > > > > > +#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); > > > > ^ Explicit export should be removed. This only works because we didn't > > remove export.h from all helpers.c yet, but there's a patch to do > > that and this will stop working. > > What's the benefit of removing explicit exports from the Rust helper C > code? It requires special casing things like modversions for these > files, so I assume there's a reason for this. I asked about it here, > but never got a response: > > https://lore.kernel.org/rust-for-linux/CABCJKudqAEvLcdqTqyfE2+iW+jeqBpnTGgYJvrZ0by6hGdfevQ@mail.gmail.com/ > > Sami Ah, I didn't saw that email, probably because I archived the mails after the patch is applied. We're working towards having an option that enables inlining these helpers into Rust; when that option is enabled, the helpers must not be exported. See https://lore.kernel.org/rust-for-linux/20240529202817.3641974-1-gary@garyguo.net/ and https://lwn.net/Articles/993163/. It's also quite tedious for every helper to carry this export. Best, Gary
Hi Gary, On Fri, Oct 11, 2024 at 9:12 AM Gary Guo <gary@garyguo.net> wrote: > > On Fri, 11 Oct 2024 08:23:18 -0700 > Sami Tolvanen <samitolvanen@google.com> wrote: > > > On Fri, Oct 11, 2024 at 5:13 AM Gary Guo <gary@garyguo.net> wrote: > > > > > > On Fri, 11 Oct 2024 10:13:34 +0000 > > > Alice Ryhl <aliceryhl@google.com> wrote: > > > > > > > +#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); > > > > > > ^ Explicit export should be removed. This only works because we didn't > > > remove export.h from all helpers.c yet, but there's a patch to do > > > that and this will stop working. > > > > What's the benefit of removing explicit exports from the Rust helper C > > code? It requires special casing things like modversions for these > > files, so I assume there's a reason for this. I asked about it here, > > but never got a response: > > > > https://lore.kernel.org/rust-for-linux/CABCJKudqAEvLcdqTqyfE2+iW+jeqBpnTGgYJvrZ0by6hGdfevQ@mail.gmail.com/ > > > > Sami > > Ah, I didn't saw that email, probably because I archived the mails after > the patch is applied. Sometimes you might get pings about patches that are already applied too. :) > We're working towards having an option that enables inlining these > helpers into Rust; when that option is enabled, the helpers must not be > exported. See > https://lore.kernel.org/rust-for-linux/20240529202817.3641974-1-gary@garyguo.net/ > and https://lwn.net/Articles/993163/. Interesting, thanks for the links. It would have been helpful to explain the motivation for the change also in the patch that was applied. Did you consider using the preprocessor to simply skip exporting the helpers when cross-language LTO inlining is used? This would allow us to use the existing C build rules for the code instead of adding a separate rule to handle Rust-style exports, like I'm doing here: https://github.com/samitolvanen/linux/commit/545277e4d0432dafc530b1618f0152aed82af2f5 > It's also quite tedious for every helper to carry this export. It's just one line per helper, but sure, I do see your point. Sami
On Fri, Oct 11, 2024 at 7:52 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > It's just one line per helper, but sure, I do see your point. I guess we will have a lot of helpers added over time, so even if it is one line, it may end up being a lot of lines in total. The rules should stay constant, which would be better. Having said that, it is true the extra complexity of the rules isn't great either. Cheers, Miguel
On Fri, Oct 11, 2024 at 12:48 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Fri, Oct 11, 2024 at 7:52 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > It's just one line per helper, but sure, I do see your point. > > I guess we will have a lot of helpers added over time, so even if it > is one line, it may end up being a lot of lines in total. The rules > should stay constant, which would be better. Having said that, it is > true the extra complexity of the rules isn't great either. My only concern is that custom C build rules must be kept in sync with Makefile.build changes, while EXPORT_SYMBOL lines should be basically maintenance-free. However, perhaps this isn't really an issue since most of the complexity is already contained in rule_cc_o_c that we can conveniently reuse. Anyway, I think this is something we can worry about later if we actually run into problems. I was mostly interested in the reasoning behind these changes. Sami
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..6948cae5738f --- /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..4b7655b2a022 --- /dev/null +++ b/rust/kernel/jump_label.rs @@ -0,0 +1,30 @@ +// 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_branch_unlikely { + ($key:path, $keytyp:ty, $field:ident) => {{ + let _key: *const $keytyp = ::core::ptr::addr_of!($key); + let _key: *const $crate::bindings::static_key_false = ::core::ptr::addr_of!((*_key).$field); + let _key: *const $crate::bindings::static_key = _key.cast(); + + $crate::bindings::static_key_count(_key.cast_mut()) > 0 + }}; +} +pub use static_branch_unlikely; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index b5f4b3ce6b48..708ff817ccc3 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;