[PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Alice Ryhl]

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(-)

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
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Miguel Ojeda]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Steven Rostedt]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Alice Ryhl]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Steven Rostedt]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Alice Ryhl]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Steven Rostedt]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Alice Ryhl]

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

Re: [PATCH RESEND] jump_label: rust: pass a mut ptr to `static_key_count`

[Steven Rostedt]

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