Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The commit message explicitly contradicts the documentation and selftests regarding the stack map reset behavior. - [Medium] The `stackmap-instance-gate.tc` selftest lacks an EXIT trap to clean up the secondary trace instance if the script is interrupted. --
commit a336359d2bbb858ace4ecacee011a90b6d577fea Author: Pengfei Li <[email protected]> trace: add documentation, selftest and tooling for stackmap This commit adds supporting documentation, selftests, and a python parser for the ftrace stackmap feature. It documents the tracefs interfaces, configuration options, and tests both standard and error conditions. Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/ > - Reset is destructive: it requires tracing to be stopped and also > clears the ring buffer so no stale <stack_id N> survives [Severity: Medium] The commit message states that reset requires tracing to be stopped and clears the ring buffer. However, the documentation and selftests in this patch enforce the exact opposite behavior. Could the commit message be out of date with the actual implementation? > diff --git a/Documentation/trace/ftrace-stackmap.rst > b/Documentation/trace/ftrace-stackmap.rst > --- /dev/null > +++ b/Documentation/trace/ftrace-stackmap.rst [ ... ] > +To reset the stack map:: > + > + echo 0 > /sys/kernel/debug/tracing/stack_map > + > +Reset returns ``-EBUSY`` only if another reset is already in progress. > + > +Reset clears the map and nothing else: the trace buffer is left > +untouched and tracing does not have to be stopped. As a result a trace [Severity: Medium] As noted above, this documentation explicitly describes a non-destructive behavior, completely contradicting the commit description. Is the documentation correct, or is the commit message correct? > diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-basic.tc > b/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-basic.tc > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-basic.tc [ ... ] > +# Reset does not require tracing to be stopped: it clears the map only > +# and leaves the ring buffer alone, so it must succeed with tracing on. > +enable_tracing > +echo 0 > stack_map || fail "stackmap reset failed while tracing is active" [Severity: Medium] This test actively enforces that reset succeeds while tracing is active, again contradicting the commit message claim that it should require tracing to be stopped. > diff --git > a/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-instance-gate.tc > b/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-instance-gate.tc > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-instance-gate.tc [ ... ] > +fail() { > + echo "FAIL: $1" > + rmdir instances/test_stackmap_gate 2>/dev/null > + exit_fail > +} [ ... ] > +# 2. Create a secondary instance and verify it does NOT see the option > +# or the stack_map* nodes. > +mkdir instances/test_stackmap_gate || fail "could not create secondary > instance" [Severity: Medium] If this selftest is interrupted (e.g., by a signal), won't the manual cleanup in fail() be bypassed, leaking the test_stackmap_gate trace instance? Other selftests in this patchset correctly use an EXIT trap to ensure cleanup happens even on interruption. Would it make sense to add a similar cleanup trap here to prevent poisoning the tracefs environment for subsequent tests? > diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-reset.tc > b/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-reset.tc > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/stackmap-reset.tc [ ... ] > +# Reset clears the map only. It must succeed and must not disturb the > +# trace buffer. > +echo 0 > stack_map || fail "reset failed" > + > +after=$(grep -c "<stack_id" trace || true) > +: "${after:=0}" > +if [ "$after" -ne "$before" ]; then > + fail "reset changed the trace buffer: $before -> $after <stack_id> > events" > +fi [Severity: Medium] Similarly, this test verifies the ring buffer is not cleared during a reset. Could we update the commit message to reflect the current design? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
