Message-ID: <Z0YBiu9XdstHmjWr@x1>
Date: Tue, 26 Nov 2024 14:12:42 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Howard Chu <howardchu95@...il.com>
Cc: peterz@...radead.org, mingo@...hat.com, namhyung@...nel.org,
	mark.rutland@....com, alexander.shishkin@...ux.intel.com,
	jolsa@...nel.org, irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] perf trace: Add tests for BTF general augmentation

On Tue, Nov 26, 2024 at 01:49:44PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Nov 22, 2024 at 04:55:10PM -0800, Howard Chu wrote:
> > Currently, we only have perf trace augmentation tests for enum
> > arguments. This patch adds tests for more general syscall arguments,
> > such as struct pointers, strings, and buffers.
> 
> scripts/checkpatch.pl has some warnings here I think we can address
> easily, some not so much, like the SPDX that we need to add logic to
> 'perf test' to noticed its the SPDX and skip it, looking at the next
> line for the test description.
> 
> 
> The long lines we can just make them multiple lines with the first ones
> ending in \

And while testing it with -vv:

root@...ber:~# perf test -vv btf
110: perf trace BTF general tests:
--- start ---
test child forked, pid 242944
Testing perf trace's string augmentation
grep: warning: stray \ before /
Testing perf trace's buffer augmentation
grep: warning: stray \ before /
Testing perf trace's struct augmentation
grep: warning: stray \ before /
---- end(0) ----
110: perf trace BTF general tests                                    : Ok
root@...ber:~#

The long lines can be solved like:

