Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The `trigger-wprobe.tc` test has a race condition causing intermittent failures because the `clear_wprobe` trigger executes asynchronously and the test does not wait for it to fire before verifying the trace buffer. - [Medium] The `trigger-wprobe.tc` test uses `timer=t` as a fetcharg without enforcing a BTF requirement, which will cause false-positive test failures on kernels without BTF support. --
--- Patch [10]: [PATCH v13 10/12] selftests: ftrace: Add wprobe trigger testcase --- commit f8200da0c99be51814ee14df413b8fd81995a06f Author: Masami Hiramatsu (Google) <[email protected]> selftests: ftrace: Add wprobe trigger testcase This commit adds a testcase to verify wprobe triggers. It uses set_wprobe and clear_wprobe triggers on fprobe and static trace events to monitor memory accesses within the trace-events-sample kernel module, and tests wprobe trigger syntax error logging. Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao...@gmail.com/ > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc > b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc > new file mode 100644 > index 0000000000000..0565e2c421778 > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc [ ... ] > +:;: "Add a wprobe event used by trigger" ;: > +echo 'w:watch rw@0:8 address=$addr value=$value' > dynamic_events > + > +:;: "Add events for triggering wprobe" ;: > +echo 'f:fprobes/testevent sample_timer_cb timer=t' >> dynamic_events [Severity: Medium] Is it intentional to use a local variable t as a fetcharg here? This relies on BTF debug info. On kernels without CONFIG_PROBE_EVENTS_BTF_ARGS=y or architectures lacking BTF support, the kernel's tracing subsystem cannot resolve the argument and the fprobe registration will fail. Should this test either add a requirement for BTF arguments in the requires directive, or use a register argument like $arg1 instead? > + > +:;: "Enable all events before setting triggers" ;: > +echo 1 > tracing_on > +echo 1 >> events/fprobes/testevent/enable > +echo 1 >> events/sample-trace/foo_bar_with_fn/enable [ ... ] > +:;: "Set clear_wprobe trigger on foo_bar_with_fn" ;: > +echo 'clear_wprobe:watch' >> events/sample-trace/foo_bar_with_fn/trigger > +cat events/sample-trace/foo_bar_with_fn/trigger | grep ^clear_wprobe > + > +# Disable set_wprobe to prevent sample_timer_cb from re-arming the watchpoint > +echo '!set_wprobe:watch:timer' >> events/fprobes/testevent/trigger > + > +# Clear trace and wait to ensure no new watchpoint events are generated > +clear_trace > +sleep 1 [Severity: Medium] Can this sequence cause a race condition resulting in intermittent test failures? If the test clears the trace buffer here without first waiting for foo_bar_with_fn to execute and actually clear the watchpoint, there is a race window. Since both foo_bar_with_fn and sample_timer_cb run periodically every second, could sample_timer_cb execute after clear_trace but before foo_bar_with_fn has fired? If so, it would access the still-armed watchpoint and log a watch event in the newly cleared trace buffer, causing the final verification step (! grep -q watch trace) to fail. > + > +:;: "Ensure clear_wprobe trigger deactivated the watchpoint" ;: > +! grep -q watch trace > + -- Sashiko AI review ยท https://sashiko.dev/#/patchset/178739053919.1520941.17662338993878200834.stgit@devnote2?part=10
