Hi,
Here is a series of some fixes/improvements for the test modules and boot
time selftest of kprobe events. I found a WARNING message with some boot
time selftest configuration, which came from the combination of embedded
kprobe generate API tests module and ftrace boot-time selftest. So the main
problem is that the test module should not be built-in. But I also think
this WARNING message is useless (because there are warning messages already)
and the cleanup code is redundant. This series fixes those issues.
Thank you,
---
Masami Hiramatsu (Google) (3):
tracing: Build event generation tests only as modules
tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests
tracing/kprobe: Remove cleanup code unrelated to selftest
kernel/trace/Kconfig | 4 ++--
kernel/trace/trace_kprobe.c | 29 ++++++++++++-----------------
2 files changed, 14 insertions(+), 19 deletions(-)
--
Masami Hiramatsu (Google) <[email protected]>
From: Masami Hiramatsu (Google) <[email protected]>
Since the kprobes and synth event generation tests adds and enable
generated events in init_module() and delete it in exit_module(),
if we make it as built-in, those events are left in kernel and cause
kprobe event self-test failure.
[ 97.349708] ------------[ cut here ]------------
[ 97.353453] WARNING: CPU: 3 PID: 1 at kernel/trace/trace_kprobe.c:2133 kprobe_trace_self_tests_init+0x3f1/0x480
[ 97.357106] Modules linked in:
[ 97.358488] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.9.0-g699646734ab5-dirty #14
[ 97.361556] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[ 97.363880] RIP: 0010:kprobe_trace_self_tests_init+0x3f1/0x480
[ 97.365538] Code: a8 24 08 82 e9 ae fd ff ff 90 0f 0b 90 48 c7 c7 e5 aa 0b 82 e9 ee fc ff ff 90 0f 0b 90 48 c7 c7 2d 61 06 82 e9 8e fd ff ff 90 <0f> 0b 90 48 c7 c7 33 0b 0c 82 89 c6 e8 6e 03 1f ff 41 ff c7 e9 90
[ 97.370429] RSP: 0000:ffffc90000013b50 EFLAGS: 00010286
[ 97.371852] RAX: 00000000fffffff0 RBX: ffff888005919c00 RCX: 0000000000000000
[ 97.373829] RDX: ffff888003f40000 RSI: ffffffff8236a598 RDI: ffff888003f40a68
[ 97.375715] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[ 97.377675] R10: ffffffff811c9ae5 R11: ffffffff8120c4e0 R12: 0000000000000000
[ 97.379591] R13: 0000000000000001 R14: 0000000000000015 R15: 0000000000000000
[ 97.381536] FS: 0000000000000000(0000) GS:ffff88807dcc0000(0000) knlGS:0000000000000000
[ 97.383813] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 97.385449] CR2: 0000000000000000 CR3: 0000000002244000 CR4: 00000000000006b0
[ 97.387347] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 97.389277] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 97.391196] Call Trace:
[ 97.391967] <TASK>
[ 97.392647] ? __warn+0xcc/0x180
[ 97.393640] ? kprobe_trace_self_tests_init+0x3f1/0x480
[ 97.395181] ? report_bug+0xbd/0x150
[ 97.396234] ? handle_bug+0x3e/0x60
[ 97.397311] ? exc_invalid_op+0x1a/0x50
[ 97.398434] ? asm_exc_invalid_op+0x1a/0x20
[ 97.399652] ? trace_kprobe_is_busy+0x20/0x20
[ 97.400904] ? tracing_reset_all_online_cpus+0x15/0x90
[ 97.402304] ? kprobe_trace_self_tests_init+0x3f1/0x480
[ 97.403773] ? init_kprobe_trace+0x50/0x50
[ 97.404972] do_one_initcall+0x112/0x240
[ 97.406113] do_initcall_level+0x95/0xb0
[ 97.407286] ? kernel_init+0x1a/0x1a0
[ 97.408401] do_initcalls+0x3f/0x70
[ 97.409452] kernel_init_freeable+0x16f/0x1e0
[ 97.410662] ? rest_init+0x1f0/0x1f0
[ 97.411738] kernel_init+0x1a/0x1a0
[ 97.412788] ret_from_fork+0x39/0x50
[ 97.413817] ? rest_init+0x1f0/0x1f0
[ 97.414844] ret_from_fork_asm+0x11/0x20
[ 97.416285] </TASK>
[ 97.417134] irq event stamp: 13437323
[ 97.418376] hardirqs last enabled at (13437337): [<ffffffff8110bc0c>] console_unlock+0x11c/0x150
[ 97.421285] hardirqs last disabled at (13437370): [<ffffffff8110bbf1>] console_unlock+0x101/0x150
[ 97.423838] softirqs last enabled at (13437366): [<ffffffff8108e17f>] handle_softirqs+0x23f/0x2a0
[ 97.426450] softirqs last disabled at (13437393): [<ffffffff8108e346>] __irq_exit_rcu+0x66/0xd0
[ 97.428850] ---[ end trace 0000000000000000 ]---
To avoid this issue, build these tests only as modules.
Fixes: 9fe41efaca08 ("tracing: Add synth event generation test module")
Fixes: 64836248dda2 ("tracing: Add kprobe event command generation test module")
Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
---
kernel/trace/Kconfig | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 166ad5444eea..721c3b221048 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -1136,7 +1136,7 @@ config PREEMPTIRQ_DELAY_TEST
config SYNTH_EVENT_GEN_TEST
tristate "Test module for in-kernel synthetic event generation"
- depends on SYNTH_EVENTS
+ depends on SYNTH_EVENTS && m
help
This option creates a test module to check the base
functionality of in-kernel synthetic event definition and
@@ -1149,7 +1149,7 @@ config SYNTH_EVENT_GEN_TEST
config KPROBE_EVENT_GEN_TEST
tristate "Test module for in-kernel kprobe event generation"
- depends on KPROBE_EVENTS
+ depends on KPROBE_EVENTS && m
help
This option creates a test module to check the base
functionality of in-kernel kprobe event definition.
From: Masami Hiramatsu (Google) <[email protected]>
Since the kprobe-events selftest shows OK or NG with the reason, the
WARN_ON_ONCE()s for each place are redundant. Let's remove it.
Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
---
kernel/trace/trace_kprobe.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 16383247bdbf..4abed36544d0 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -2023,18 +2023,18 @@ static __init int kprobe_trace_self_tests_init(void)
pr_info("Testing kprobe tracing: ");
ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
- if (WARN_ON_ONCE(ret)) {
+ if (ret) {
pr_warn("error on probing function entry.\n");
warn++;
} else {
/* Enable trace point */
tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
- if (WARN_ON_ONCE(tk == NULL)) {
+ if (tk == NULL) {
pr_warn("error on getting new probe.\n");
warn++;
} else {
file = find_trace_probe_file(tk, top_trace_array());
- if (WARN_ON_ONCE(file == NULL)) {
+ if (file == NULL) {
pr_warn("error on getting probe file.\n");
warn++;
} else
@@ -2044,18 +2044,18 @@ static __init int kprobe_trace_self_tests_init(void)
}
ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
- if (WARN_ON_ONCE(ret)) {
+ if (ret) {
pr_warn("error on probing function return.\n");
warn++;
} else {
/* Enable trace point */
tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
- if (WARN_ON_ONCE(tk == NULL)) {
+ if (tk == NULL) {
pr_warn("error on getting 2nd new probe.\n");
warn++;
} else {
file = find_trace_probe_file(tk, top_trace_array());
- if (WARN_ON_ONCE(file == NULL)) {
+ if (file == NULL) {
pr_warn("error on getting probe file.\n");
warn++;
} else
@@ -2079,7 +2079,7 @@ static __init int kprobe_trace_self_tests_init(void)
/* Disable trace points before removing it */
tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
- if (WARN_ON_ONCE(tk == NULL)) {
+ if (tk == NULL) {
pr_warn("error on getting test probe.\n");
warn++;
} else {
@@ -2089,7 +2089,7 @@ static __init int kprobe_trace_self_tests_init(void)
}
file = find_trace_probe_file(tk, top_trace_array());
- if (WARN_ON_ONCE(file == NULL)) {
+ if (file == NULL) {
pr_warn("error on getting probe file.\n");
warn++;
} else
@@ -2098,7 +2098,7 @@ static __init int kprobe_trace_self_tests_init(void)
}
tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
- if (WARN_ON_ONCE(tk == NULL)) {
+ if (tk == NULL) {
pr_warn("error on getting 2nd test probe.\n");
warn++;
} else {
@@ -2108,7 +2108,7 @@ static __init int kprobe_trace_self_tests_init(void)
}
file = find_trace_probe_file(tk, top_trace_array());
- if (WARN_ON_ONCE(file == NULL)) {
+ if (file == NULL) {
pr_warn("error on getting probe file.\n");
warn++;
} else
@@ -2117,20 +2117,20 @@ static __init int kprobe_trace_self_tests_init(void)
}
ret = create_or_delete_trace_kprobe("-:testprobe");
- if (WARN_ON_ONCE(ret)) {
+ if (ret) {
pr_warn("error on deleting a probe.\n");
warn++;
}
ret = create_or_delete_trace_kprobe("-:testprobe2");
- if (WARN_ON_ONCE(ret)) {
+ if (ret) {
pr_warn("error on deleting a probe.\n");
warn++;
}
end:
ret = dyn_events_release_all(&trace_kprobe_ops);
- if (WARN_ON_ONCE(ret)) {
+ if (ret) {
pr_warn("error on cleaning up probes.\n");
warn++;
}
From: Masami Hiramatsu (Google) <[email protected]>
This cleanup all kprobe events code is not related to the selftest
itself, and it can fail by the reason unrelated to this test.
If the test is successful, the generated events are cleaned up.
And if not, we cannot guarantee that the kprobe events will work
correctly. So, anyway, there is no need to clean it up.
Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
---
kernel/trace/trace_kprobe.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 4abed36544d0..f94628c15c14 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -2129,11 +2129,6 @@ static __init int kprobe_trace_self_tests_init(void)
}
end:
- ret = dyn_events_release_all(&trace_kprobe_ops);
- if (ret) {
- pr_warn("error on cleaning up probes.\n");
- warn++;
- }
/*
* Wait for the optimizer work to finish. Otherwise it might fiddle
* with probes in already freed __init text.
On Sun, 26 May 2024 19:10:57 +0900
"Masami Hiramatsu (Google)" <[email protected]> wrote:
> Hi,
>
> Here is a series of some fixes/improvements for the test modules and boot
> time selftest of kprobe events. I found a WARNING message with some boot
> time selftest configuration, which came from the combination of embedded
> kprobe generate API tests module and ftrace boot-time selftest. So the main
> problem is that the test module should not be built-in. But I also think
> this WARNING message is useless (because there are warning messages already)
> and the cleanup code is redundant. This series fixes those issues.
Note, when I enable trace tests as builtin instead of modules, I just
disable the bootup self tests when it detects this. This helps with
doing tests via config options than having to add user space code that
loads modules.
Could you do something similar?
-- Steve
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (Google) (3):
> tracing: Build event generation tests only as modules
> tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests
> tracing/kprobe: Remove cleanup code unrelated to selftest
>
On Mon, 27 May 2024 19:29:07 -0400
Steven Rostedt <[email protected]> wrote:
> On Sun, 26 May 2024 19:10:57 +0900
> "Masami Hiramatsu (Google)" <[email protected]> wrote:
>
> > Hi,
> >
> > Here is a series of some fixes/improvements for the test modules and boot
> > time selftest of kprobe events. I found a WARNING message with some boot
> > time selftest configuration, which came from the combination of embedded
> > kprobe generate API tests module and ftrace boot-time selftest. So the main
> > problem is that the test module should not be built-in. But I also think
> > this WARNING message is useless (because there are warning messages already)
> > and the cleanup code is redundant. This series fixes those issues.
>
> Note, when I enable trace tests as builtin instead of modules, I just
> disable the bootup self tests when it detects this. This helps with
> doing tests via config options than having to add user space code that
> loads modules.
>
> Could you do something similar?
OK, in that case, I would like to move the test cleanup code in
module_exit function into the end of module_init function.
It looks there is no reason to split those into 2 parts.
Thank you,
>
> -- Steve
>
>
> >
> > Thank you,
> >
> > ---
> >
> > Masami Hiramatsu (Google) (3):
> > tracing: Build event generation tests only as modules
> > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests
> > tracing/kprobe: Remove cleanup code unrelated to selftest
> >
--
Masami Hiramatsu (Google) <[email protected]>
On Wed, 29 May 2024 01:46:40 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> On Mon, 27 May 2024 19:29:07 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Sun, 26 May 2024 19:10:57 +0900
> > "Masami Hiramatsu (Google)" <[email protected]> wrote:
> >
> > > Hi,
> > >
> > > Here is a series of some fixes/improvements for the test modules and boot
> > > time selftest of kprobe events. I found a WARNING message with some boot
> > > time selftest configuration, which came from the combination of embedded
> > > kprobe generate API tests module and ftrace boot-time selftest. So the main
> > > problem is that the test module should not be built-in. But I also think
> > > this WARNING message is useless (because there are warning messages already)
> > > and the cleanup code is redundant. This series fixes those issues.
> >
> > Note, when I enable trace tests as builtin instead of modules, I just
> > disable the bootup self tests when it detects this. This helps with
> > doing tests via config options than having to add user space code that
> > loads modules.
> >
> > Could you do something similar?
>
> OK, in that case, I would like to move the test cleanup code in
> module_exit function into the end of module_init function.
> It looks there is no reason to split those into 2 parts.
Wait, I would like to hear Tom's opinion. I found following usage comments
in the code.
* Following that are a few examples using the created events to test
* various ways of tracing a synthetic event.
*
* To test, select CONFIG_SYNTH_EVENT_GEN_TEST and build the module.
* Then:
*
* # insmod kernel/trace/synth_event_gen_test.ko
* # cat /sys/kernel/tracing/trace
*
* You should see several events in the trace buffer -
* "create_synth_test", "empty_synth_test", and several instances of
* "gen_synth_test".
*
* To remove the events, remove the module:
*
* # rmmod synth_event_gen_test
Tom, is that intended behavior ? and are you expected to reuse these
events outside of the module? e.g. load the test module and run some
test script in user space which uses those events?
As far as I can see, those tests are not intended to be embedded in the
kernel because those are expected to be removed.
Thank you,
>
> Thank you,
>
> >
> > -- Steve
> >
> >
> > >
> > > Thank you,
> > >
> > > ---
> > >
> > > Masami Hiramatsu (Google) (3):
> > > tracing: Build event generation tests only as modules
> > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests
> > > tracing/kprobe: Remove cleanup code unrelated to selftest
> > >
>
>
> --
> Masami Hiramatsu (Google) <[email protected]>
--
Masami Hiramatsu (Google) <[email protected]>
Hi Masami,
On Wed, 2024-05-29 at 08:38 +0900, Masami Hiramatsu wrote:
> On Wed, 29 May 2024 01:46:40 +0900
> Masami Hiramatsu (Google) <[email protected]> wrote:
>
> > On Mon, 27 May 2024 19:29:07 -0400
> > Steven Rostedt <[email protected]> wrote:
> >
> > > On Sun, 26 May 2024 19:10:57 +0900
> > > "Masami Hiramatsu (Google)" <[email protected]> wrote:
> > >
> > > > Hi,
> > > >
> > > > Here is a series of some fixes/improvements for the test
> > > > modules and boot
> > > > time selftest of kprobe events. I found a WARNING message with
> > > > some boot
> > > > time selftest configuration, which came from the combination of
> > > > embedded
> > > > kprobe generate API tests module and ftrace boot-time selftest.
> > > > So the main
> > > > problem is that the test module should not be built-in. But I
> > > > also think
> > > > this WARNING message is useless (because there are warning
> > > > messages already)
> > > > and the cleanup code is redundant. This series fixes those
> > > > issues.
> > >
> > > Note, when I enable trace tests as builtin instead of modules, I
> > > just
> > > disable the bootup self tests when it detects this. This helps
> > > with
> > > doing tests via config options than having to add user space code
> > > that
> > > loads modules.
> > >
> > > Could you do something similar?
> >
> > OK, in that case, I would like to move the test cleanup code in
> > module_exit function into the end of module_init function.
> > It looks there is no reason to split those into 2 parts.
>
> Wait, I would like to hear Tom's opinion. I found following usage
> comments
> in the code.
>
> * Following that are a few examples using the created events to test
> * various ways of tracing a synthetic event.
> *
> * To test, select CONFIG_SYNTH_EVENT_GEN_TEST and build the module.
> * Then:
> *
> * # insmod kernel/trace/synth_event_gen_test.ko
> * # cat /sys/kernel/tracing/trace
> *
> * You should see several events in the trace buffer -
> * "create_synth_test", "empty_synth_test", and several instances of
> * "gen_synth_test".
> *
> * To remove the events, remove the module:
> *
> * # rmmod synth_event_gen_test
>
> Tom, is that intended behavior ? and are you expected to reuse these
> events outside of the module? e.g. load the test module and run some
> test script in user space which uses those events?
>
Yeah, this module was meant as a sample module showing how to create
and generate synthetic events in-kernel.
So the interested user insmods the module, looks at the trace stream
and sees, ok the events are there as expected, so it does work, great,
let's remove the module to get rid of them and go write our own.
Having both the creation and cleanup in module_init() wouldn't allow
the user the opportunity to do that i.e. verify the results by reading
the trace file.
Tom
> As far as I can see, those tests are not intended to be embedded in
> the
> kernel because those are expected to be removed.
>
> Thank you,
>
> >
> > Thank you,
> >
> > >
> > > -- Steve
> > >
> > >
> > > >
> > > > Thank you,
> > > >
> > > > ---
> > > >
> > > > Masami Hiramatsu (Google) (3):
> > > > tracing: Build event generation tests only as modules
> > > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in
> > > > selftests
> > > > tracing/kprobe: Remove cleanup code unrelated to selftest
> > > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <[email protected]>
>
>
On Wed, 29 May 2024 11:01:43 -0500
Tom Zanussi <[email protected]> wrote:
> Hi Masami,
>
> On Wed, 2024-05-29 at 08:38 +0900, Masami Hiramatsu wrote:
> > On Wed, 29 May 2024 01:46:40 +0900
> > Masami Hiramatsu (Google) <[email protected]> wrote:
> >
> > > On Mon, 27 May 2024 19:29:07 -0400
> > > Steven Rostedt <[email protected]> wrote:
> > >
> > > > On Sun, 26 May 2024 19:10:57 +0900
> > > > "Masami Hiramatsu (Google)" <[email protected]> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > Here is a series of some fixes/improvements for the test
> > > > > modules and boot
> > > > > time selftest of kprobe events. I found a WARNING message with
> > > > > some boot
> > > > > time selftest configuration, which came from the combination of
> > > > > embedded
> > > > > kprobe generate API tests module and ftrace boot-time selftest.
> > > > > So the main
> > > > > problem is that the test module should not be built-in. But I
> > > > > also think
> > > > > this WARNING message is useless (because there are warning
> > > > > messages already)
> > > > > and the cleanup code is redundant. This series fixes those
> > > > > issues.
> > > >
> > > > Note, when I enable trace tests as builtin instead of modules, I
> > > > just
> > > > disable the bootup self tests when it detects this. This helps
> > > > with
> > > > doing tests via config options than having to add user space code
> > > > that
> > > > loads modules.
> > > >
> > > > Could you do something similar?
> > >
> > > OK, in that case, I would like to move the test cleanup code in
> > > module_exit function into the end of module_init function.
> > > It looks there is no reason to split those into 2 parts.
> >
> > Wait, I would like to hear Tom's opinion. I found following usage
> > comments
> > in the code.
> >
> > * Following that are a few examples using the created events to test
> > * various ways of tracing a synthetic event.
> > *
> > * To test, select CONFIG_SYNTH_EVENT_GEN_TEST and build the module.
> > * Then:
> > *
> > * # insmod kernel/trace/synth_event_gen_test.ko
> > * # cat /sys/kernel/tracing/trace
> > *
> > * You should see several events in the trace buffer -
> > * "create_synth_test", "empty_synth_test", and several instances of
> > * "gen_synth_test".
> > *
> > * To remove the events, remove the module:
> > *
> > * # rmmod synth_event_gen_test
> >
> > Tom, is that intended behavior ? and are you expected to reuse these
> > events outside of the module? e.g. load the test module and run some
> > test script in user space which uses those events?
> >
>
> Yeah, this module was meant as a sample module showing how to create
> and generate synthetic events in-kernel.
>
> So the interested user insmods the module, looks at the trace stream
> and sees, ok the events are there as expected, so it does work, great,
> let's remove the module to get rid of them and go write our own.
>
> Having both the creation and cleanup in module_init() wouldn't allow
> the user the opportunity to do that i.e. verify the results by reading
> the trace file.
So, in summary, it is designed to be a module. Steve, I think these tests
should be kept as modules. There are many reason to do so.
- This test is designed to be used as module.
- This can conflict with other boot time selftest if it is embedded.
- We can make these tests and boot time selftest mutable exclusive but
if we make these tests as modules, we can build and run both tests
safely.
- Embedding these tests leave new events when the kernel boot, which
user must be cleaned up by manual.
What would you think?
Thank you,
>
> Tom
>
> > As far as I can see, those tests are not intended to be embedded in
> > the
> > kernel because those are expected to be removed.
> >
> > Thank you,
> >
> > >
> > > Thank you,
> > >
> > > >
> > > > -- Steve
> > > >
> > > >
> > > > >
> > > > > Thank you,
> > > > >
> > > > > ---
> > > > >
> > > > > Masami Hiramatsu (Google) (3):
> > > > > tracing: Build event generation tests only as modules
> > > > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in
> > > > > selftests
> > > > > tracing/kprobe: Remove cleanup code unrelated to selftest
> > > > >
> > >
> > >
> > > --
> > > Masami Hiramatsu (Google) <[email protected]>
> >
> >
>
>
--
Masami Hiramatsu (Google) <[email protected]>
On Fri, 31 May 2024 11:37:21 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> So, in summary, it is designed to be a module. Steve, I think these tests
> should be kept as modules. There are many reason to do so.
>
> - This test is designed to be used as module.
> - This can conflict with other boot time selftest if it is embedded.
> - We can make these tests and boot time selftest mutable exclusive but
> if we make these tests as modules, we can build and run both tests
> safely.
> - Embedding these tests leave new events when the kernel boot, which
> user must be cleaned up by manual.
>
> What would you think?
I was mostly following what Ingo told me long ago, where having it
built in is just one more way to test it ;-)
But that said, from your first patch, you show the stack dump and
mention:
> Since the kprobes and synth event generation tests adds and enable
> generated events in init_module() and delete it in exit_module(),
> if we make it as built-in, those events are left in kernel and cause
> kprobe event self-test failure.
But you don't explain what exactly the conflict is. What about those
events causes kprobe selftests to fail?
-- Steve
On Fri, 31 May 2024 03:24:25 -0400
Steven Rostedt <[email protected]> wrote:
> On Fri, 31 May 2024 11:37:21 +0900
> Masami Hiramatsu (Google) <[email protected]> wrote:
>
> > So, in summary, it is designed to be a module. Steve, I think these tests
> > should be kept as modules. There are many reason to do so.
> >
> > - This test is designed to be used as module.
> > - This can conflict with other boot time selftest if it is embedded.
> > - We can make these tests and boot time selftest mutable exclusive but
> > if we make these tests as modules, we can build and run both tests
> > safely.
> > - Embedding these tests leave new events when the kernel boot, which
> > user must be cleaned up by manual.
> >
> > What would you think?
>
> I was mostly following what Ingo told me long ago, where having it
> built in is just one more way to test it ;-)
Ah, would you mean embedding the module is also a part of tests?
>
> But that said, from your first patch, you show the stack dump and
> mention:
>
> > Since the kprobes and synth event generation tests adds and enable
> > generated events in init_module() and delete it in exit_module(),
> > if we make it as built-in, those events are left in kernel and cause
> > kprobe event self-test failure.
>
> But you don't explain what exactly the conflict is. What about those
> events causes kprobe selftests to fail?
The major conflict happens when the boot-time test cleans up the kprobe
events by
dyn_events_release_all(&trace_kprobe_ops);
And I removed it by [3/3] patch in this series :) because it does not
needed and not confirmed there is no other kprobe events when the test
starts. Also the warning message are redundant so I removed it by [2/3].
So without this [1/3], if we apply [2/3] and [3/3], the problem will be
mitigated, but I think the root cause is that these modules are built-in.
Thank you,
>
>
> -- Steve
--
Masami Hiramatsu (Google) <[email protected]>
On Fri, 31 May 2024 23:20:47 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> The major conflict happens when the boot-time test cleans up the kprobe
> events by
>
> dyn_events_release_all(&trace_kprobe_ops);
>
> And I removed it by [3/3] patch in this series :) because it does not
> needed and not confirmed there is no other kprobe events when the test
> starts. Also the warning message are redundant so I removed it by [2/3].
>
> So without this [1/3], if we apply [2/3] and [3/3], the problem will be
> mitigated, but I think the root cause is that these modules are built-in.
I'm OK with making them module only, but I don't see any selftests for
sythetic events. I think they should have a boot up test as well. If we
remove them, let's add something to test them at boot up. Then the boot up
code could clean it up.
Or change the test module to be a boot up test that cleans itself up if it
is compiled in as not a module?
-- Steve
On Tue, 4 Jun 2024 23:18:29 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> BTW, some these bootup tests can be ported on KUnit. Do you have a plan to
> use KUnit?
I haven't thought about that. If someone else wants to do it, I will
happily review it ;-)
-- Steve
On Tue, 4 Jun 2024 09:57:46 -0400
Steven Rostedt <[email protected]> wrote:
> On Fri, 31 May 2024 23:20:47 +0900
> Masami Hiramatsu (Google) <[email protected]> wrote:
>
> > The major conflict happens when the boot-time test cleans up the kprobe
> > events by
> >
> > dyn_events_release_all(&trace_kprobe_ops);
> >
> > And I removed it by [3/3] patch in this series :) because it does not
> > needed and not confirmed there is no other kprobe events when the test
> > starts. Also the warning message are redundant so I removed it by [2/3].
> >
> > So without this [1/3], if we apply [2/3] and [3/3], the problem will be
> > mitigated, but I think the root cause is that these modules are built-in.
>
> I'm OK with making them module only, but I don't see any selftests for
> sythetic events. I think they should have a boot up test as well. If we
> remove them, let's add something to test them at boot up. Then the boot up
> code could clean it up.
>
> Or change the test module to be a boot up test that cleans itself up if it
> is compiled in as not a module?
Yeah, I think we may need another test code for synthetic events, which
also triggering the synthetic events.
BTW, some these bootup tests can be ported on KUnit. Do you have a plan to
use KUnit?
Thank you,
>
> -- Steve
>
--
Masami Hiramatsu (Google) <[email protected]>
On Fri, 31 May 2024 03:24:25 -0400
Steven Rostedt <[email protected]> wrote:
> On Fri, 31 May 2024 11:37:21 +0900
> Masami Hiramatsu (Google) <[email protected]> wrote:
>
> > So, in summary, it is designed to be a module. Steve, I think these tests
> > should be kept as modules. There are many reason to do so.
> >
> > - This test is designed to be used as module.
> > - This can conflict with other boot time selftest if it is embedded.
> > - We can make these tests and boot time selftest mutable exclusive but
> > if we make these tests as modules, we can build and run both tests
> > safely.
> > - Embedding these tests leave new events when the kernel boot, which
> > user must be cleaned up by manual.
> >
> > What would you think?
>
> I was mostly following what Ingo told me long ago, where having it
> built in is just one more way to test it ;-)
>
> But that said, from your first patch, you show the stack dump and
> mention:
>
> > Since the kprobes and synth event generation tests adds and enable
> > generated events in init_module() and delete it in exit_module(),
> > if we make it as built-in, those events are left in kernel and cause
> > kprobe event self-test failure.
>
> But you don't explain what exactly the conflict is. What about those
> events causes kprobe selftests to fail?
I also found another problem on these modules. These modules get trace
event file references to prevent removing probes. Therefore, if we
embed these modules, we can not remove these events forever!
/*
* Now get the gen_kprobe_test event file. We need to prevent
* the instance and event from disappearing from underneath
* us, which trace_get_event_file() does (though in this case
* we're using the top-level instance which never goes away).
*/
gen_kprobe_test = trace_get_event_file(NULL, "kprobes",
"gen_kprobe_test");
if (IS_ERR(gen_kprobe_test)) {
ret = PTR_ERR(gen_kprobe_test);
goto delete;
}
This means most ftracetest fails because we can not clean up the
tracing state by removing dynamic events, which are installed while
booting.
Note that these references (locks) will be removed when the module
is unloaded.
Thanks,
>
>
> -- Steve
--
Masami Hiramatsu (Google) <[email protected]>
On Mon, 10 Jun 2024 11:10:01 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> > But you don't explain what exactly the conflict is. What about those
> > events causes kprobe selftests to fail?
>
> I also found another problem on these modules. These modules get trace
> event file references to prevent removing probes. Therefore, if we
> embed these modules, we can not remove these events forever!
>
> /*
> * Now get the gen_kprobe_test event file. We need to prevent
> * the instance and event from disappearing from underneath
> * us, which trace_get_event_file() does (though in this case
> * we're using the top-level instance which never goes away).
> */
> gen_kprobe_test = trace_get_event_file(NULL, "kprobes",
> "gen_kprobe_test");
> if (IS_ERR(gen_kprobe_test)) {
> ret = PTR_ERR(gen_kprobe_test);
> goto delete;
> }
>
> This means most ftracetest fails because we can not clean up the
> tracing state by removing dynamic events, which are installed while
> booting.
> Note that these references (locks) will be removed when the module
> is unloaded.
I'm fine if you want to force these as modules. Just make sure the change
log lists all the issues of them not being modules.
-- Steve