* [PATCH v1] perf tests: Harden branch stack sampling test
@ 2025-03-12 5:58 Ian Rogers
2025-03-12 10:54 ` Leo Yan
0 siblings, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2025-03-12 5:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, James Clark,
Anshuman Khandual, linux-perf-users, linux-kernel, German Gomez
On continuous testing the perf script output can be empty, or nearly
empty, causing tr/grep to exit and due to "set -e" the test traps and
fails. Add some empty file handling that sets the test to skip and
make grep and other text rewriting failures non-fatal by adding
"|| true".
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/tests/shell/test_brstack.sh | 79 +++++++++++++++++++-------
1 file changed, 59 insertions(+), 20 deletions(-)
diff --git a/tools/perf/tests/shell/test_brstack.sh b/tools/perf/tests/shell/test_brstack.sh
index e01df7581393..6f5ae227b3e8 100755
--- a/tools/perf/tests/shell/test_brstack.sh
+++ b/tools/perf/tests/shell/test_brstack.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
# Check branch stack sampling
# SPDX-License-Identifier: GPL-2.0
@@ -17,35 +17,54 @@ fi
skip_test_missing_symbol brstack_bench
+set -x
+err=0
TMPDIR=$(mktemp -d /tmp/__perf_test.program.XXXXX)
TESTPROG="perf test -w brstack"
cleanup() {
rm -rf $TMPDIR
+ trap - EXIT TERM INT
}
-trap cleanup EXIT TERM INT
+trap_cleanup() {
+ set +e
+ echo "Unexpected signal in ${FUNCNAME[1]}"
+ cleanup
+ exit 1
+}
+trap trap_cleanup EXIT TERM INT
test_user_branches() {
echo "Testing user branch stack sampling"
- perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u -- ${TESTPROG} > /dev/null 2>&1
- perf script -i $TMPDIR/perf.data --fields brstacksym | tr -s ' ' '\n' > $TMPDIR/perf.script
+ perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
+ perf script -i "$TMPDIR/perf.data" --fields brstacksym > "$TMPDIR/perf.script"
# example of branch entries:
# brstack_foo+0x14/brstack_bar+0x40/P/-/-/0/CALL
- set -x
- grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$" $TMPDIR/perf.script
- grep -E -m1 "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$" $TMPDIR/perf.script
- set +x
-
+ expected=(
+ "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$"
+ "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
+ "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$"
+ "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
+ "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$"
+ "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$"
+ "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$"
+ "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$"
+ )
+ for x in "${expected[@]}"
+ do
+ if ! tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep -E -m1 -q "$x"
+ then
+ echo "Branches missing $x"
+ if [ "x$err" == "x0" ]
+ then
+ err=2
+ fi
+ fi
+ done
# some branch types are still not being tested:
# IND COND_CALL COND_RET SYSCALL SYSRET IRQ SERROR NO_TX
}
@@ -57,14 +76,31 @@ test_filter() {
test_filter_expect=$2
echo "Testing branch stack filtering permutation ($test_filter_filter,$test_filter_expect)"
-
- perf record -o $TMPDIR/perf.data --branch-filter $test_filter_filter,save_type,u -- ${TESTPROG} > /dev/null 2>&1
- perf script -i $TMPDIR/perf.data --fields brstack | tr -s ' ' '\n' | grep '.' > $TMPDIR/perf.script
+ perf record -o "$TMPDIR/perf.data" --branch-filter "$test_filter_filter,save_type,u" -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
+ perf script -i "$TMPDIR/perf.data" --fields brstack > "$TMPDIR/perf.script"
# fail if we find any branch type that doesn't match any of the expected ones
# also consider UNKNOWN branch types (-)
- if grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$" $TMPDIR/perf.script; then
- return 1
+ if [ ! -s "$TMPDIR/perf.script" ]
+ then
+ echo "Empty script output"
+ if [ "x$err" == "x0" ]
+ then
+ err=2
+ fi
+ return
+ fi
+ # Look for lines not matching test_filter_expect ignoring issues caused
+ # by empty output
+ tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep '.' | \
+ grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$" \
+ > "$TMPDIR/perf.script-filtered" || true
+ if [ -s "$TMPDIR/perf.script-filtered" ]
+ then
+ echo "Unexpected branch filter in script output"
+ cat "$TMPDIR/perf.script"
+ err=1
+ return
fi
}
@@ -80,3 +116,6 @@ test_filter "any_ret" "RET|COND_RET|SYSRET|ERET"
test_filter "call,cond" "CALL|SYSCALL|COND"
test_filter "any_call,cond" "CALL|IND_CALL|COND_CALL|IRQ|SYSCALL|COND"
test_filter "cond,any_call,any_ret" "COND|CALL|IND_CALL|COND_CALL|SYSCALL|IRQ|RET|COND_RET|SYSRET|ERET"
+
+cleanup
+exit $err
--
2.49.0.rc0.332.g42c0ae87b1-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] perf tests: Harden branch stack sampling test
2025-03-12 5:58 [PATCH v1] perf tests: Harden branch stack sampling test Ian Rogers
@ 2025-03-12 10:54 ` Leo Yan
2025-03-13 20:18 ` Namhyung Kim
0 siblings, 1 reply; 6+ messages in thread
From: Leo Yan @ 2025-03-12 10:54 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, Kan Liang, James Clark, Anshuman Khandual,
linux-perf-users, linux-kernel, German Gomez
Hi Ian,
On Tue, Mar 11, 2025 at 10:58:46PM -0700, Ian Rogers wrote:
> On continuous testing the perf script output can be empty, or nearly
> empty, causing tr/grep to exit and due to "set -e" the test traps and
> fails. Add some empty file handling that sets the test to skip and
> make grep and other text rewriting failures non-fatal by adding
> "|| true".
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/tests/shell/test_brstack.sh | 79 +++++++++++++++++++-------
> 1 file changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/tests/shell/test_brstack.sh b/tools/perf/tests/shell/test_brstack.sh
> index e01df7581393..6f5ae227b3e8 100755
> --- a/tools/perf/tests/shell/test_brstack.sh
> +++ b/tools/perf/tests/shell/test_brstack.sh
> @@ -1,4 +1,4 @@
> -#!/bin/sh
> +#!/bin/bash
> # Check branch stack sampling
>
> # SPDX-License-Identifier: GPL-2.0
> @@ -17,35 +17,54 @@ fi
>
> skip_test_missing_symbol brstack_bench
>
> +set -x
> +err=0
> TMPDIR=$(mktemp -d /tmp/__perf_test.program.XXXXX)
> TESTPROG="perf test -w brstack"
>
> cleanup() {
> rm -rf $TMPDIR
> + trap - EXIT TERM INT
> }
>
> -trap cleanup EXIT TERM INT
> +trap_cleanup() {
> + set +e
> + echo "Unexpected signal in ${FUNCNAME[1]}"
> + cleanup
> + exit 1
> +}
> +trap trap_cleanup EXIT TERM INT
>
> test_user_branches() {
> echo "Testing user branch stack sampling"
>
> - perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u -- ${TESTPROG} > /dev/null 2>&1
> - perf script -i $TMPDIR/perf.data --fields brstacksym | tr -s ' ' '\n' > $TMPDIR/perf.script
> + perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
> + perf script -i "$TMPDIR/perf.data" --fields brstacksym > "$TMPDIR/perf.script"
>
> # example of branch entries:
> # brstack_foo+0x14/brstack_bar+0x40/P/-/-/0/CALL
>
> - set -x
> - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$" $TMPDIR/perf.script
> - grep -E -m1 "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$" $TMPDIR/perf.script
> - set +x
> -
> + expected=(
> + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$"
> + "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$"
> + "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> + "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$"
> + "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$"
> + "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$"
> + "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$"
> + )
> + for x in "${expected[@]}"
> + do
> + if ! tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep -E -m1 -q "$x"
> + then
> + echo "Branches missing $x"
> + if [ "x$err" == "x0" ]
> + then
> + err=2
Here it sets "err=2", as a result, if any grep command fails, the script
exits while reporting to skip the test. This seems incorrect to me.
My understanding is the regular expressions above are mandatory to be
matched, otherwise, it must be something is wrong. We should not skip
the test in this case.
I can understand that 'perf record' cannot record all branch types, if
this is the case, maybe we can improve the recording quality rather
than reporting skip? E.g.,
cat <<EOF > "$TMPDIR/loop.sh"
for run in {1..5}; do perf test -w brstack; done
EOF
perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u
-- sh $TMPDIR/loop.sh
If we run the test for 5 times, should this can allow us to ensure the
branch samples are recorded?
> + fi
> + fi
> + done
> # some branch types are still not being tested:
> # IND COND_CALL COND_RET SYSCALL SYSRET IRQ SERROR NO_TX
> }
> @@ -57,14 +76,31 @@ test_filter() {
> test_filter_expect=$2
>
> echo "Testing branch stack filtering permutation ($test_filter_filter,$test_filter_expect)"
> -
> - perf record -o $TMPDIR/perf.data --branch-filter $test_filter_filter,save_type,u -- ${TESTPROG} > /dev/null 2>&1
> - perf script -i $TMPDIR/perf.data --fields brstack | tr -s ' ' '\n' | grep '.' > $TMPDIR/perf.script
> + perf record -o "$TMPDIR/perf.data" --branch-filter "$test_filter_filter,save_type,u" -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
> + perf script -i "$TMPDIR/perf.data" --fields brstack > "$TMPDIR/perf.script"
>
> # fail if we find any branch type that doesn't match any of the expected ones
> # also consider UNKNOWN branch types (-)
> - if grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$" $TMPDIR/perf.script; then
> - return 1
> + if [ ! -s "$TMPDIR/perf.script" ]
> + then
> + echo "Empty script output"
> + if [ "x$err" == "x0" ]
> + then
> + err=2
> + fi
> + return
> + fi
> + # Look for lines not matching test_filter_expect ignoring issues caused
> + # by empty output
> + tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep '.' | \
> + grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$" \
> + > "$TMPDIR/perf.script-filtered" || true
> + if [ -s "$TMPDIR/perf.script-filtered" ]
> + then
> + echo "Unexpected branch filter in script output"
> + cat "$TMPDIR/perf.script"
> + err=1
> + return
> fi
> }
>
> @@ -80,3 +116,6 @@ test_filter "any_ret" "RET|COND_RET|SYSRET|ERET"
> test_filter "call,cond" "CALL|SYSCALL|COND"
> test_filter "any_call,cond" "CALL|IND_CALL|COND_CALL|IRQ|SYSCALL|COND"
> test_filter "cond,any_call,any_ret" "COND|CALL|IND_CALL|COND_CALL|SYSCALL|IRQ|RET|COND_RET|SYSRET|ERET"
> +
> +cleanup
> +exit $err
> --
> 2.49.0.rc0.332.g42c0ae87b1-goog
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] perf tests: Harden branch stack sampling test
2025-03-12 10:54 ` Leo Yan
@ 2025-03-13 20:18 ` Namhyung Kim
2025-03-14 9:13 ` Leo Yan
0 siblings, 1 reply; 6+ messages in thread
From: Namhyung Kim @ 2025-03-13 20:18 UTC (permalink / raw)
To: Leo Yan
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Kan Liang, James Clark, Anshuman Khandual, linux-perf-users,
linux-kernel, German Gomez
Hello,
On Wed, Mar 12, 2025 at 10:54:50AM +0000, Leo Yan wrote:
> Hi Ian,
>
> On Tue, Mar 11, 2025 at 10:58:46PM -0700, Ian Rogers wrote:
> > On continuous testing the perf script output can be empty, or nearly
> > empty, causing tr/grep to exit and due to "set -e" the test traps and
> > fails. Add some empty file handling that sets the test to skip and
> > make grep and other text rewriting failures non-fatal by adding
> > "|| true".
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/tests/shell/test_brstack.sh | 79 +++++++++++++++++++-------
> > 1 file changed, 59 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/perf/tests/shell/test_brstack.sh b/tools/perf/tests/shell/test_brstack.sh
> > index e01df7581393..6f5ae227b3e8 100755
> > --- a/tools/perf/tests/shell/test_brstack.sh
> > +++ b/tools/perf/tests/shell/test_brstack.sh
> > @@ -1,4 +1,4 @@
> > -#!/bin/sh
> > +#!/bin/bash
> > # Check branch stack sampling
> >
> > # SPDX-License-Identifier: GPL-2.0
> > @@ -17,35 +17,54 @@ fi
> >
> > skip_test_missing_symbol brstack_bench
> >
> > +set -x
> > +err=0
> > TMPDIR=$(mktemp -d /tmp/__perf_test.program.XXXXX)
> > TESTPROG="perf test -w brstack"
> >
> > cleanup() {
> > rm -rf $TMPDIR
> > + trap - EXIT TERM INT
> > }
> >
> > -trap cleanup EXIT TERM INT
> > +trap_cleanup() {
> > + set +e
> > + echo "Unexpected signal in ${FUNCNAME[1]}"
> > + cleanup
> > + exit 1
> > +}
> > +trap trap_cleanup EXIT TERM INT
> >
> > test_user_branches() {
> > echo "Testing user branch stack sampling"
> >
> > - perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u -- ${TESTPROG} > /dev/null 2>&1
> > - perf script -i $TMPDIR/perf.data --fields brstacksym | tr -s ' ' '\n' > $TMPDIR/perf.script
> > + perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
> > + perf script -i "$TMPDIR/perf.data" --fields brstacksym > "$TMPDIR/perf.script"
> >
> > # example of branch entries:
> > # brstack_foo+0x14/brstack_bar+0x40/P/-/-/0/CALL
> >
> > - set -x
> > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$" $TMPDIR/perf.script
> > - grep -E -m1 "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$" $TMPDIR/perf.script
> > - set +x
> > -
> > + expected=(
> > + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$"
> > + "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> > + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$"
> > + "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> > + "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$"
> > + "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$"
> > + "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$"
> > + "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$"
> > + )
> > + for x in "${expected[@]}"
> > + do
> > + if ! tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep -E -m1 -q "$x"
> > + then
> > + echo "Branches missing $x"
> > + if [ "x$err" == "x0" ]
> > + then
> > + err=2
>
> Here it sets "err=2", as a result, if any grep command fails, the script
> exits while reporting to skip the test. This seems incorrect to me.
>
> My understanding is the regular expressions above are mandatory to be
> matched, otherwise, it must be something is wrong. We should not skip
> the test in this case.
>
> I can understand that 'perf record' cannot record all branch types, if
> this is the case, maybe we can improve the recording quality rather
> than reporting skip? E.g.,
>
> cat <<EOF > "$TMPDIR/loop.sh"
> for run in {1..5}; do perf test -w brstack; done
> EOF
>
> perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u
> -- sh $TMPDIR/loop.sh
>
> If we run the test for 5 times, should this can allow us to ensure the
> branch samples are recorded?
The brstack (and other workload programs) can take an argument to
control its duration. For brstack, it's the number of loop iteration
and default is 999999.
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] perf tests: Harden branch stack sampling test
2025-03-13 20:18 ` Namhyung Kim
@ 2025-03-14 9:13 ` Leo Yan
2025-03-17 15:38 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Leo Yan @ 2025-03-14 9:13 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Kan Liang, James Clark, Anshuman Khandual, linux-perf-users,
linux-kernel, German Gomez
Hi Namhyung,
On Thu, Mar 13, 2025 at 01:18:08PM -0700, Namhyung Kim wrote:
[...]
> > > test_user_branches() {
> > > echo "Testing user branch stack sampling"
> > >
> > > - perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u -- ${TESTPROG} > /dev/null 2>&1
> > > - perf script -i $TMPDIR/perf.data --fields brstacksym | tr -s ' ' '\n' > $TMPDIR/perf.script
> > > + perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
> > > + perf script -i "$TMPDIR/perf.data" --fields brstacksym > "$TMPDIR/perf.script"
> > >
> > > # example of branch entries:
> > > # brstack_foo+0x14/brstack_bar+0x40/P/-/-/0/CALL
> > >
> > > - set -x
> > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$" $TMPDIR/perf.script
> > > - grep -E -m1 "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$" $TMPDIR/perf.script
> > > - set +x
> > > -
> > > + expected=(
> > > + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$"
> > > + "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> > > + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$"
> > > + "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> > > + "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$"
> > > + "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$"
> > > + "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$"
> > > + "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$"
> > > + )
> > > + for x in "${expected[@]}"
> > > + do
> > > + if ! tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep -E -m1 -q "$x"
> > > + then
> > > + echo "Branches missing $x"
> > > + if [ "x$err" == "x0" ]
> > > + then
> > > + err=2
> >
> > Here it sets "err=2", as a result, if any grep command fails, the script
> > exits while reporting to skip the test. This seems incorrect to me.
> >
> > My understanding is the regular expressions above are mandatory to be
> > matched, otherwise, it must be something is wrong. We should not skip
> > the test in this case.
> >
> > I can understand that 'perf record' cannot record all branch types, if
> > this is the case, maybe we can improve the recording quality rather
> > than reporting skip? E.g.,
> >
> > cat <<EOF > "$TMPDIR/loop.sh"
> > for run in {1..5}; do perf test -w brstack; done
> > EOF
> >
> > perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u
> > -- sh $TMPDIR/loop.sh
> >
> > If we run the test for 5 times, should this can allow us to ensure the
> > branch samples are recorded?
>
> The brstack (and other workload programs) can take an argument to
> control its duration. For brstack, it's the number of loop iteration
> and default is 999999.
Sorry I did not dig into the brstack workload program.
If the workload has run for a large number of loops, the question is:
why isn't the test capturing the expected branch stacks?
Thanks,
Leo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] perf tests: Harden branch stack sampling test
2025-03-14 9:13 ` Leo Yan
@ 2025-03-17 15:38 ` Ian Rogers
2025-03-18 11:25 ` Leo Yan
0 siblings, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2025-03-17 15:38 UTC (permalink / raw)
To: Leo Yan
Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Adrian Hunter, Kan Liang, James Clark,
Anshuman Khandual, linux-perf-users, linux-kernel, German Gomez
On Fri, Mar 14, 2025 at 2:13 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Hi Namhyung,
>
> On Thu, Mar 13, 2025 at 01:18:08PM -0700, Namhyung Kim wrote:
>
> [...]
>
> > > > test_user_branches() {
> > > > echo "Testing user branch stack sampling"
> > > >
> > > > - perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u -- ${TESTPROG} > /dev/null 2>&1
> > > > - perf script -i $TMPDIR/perf.data --fields brstacksym | tr -s ' ' '\n' > $TMPDIR/perf.script
> > > > + perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u -- ${TESTPROG} > "$TMPDIR/record.txt" 2>&1
> > > > + perf script -i "$TMPDIR/perf.data" --fields brstacksym > "$TMPDIR/perf.script"
> > > >
> > > > # example of branch entries:
> > > > # brstack_foo+0x14/brstack_bar+0x40/P/-/-/0/CALL
> > > >
> > > > - set -x
> > > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$" $TMPDIR/perf.script
> > > > - grep -E -m1 "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$" $TMPDIR/perf.script
> > > > - set +x
> > > > -
> > > > + expected=(
> > > > + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/IND_CALL/.*$"
> > > > + "^brstack_foo\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> > > > + "^brstack_bench\+[^ ]*/brstack_foo\+[^ ]*/CALL/.*$"
> > > > + "^brstack_bench\+[^ ]*/brstack_bar\+[^ ]*/CALL/.*$"
> > > > + "^brstack_bar\+[^ ]*/brstack_foo\+[^ ]*/RET/.*$"
> > > > + "^brstack_foo\+[^ ]*/brstack_bench\+[^ ]*/RET/.*$"
> > > > + "^brstack_bench\+[^ ]*/brstack_bench\+[^ ]*/COND/.*$"
> > > > + "^brstack\+[^ ]*/brstack\+[^ ]*/UNCOND/.*$"
> > > > + )
> > > > + for x in "${expected[@]}"
> > > > + do
> > > > + if ! tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep -E -m1 -q "$x"
> > > > + then
> > > > + echo "Branches missing $x"
> > > > + if [ "x$err" == "x0" ]
> > > > + then
> > > > + err=2
> > >
> > > Here it sets "err=2", as a result, if any grep command fails, the script
> > > exits while reporting to skip the test. This seems incorrect to me.
> > >
> > > My understanding is the regular expressions above are mandatory to be
> > > matched, otherwise, it must be something is wrong. We should not skip
> > > the test in this case.
> > >
> > > I can understand that 'perf record' cannot record all branch types, if
> > > this is the case, maybe we can improve the recording quality rather
> > > than reporting skip? E.g.,
> > >
> > > cat <<EOF > "$TMPDIR/loop.sh"
> > > for run in {1..5}; do perf test -w brstack; done
> > > EOF
> > >
> > > perf record -o "$TMPDIR/perf.data" --branch-filter any,save_type,u
> > > -- sh $TMPDIR/loop.sh
> > >
> > > If we run the test for 5 times, should this can allow us to ensure the
> > > branch samples are recorded?
> >
> > The brstack (and other workload programs) can take an argument to
> > control its duration. For brstack, it's the number of loop iteration
> > and default is 999999.
>
> Sorry I did not dig into the brstack workload program.
>
> If the workload has run for a large number of loops, the question is:
> why isn't the test capturing the expected branch stacks?
On our testing skipped == failed, I can change 2 to 1 above but I'd
made it 2 as it wasn't clear to me all branch filter types would be
supported by perf record and skipping/2 was a less terrible error
message.
I'm keen to land the pulling apart of the perf command from the
tr/grep as if we hit say an asan error currently that is hidden by
code like:
```
perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u --
${TESTPROG} > /dev/null 2>&1
```
where all the output is sent to /dev/null but the asan error code will
cause the "set -e" to fail. If this code fails with asan then
currently the first thing to do is start pulling apart the
expressions.
Code like:
```
perf script -i $TMPDIR/perf.data --fields brstack | tr -s ' ' '\n' |
grep '.' > $TMPDIR/perf.script
```
is problematic as again we lose the asan like errors. Running the previous:
```
if grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$"
$TMPDIR/perf.script; then
```
could fail because of an unexpected branch filter type, but was
failing for me just because there were blank or similar lines in the
output. The new code doesn't change this but allows the output to be
dumped for later diagnostics. The '|| true' in the expression means we
get to dumping the diagnostics and dump just fail because some
sub-command mismatched its input.
Thanks,
Ian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] perf tests: Harden branch stack sampling test
2025-03-17 15:38 ` Ian Rogers
@ 2025-03-18 11:25 ` Leo Yan
0 siblings, 0 replies; 6+ messages in thread
From: Leo Yan @ 2025-03-18 11:25 UTC (permalink / raw)
To: Ian Rogers
Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Adrian Hunter, Kan Liang, James Clark,
Anshuman Khandual, linux-perf-users, linux-kernel, German Gomez
On Mon, Mar 17, 2025 at 08:38:44AM -0700, Ian Rogers wrote:
[...]
> On our testing skipped == failed, I can change 2 to 1 above but I'd
> made it 2 as it wasn't clear to me all branch filter types would be
> supported by perf record and skipping/2 was a less terrible error
> message.
My concern is changing from returned val from 1 to 2 will lead to CI
to never report errors.
> I'm keen to land the pulling apart of the perf command from the
> tr/grep as if we hit say an asan error currently that is hidden by
> code like:
> ```
> perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u --
> ${TESTPROG} > /dev/null 2>&1
> ```
> where all the output is sent to /dev/null but the asan error code will
> cause the "set -e" to fail. If this code fails with asan then
> currently the first thing to do is start pulling apart the
> expressions.
Agreed the most part in this patch, except returns err=2 in
test_user_branches() and test_filter().
> Code like:
> ```
> perf script -i $TMPDIR/perf.data --fields brstack | tr -s ' ' '\n' |
> grep '.' > $TMPDIR/perf.script
> ```
> is problematic as again we lose the asan like errors. Running the previous:
> ```
> if grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$"
> $TMPDIR/perf.script; then
> ```
> could fail because of an unexpected branch filter type, but was
> failing for me just because there were blank or similar lines in the
> output.
I saw you changed the command as:
tr -s ' ' '\n' < "$TMPDIR/perf.script" | grep '.' | \
grep -E -vm1 "^[^ ]*/($test_filter_expect|-|( *))/.*$" \
> "$TMPDIR/perf.script-filtered" || true
This is a good change for me. After we removed the noises caused by
spaces and empty lines, for an empty script output, I still think we
should report error, as the test fails to capture any branch stack.
> The new code doesn't change this but allows the output to be
> dumped for later diagnostics. The '|| true' in the expression means we
> get to dumping the diagnostics and dump just fail because some
> sub-command mismatched its input.
Yeah, agreed '|| true' is a good improvement.
Thanks,
Leo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-18 11:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-12 5:58 [PATCH v1] perf tests: Harden branch stack sampling test Ian Rogers
2025-03-12 10:54 ` Leo Yan
2025-03-13 20:18 ` Namhyung Kim
2025-03-14 9:13 ` Leo Yan
2025-03-17 15:38 ` Ian Rogers
2025-03-18 11:25 ` Leo Yan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).