Message-ID: <CAP-5=fWM2hqu02nNyBUBgLu01AC=C7mwxayezzs7frCyAsirPg@mail.gmail.com>
Date: Mon, 17 Mar 2025 08:38:44 -0700
From: Ian Rogers <irogers@...gle.com>
To: Leo Yan <leo.yan@....com>
Cc: Namhyung Kim <namhyung@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@...aro.org>, Anshuman Khandual <anshuman.khandual@....com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org, 
	German Gomez <german.gomez@....com>
Subject: Re: [PATCH v1] perf tests: Harden branch stack sampling test

On Fri, Mar 14, 2025 at 2:13 AM Leo Yan <leo.yan@....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

Powered by blists - more mailing lists