| Message ID | 20241118202727.73646-1-aliceryhl@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] jump_label: rust: pass a mut ptr to `static_key_count` | expand |
On Mon, Nov 18, 2024 at 9:27 PM Alice Ryhl <aliceryhl@google.com> wrote: > > When building the rust_print sample with CONFIG_JUMP_LABEL=n, the Rust > static key support falls back to using static_key_count. This function > accepts a mutable pointer to the `struct static_key`, but the Rust > abstractions are incorrectly passing a const pointer. > > This means that builds using CONFIG_JUMP_LABEL=n and SAMPLE_RUST_PRINT=y > fail with the following error message: > > error[E0308]: mismatched types > --> <root>/samples/rust/rust_print_main.rs:87:5 > | > 87 | / kernel::declare_trace! { > 88 | | /// # Safety > 89 | | /// > 90 | | /// Always safe to call. > 91 | | unsafe fn rust_sample_loaded(magic: c_int); > 92 | | } > | | ^ > | | | > | |_____types differ in mutability > | arguments to this function are incorrect > | > = note: expected raw pointer `*mut kernel::bindings::static_key` > found raw pointer `*const kernel::bindings::static_key` > note: function defined here > --> <root>/rust/bindings/bindings_helpers_generated.rs:33:12 > | > 33 | pub fn static_key_count(key: *mut static_key) -> c_int; > | ^^^^^^^^^^^^^^^^ > > To fix this, insert a pointer cast so that the pointer is mutable. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202411181440.qEdcuyh6-lkp@intel.com/ > Fixes: 169484ab6677 ("rust: add arch_static_branch") > Signed-off-by: Alice Ryhl <aliceryhl@google.com> Yeah, the original had the `.cast_mut()`. Acked-by: Miguel Ojeda <ojeda@kernel.org> Cheers, Miguel
On Mon, 18 Nov 2024 20:27:26 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > When building the rust_print sample with CONFIG_JUMP_LABEL=n, the Rust > static key support falls back to using static_key_count. This function > accepts a mutable pointer to the `struct static_key`, but the Rust > abstractions are incorrectly passing a const pointer. > > This means that builds using CONFIG_JUMP_LABEL=n and SAMPLE_RUST_PRINT=y > fail with the following error message: > > error[E0308]: mismatched types > --> <root>/samples/rust/rust_print_main.rs:87:5 > | > 87 | / kernel::declare_trace! { > 88 | | /// # Safety > 89 | | /// > 90 | | /// Always safe to call. > 91 | | unsafe fn rust_sample_loaded(magic: c_int); > 92 | | } > | | ^ > | | | > | |_____types differ in mutability > | arguments to this function are incorrect > | > = note: expected raw pointer `*mut kernel::bindings::static_key` > found raw pointer `*const kernel::bindings::static_key` > note: function defined here > --> <root>/rust/bindings/bindings_helpers_generated.rs:33:12 > | > 33 | pub fn static_key_count(key: *mut static_key) -> c_int; > | ^^^^^^^^^^^^^^^^ > > To fix this, insert a pointer cast so that the pointer is mutable. Hi Alice, BTW, how do you test the sample tracepoint? I see you add a tracepoint in the module load call, but currently we can enable module tracepoints on module load. How do you enable it when the rust_print module gets loaded? -- Steve > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202411181440.qEdcuyh6-lkp@intel.com/ > Fixes: 169484ab6677 ("rust: add arch_static_branch") > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/kernel/jump_label.rs | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/rust/kernel/jump_label.rs b/rust/kernel/jump_label.rs > index 2f2df03a3275..b5aff632ecc7 100644 > --- a/rust/kernel/jump_label.rs > +++ b/rust/kernel/jump_label.rs > @@ -26,7 +26,7 @@ macro_rules! static_branch_unlikely { > > #[cfg(not(CONFIG_JUMP_LABEL))] > { > - $crate::bindings::static_key_count(_key) > 0 > + $crate::bindings::static_key_count(_key.cast_mut()) > 0 > } > > #[cfg(CONFIG_JUMP_LABEL)] > > base-commit: 22193c586b43ee88d66954395885742a6e4a49a9
On Tue, Nov 19, 2024 at 12:27 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 18 Nov 2024 20:27:26 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: > > > When building the rust_print sample with CONFIG_JUMP_LABEL=n, the Rust > > static key support falls back to using static_key_count. This function > > accepts a mutable pointer to the `struct static_key`, but the Rust > > abstractions are incorrectly passing a const pointer. > > > > This means that builds using CONFIG_JUMP_LABEL=n and SAMPLE_RUST_PRINT=y > > fail with the following error message: > > > > error[E0308]: mismatched types > > --> <root>/samples/rust/rust_print_main.rs:87:5 > > | > > 87 | / kernel::declare_trace! { > > 88 | | /// # Safety > > 89 | | /// > > 90 | | /// Always safe to call. > > 91 | | unsafe fn rust_sample_loaded(magic: c_int); > > 92 | | } > > | | ^ > > | | | > > | |_____types differ in mutability > > | arguments to this function are incorrect > > | > > = note: expected raw pointer `*mut kernel::bindings::static_key` > > found raw pointer `*const kernel::bindings::static_key` > > note: function defined here > > --> <root>/rust/bindings/bindings_helpers_generated.rs:33:12 > > | > > 33 | pub fn static_key_count(key: *mut static_key) -> c_int; > > | ^^^^^^^^^^^^^^^^ > > > > To fix this, insert a pointer cast so that the pointer is mutable. > > Hi Alice, > > BTW, how do you test the sample tracepoint? I see you add a tracepoint in > the module load call, but currently we can enable module tracepoints on > module load. How do you enable it when the rust_print module gets loaded? I've only really used the sample in this series as a build test. The bulk of my testing has occurred by using this tracepoint support in Rust Binder where I have 30 different tracepoints and I've been able to verify that I can enable them and so on there. Alice
On Tue, 19 Nov 2024 09:36:38 +0100 Alice Ryhl <aliceryhl@google.com> wrote: > I've only really used the sample in this series as a build test. The > bulk of my testing has occurred by using this tracepoint support in > Rust Binder where I have 30 different tracepoints and I've been able > to verify that I can enable them and so on there. Could you modify the sample code to have a tracepoint that I could enable. I don't use the Rust Binder code so I can't test it. I use the trace_event sample code for selftests, and it would also be a good idea to add the rust trace event sample to the self tests as well. Would you be able to create some simple code to do that? If you add something in the rust sample code, I'll add it to the ftrace selftests. Thanks, -- Steve
On Tue, Nov 19, 2024 at 4:12 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 19 Nov 2024 09:36:38 +0100 > Alice Ryhl <aliceryhl@google.com> wrote: > > > I've only really used the sample in this series as a build test. The > > bulk of my testing has occurred by using this tracepoint support in > > Rust Binder where I have 30 different tracepoints and I've been able > > to verify that I can enable them and so on there. > > Could you modify the sample code to have a tracepoint that I could enable. > I don't use the Rust Binder code so I can't test it. I use the trace_event > sample code for selftests, and it would also be a good idea to add the rust > trace event sample to the self tests as well. > > Would you be able to create some simple code to do that? If you add > something in the rust sample code, I'll add it to the ftrace selftests. Sure, I'll submit a sample which does that for the 6.14 cycle. Alice
On Tue, 19 Nov 2024 16:31:40 +0100 Alice Ryhl <aliceryhl@google.com> wrote: > > Would you be able to create some simple code to do that? If you add > > something in the rust sample code, I'll add it to the ftrace selftests. > > Sure, I'll submit a sample which does that for the 6.14 cycle. Is it because you don't have time this week or is it because you think it's just too late to add code during the merge window? If it's the former, then sure, we can wait. But if you want to wait just because we are in the merge window, you don't need to. For things that add testing, it's OK to send even this late in the game. What I and many should not accept, is any active development on the kernel itself. Adding a sample module to be able to test if things are working properly does not fall under that category. The reason I ask, is I feel nervous about sending code to Linus that I personally do not test. It's not a show stopper. I'll still send your patches without or without this change. Before I send a pull request to Linus, I like to run one last smoke test on the code, and that's when I discovered I don't really have any tests to test your code. -- Steve
On Tue, Nov 19, 2024 at 4:53 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 19 Nov 2024 16:31:40 +0100 > Alice Ryhl <aliceryhl@google.com> wrote: > > > > Would you be able to create some simple code to do that? If you add > > > something in the rust sample code, I'll add it to the ftrace selftests. > > > > Sure, I'll submit a sample which does that for the 6.14 cycle. > > Is it because you don't have time this week or is it because you think it's > just too late to add code during the merge window? If it's the former, then > sure, we can wait. But if you want to wait just because we are in the merge > window, you don't need to. For things that add testing, it's OK to send > even this late in the game. What I and many should not accept, is any > active development on the kernel itself. Adding a sample module to be able > to test if things are working properly does not fall under that category. > > The reason I ask, is I feel nervous about sending code to Linus that I > personally do not test. It's not a show stopper. I'll still send your > patches without or without this change. Before I send a pull request to > Linus, I like to run one last smoke test on the code, and that's when I > discovered I don't really have any tests to test your code. Mostly because of the merge window, though also because waiting would let me use things landing in 6.13 through other maintainer trees; this would let me have the sample declare a miscdevice. But what I can submit now is this: create a Rust file that exposes a function which just triggers a tracepoint, and then I can call that function from C somewhere in the ftrace selftests, and then afterwards the C code could assert that the tracepoint got triggered. Is there an existing ftrace test that I can look at to base my work on? Alice
On Tue, 19 Nov 2024 17:16:56 +0100 Alice Ryhl <aliceryhl@google.com> wrote: > Mostly because of the merge window, though also because waiting would > let me use things landing in 6.13 through other maintainer trees; this > would let me have the sample declare a miscdevice. But what I can > submit now is this: create a Rust file that exposes a function which > just triggers a tracepoint, and then I can call that function from C > somewhere in the ftrace selftests, and then afterwards the C code > could assert that the tracepoint got triggered. Is there an existing > ftrace test that I can look at to base my work on? You can look at samples/trace_events/trace-events-sample.c which actually creates a thread that runs periodically and triggers trace events. And the test: tools/testing/selftests/ftrace/test.d/dynevent/add_remove_tprobe_module.tc Also the module: samples/trace_printk/trace-printk.c and the test: tools/testing/selftests/ftrace/test.d/event/trace_printk.tc -- Steve
diff --git a/rust/kernel/jump_label.rs b/rust/kernel/jump_label.rs index 2f2df03a3275..b5aff632ecc7 100644 --- a/rust/kernel/jump_label.rs +++ b/rust/kernel/jump_label.rs @@ -26,7 +26,7 @@ macro_rules! static_branch_unlikely { #[cfg(not(CONFIG_JUMP_LABEL))] { - $crate::bindings::static_key_count(_key) > 0 + $crate::bindings::static_key_count(_key.cast_mut()) > 0 } #[cfg(CONFIG_JUMP_LABEL)]
When building the rust_print sample with CONFIG_JUMP_LABEL=n, the Rust static key support falls back to using static_key_count. This function accepts a mutable pointer to the `struct static_key`, but the Rust abstractions are incorrectly passing a const pointer. This means that builds using CONFIG_JUMP_LABEL=n and SAMPLE_RUST_PRINT=y fail with the following error message: error[E0308]: mismatched types --> <root>/samples/rust/rust_print_main.rs:87:5 | 87 | / kernel::declare_trace! { 88 | | /// # Safety 89 | | /// 90 | | /// Always safe to call. 91 | | unsafe fn rust_sample_loaded(magic: c_int); 92 | | } | | ^ | | | | |_____types differ in mutability | arguments to this function are incorrect | = note: expected raw pointer `*mut kernel::bindings::static_key` found raw pointer `*const kernel::bindings::static_key` note: function defined here --> <root>/rust/bindings/bindings_helpers_generated.rs:33:12 | 33 | pub fn static_key_count(key: *mut static_key) -> c_int; | ^^^^^^^^^^^^^^^^ To fix this, insert a pointer cast so that the pointer is mutable. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202411181440.qEdcuyh6-lkp@intel.com/ Fixes: 169484ab6677 ("rust: add arch_static_branch") Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- rust/kernel/jump_label.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 22193c586b43ee88d66954395885742a6e4a49a9