Re: [PATCH v17 11/13] selftests: tracing: Add wprobe trigger testcases

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] kselftest: trace buffer not cleared before verifying offline wprobe triggers
- [Low] kselftest: missing set_wprobe dependency check in requires lines
--

commit 0d531760cdf15ae449a5bc0545ea39a43134b990
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

selftests: tracing: Add wprobe trigger testcases

This commit adds several test scripts for wprobe triggers in the ftrace
selftests. The test scripts verify setting and clearing wprobe triggers,
CPU hotplug behavior for wprobes, and syntax error logging.

Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-hotplug.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-hotplug.tc
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-hotplug.tc
> @@ -0,0 +1,150 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: event trigger - test wprobe trigger across cpu hotplug
> +# requires: dynamic_events "w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]":README "f[:[<group>/][<event>]] <func-name>[%return] [<args>]":README events/sched/sched_process_fork/trigger "[(structname[,field])]<argname>[->field[->field|.field...]]":README

[Severity: Low]
Should we check for set_wprobe in the README in this requires line? If a kernel
has CONFIG_WPROBE_EVENTS=y but CONFIG_WPROBE_TRIGGERS=n, it seems this test
will execute and fail unexpectedly with "Failed to set set_wprobe trigger"
instead of being gracefully skipped as UNRESOLVED.

[ ... ]

> +:;: "Take target CPU offline" ;:
> +echo 0 > $cpu/online
> +sleep 1
> +
> +# Wait for sample_timer_cb to fire and set_wprobe trigger to activate while CPU is offline
> +sleep 2
> +
> +:;: "Check set_wprobe trigger activated the watchpoint" ;:
> +if ! grep -q watch trace; then
> +  fail "Failed to trigger watchpoint while CPU is offline"
> +fi

[Severity: Low]
Does this grep falsely match events generated while the CPU was still online?
Because the target fprobe (sample_timer_cb) fires continuously, the trace
buffer could log a watch event before the offlining process actually completes.
Should we add a clear_trace call either before or right after taking the CPU
offline?

[ ... ]

> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-syntax-errors.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-syntax-errors.tc
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-syntax-errors.tc
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: event trigger - test wprobe trigger syntax errors
> +# requires: dynamic_events error_log "w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]":README "f[:[<group>/][<event>]] <func-name>[%return] [<args>]":README events/sched/sched_process_fork/trigger

[Severity: Low]
Similar to the hotplug script, does this requires line need a set_wprobe README
check to prevent false test failures on kernels that lack wprobe trigger
support?

[ ... ]

> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
> @@ -0,0 +1,107 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: event trigger - test wprobe trigger
> +# requires: dynamic_events "w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]":README "f[:[<group>/][<event>]] <func-name>[%return] [<args>]":README events/sched/sched_process_fork/trigger "[(structname[,field])]<argname>[->field[->field|.field...]]":README

[Severity: Low]
Does this requires line also need the set_wprobe dependency check to ensure the
feature is fully supported before execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179005108298.388919.4535333252892590932.stgit@devnote2?part=11