+++ b/tools/perf/tests/shell/trace_btf_general.sh
@@ -17,7 +17,8 @@ trap cleanup EXIT TERM INT HUP
 
 trace_test_string() {
   echo "Testing perf trace's string augmentation"
-  if ! perf trace -e renameat* --max-events=1 -- mv ${file1} ${file2} 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +mv\/[0-9]+ renameat(2)?\(olddfd: .*, oldname: \"${file1}\", newdfd: .*, newname: \"${file2}\", flags: .*\) += +[0-9]+$"
+  if ! perf trace -e renameat* --max-events=1 -- mv ${file1} ${file2} 2>&1 | \
+     grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +mv\/[0-9]+ renameat(2)?\(olddfd: .*, oldname: \"${file1}\", newdfd: .*, newname: \"${file2}\", flags: .*\) += +[0-9]+$"
   then
     echo "String augmentation test failed"
     err=1

And that " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +mv\/[0-9]+ : +"

part is common to several such greps, so you could have it as:

prefix=" +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +mv\/[0-9]+ : +"

And then use

grep -q -E "%{prefix}mv\/[0-9]+ renameat(2)?\(olddfd: .*, oldname: \"${file1}\", newdfd: .*, newname: \"${file2}\", flags: .*\) += +[0-9]+$"

Or, since this part isn't interesting for what this test checks, BPF
augmentation, we could make it all more compact, i.e.:

>From the default that is:

root@...ber:~# rm -f before after ; touch before ; perf trace -e renameat2 mv before after
     0.000 ( 0.037 ms): mv/243278 renameat2(olddfd: CWD, oldname: "before", newdfd: CWD, newname: "after", flags: NOREPLACE) = 0
root@...ber:~# 

We point perf to a temporary config file, using mktemp, then tell it to
not output the stuff we don't need while test BPF augmentation, making
the output more compact and thus the regexps in the perf test shorter.

With this we also test setting the "PERF_CONFIG" environment variable
and, 'perf config' and 'perf trace' output customization code:

root@...ber:~# export PERF_CONFIG=my-tmp-perf-config
root@...ber:~# perf config trace.show_arg_names=false
root@...ber:~# cat my-tmp-perf-config 
# this file is auto-generated.
[trace]
	show_arg_names = false
root@...ber:~# rm -f before after ; touch before ; perf trace -e renameat2 mv before after
     0.000 ( 0.032 ms): :243836/243836 renameat2(CWD, "before", CWD, "after", NOREPLACE)                     = 0
root@...ber:~# perf config trace.show_duration=false
root@...ber:~# rm -f before after ; touch before ; perf trace -e renameat2 mv before after
     0.000 mv/243871 renameat2(CWD, "before", CWD, "after", NOREPLACE)                     = 0
root@...ber:~# perf config trace.show_timestamp=false
root@...ber:~# rm -f before after ; touch before ; perf trace -e renameat2 mv before after
:243888/243888 renameat2(CWD, "before", CWD, "after", NOREPLACE)                     = 0
root@...ber:~# perf config trace.args_alignment=0
root@...ber:~# rm -f before after ; touch before ; perf trace -e renameat2 mv before after
mv/243925 renameat2(CWD, "before", CWD, "after", NOREPLACE) = 0
root@...ber:~# cat my-tmp-perf-config 
# this file is auto-generated.
[trace]
	show_arg_names = false
	show_duration = false
	show_timestamp = false
	args_alignment = 0
root@...ber:~#  
> 
> ⬢ [acme@...lbox perf-tools-next]$ rm -f 0001-* ; git format-patch --no-cover-letter HEAD~ ; scripts/checkpatch.pl 0001-*
> 0001-perf-trace-Add-tests-for-BTF-general-augmentation.patch
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #31: 
> new file mode 100755
> 
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 2
> #37: FILE: tools/perf/tests/shell/trace_btf_general.sh:2:
> +# perf trace BTF general tests
> 
> WARNING: Misplaced SPDX-License-Identifier tag - use line 2 instead
> #38: FILE: tools/perf/tests/shell/trace_btf_general.sh:3:
> +# SPDX-License-Identifier: GPL-2.0
> 
> WARNING: line length of 252 exceeds 100 columns
> #55: FILE: tools/perf/tests/shell/trace_btf_general.sh:20:
> +  if ! perf trace -e renameat* --max-events=1 -- mv ${file1} ${file2} 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +mv\/[0-9]+ renameat(2)?\(olddfd: .*, oldname: \"${file1}\", newdfd: .*, newname: \"${file2}\", flags: .*\) += +[0-9]+$"
> 
> WARNING: line length of 203 exceeds 100 columns
> #65: FILE: tools/perf/tests/shell/trace_btf_general.sh:30:
> +  if ! perf trace -e write --max-events=1 -- echo "${buffer}" 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +echo\/[0-9]+ write\(fd: [0-9]+, buf: ${buffer}.*, count: [0-9]+\) += +[0-9]+$"
> 
> WARNING: line length of 275 exceeds 100 columns
> #74: FILE: tools/perf/tests/shell/trace_btf_general.sh:39:
> +  if ! perf trace -e clock_nanosleep --force-btf --max-events=1 -- sleep 1 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +sleep\/[0-9]+ clock_nanosleep\(rqtp: \(struct __kernel_timespec\)\{\.tv_sec = \(__kernel_time64_t\)1,\}, rmtp: 0x[0-9a-f]+\) += +[0-9]+$"
> 
> total: 0 errors, 6 warnings, 68 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> 0001-perf-trace-Add-tests-for-BTF-general-augmentation.patch has style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
> ⬢ [acme@...lbox perf-tools-next]$
> 
> > Signed-off-by: Howard Chu <howardchu95@...il.com>
> > ---
> >  tools/perf/tests/shell/trace_btf_general.sh | 68 +++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >  create mode 100755 tools/perf/tests/shell/trace_btf_general.sh
> > 
> > diff --git a/tools/perf/tests/shell/trace_btf_general.sh b/tools/perf/tests/shell/trace_btf_general.sh
> > new file mode 100755
> > index 000000000000..7bcca81a40d8
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/trace_btf_general.sh
> > @@ -0,0 +1,68 @@
> > +#!/bin/bash
> > +# perf trace BTF general tests
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +err=0
> > +set -e
> > +
> > +. "$(dirname $0)"/lib/probe.sh
> > +skip_if_no_perf_trace || exit 2
> > +
> > +file1=$(mktemp /tmp/file1_XXXXX)
> > +file2=$(echo $file1 | sed 's/file1/file2/g')
> > +
> > +buffer="the content of the buffer"
> > +
> > +trap cleanup EXIT TERM INT HUP
> > +
> > +trace_test_string() {
> > +  echo "Testing perf trace's string augmentation"
> > +  if ! perf trace -e renameat* --max-events=1 -- mv ${file1} ${file2} 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +mv\/[0-9]+ renameat(2)?\(olddfd: .*, oldname: \"${file1}\", newdfd: .*, newname: \"${file2}\", flags: .*\) += +[0-9]+$"
> > +  then
> > +    echo "String augmentation test failed"
> > +    err=1
> > +  fi
> > +}
> > +
> > +trace_test_buffer() {
> > +  echo "Testing perf trace's buffer augmentation"
> > +  # echo will insert a newline (\10) at the end of the buffer
> > +  if ! perf trace -e write --max-events=1 -- echo "${buffer}" 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +echo\/[0-9]+ write\(fd: [0-9]+, buf: ${buffer}.*, count: [0-9]+\) += +[0-9]+$"
> > +  then
> > +    echo "Buffer augmentation test failed"
> > +    err=1
> > +  fi
> > +}
> > +
> > +trace_test_struct_btf() {
> > +  echo "Testing perf trace's struct augmentation"
> > +  if ! perf trace -e clock_nanosleep --force-btf --max-events=1 -- sleep 1 2>&1 | grep -q -E " +[0-9]+\.[0-9]+ +\( *[0-9]+\.[0-9]+ ms\): +sleep\/[0-9]+ clock_nanosleep\(rqtp: \(struct __kernel_timespec\)\{\.tv_sec = \(__kernel_time64_t\)1,\}, rmtp: 0x[0-9a-f]+\) += +[0-9]+$"
> > +  then
> > +    echo "BTF struct augmentation test failed"
> > +    err=1
> > +  fi
> > +}
> > +
> > +cleanup() {
> > +	rm -rf ${file1} ${file2}
> > +}
> > +
> > +trap_cleanup() {
> > +	echo "Unexpected signal in ${FUNCNAME[1]}"
> > +	cleanup
> > +	exit 1
> > +}
> > +
> > +trace_test_string
> > +
> > +if [ $err = 0 ]; then
> > +  trace_test_buffer
> > +fi
> > +
> > +if [ $err = 0 ]; then
> > +  trace_test_struct_btf
> > +fi
> > +
> > +cleanup
> > +
> > +exit $err
> > -- 
> > 2.43.0

Powered by blists - more mailing lists