Re: [PATCH] perf test stat_all_pmu.sh: Correctly check 'perf stat' result

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Thu, Jan 16, 2025 at 01:31:21PM +0100, Veronika Molnarova wrote:
> 
> 
> On 12/10/24 16:20, Veronika Molnarova wrote:
> > 
> > 
> > On 11/23/24 00:12, vmolnaro@redhat.com wrote:
> >> From: Veronika Molnarova <vmolnaro@redhat.com>
> >>
> >> Test case "stat_all_pmu.sh" is not correctly checking 'perf stat' output
> >> due to a poor design. Firstly, having the 'set -e' option with a trap
> >> catching the sigexit causes the shell to exit immediately if 'perf stat' ends
> >> with any non-zero value, which is then caught by the trap reporting an
> >> unexpected signal. This causes events that should be parsed by the if-else
> >> statement to be caught by the trap handler and are reported as errors:
> >>
> >>     $ perf test -vv "perf all pmu"
> >>     Testing i915/actual-frequency/
> >>     Unexpected signal in main
> >>     Error:
> >>     Access to performance monitoring and observability operations is limited.
> >>
> >> Secondly, the if-else branches are not exclusive as the checking if the
> >> event is present in the output log covers also the "<not supported>"
> >> events, which should be accepted, and also the "Bad name events", which
> >> should be rejected.
> >>
> >> Remove the "set -e" option from the test case, correctly parse the
> >> "perf stat" output log and check its return value. Add the missing
> >> outputs for the 'perf stat' result and also add logs messages to
> >> report the branch that parsed the event for more info.
> > 
> > Hi Ian,
> > 
> > is there anything that I am missing? If so would be great to know the idea
> > of your patch. This issue is getting quite old so would be great to get a proper
> > fix together.
> > 
> > Thanks,
> > Veronika
> 
> Hello,
> 
> just wanted to ping the patch. Would be great to sort it out and close the issue.

Ian, are you ok with this?

Thanks,
Namhyung
 
> >>
> >> Fixes: 7e73ea40295620e7 ("perf test: Ignore security failures in all PMU test")
> >> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> >> ---
> >>  tools/perf/tests/shell/stat_all_pmu.sh | 48 ++++++++++++++++++--------
> >>  1 file changed, 34 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/tools/perf/tests/shell/stat_all_pmu.sh b/tools/perf/tests/shell/stat_all_pmu.sh
> >> index 8b148b300be1131e..9c466c0efa857fc2 100755
> >> --- a/tools/perf/tests/shell/stat_all_pmu.sh
> >> +++ b/tools/perf/tests/shell/stat_all_pmu.sh
> >> @@ -2,7 +2,6 @@
> >>  # perf all PMU test (exclusive)
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  
> >> -set -e
> >>  err=0
> >>  result=""
> >>  
> >> @@ -16,34 +15,55 @@ trap trap_cleanup EXIT TERM INT
> >>  # Test all PMU events; however exclude parameterized ones (name contains '?')
> >>  for p in $(perf list --raw-dump pmu | sed 's/[[:graph:]]\+?[[:graph:]]\+[[:space:]]//g')
> >>  do
> >> -  echo "Testing $p"
> >> -  result=$(perf stat -e "$p" true 2>&1)
> >> -  if echo "$result" | grep -q "$p"
> >> +  echo -n "Testing $p -- "
> >> +  output=$(perf stat -e "$p" true 2>&1)
> >> +  stat_result=$?
> >> +  if echo "$output" | grep -q "$p"
> >>    then
> >>      # Event seen in output.
> >> -    continue
> >> -  fi
> >> -  if echo "$result" | grep -q "<not supported>"
> >> -  then
> >> -    # Event not supported, so ignore.
> >> -    continue
> >> +    if [ $stat_result -eq 0 ] && ! echo "$output" | grep -q "<not supported>"
> >> +    then
> >> +      # Event supported.
> >> +      echo "supported"
> >> +      continue
> >> +    elif echo "$output" | grep -q "<not supported>"
> >> +    then
> >> +      # Event not supported, so ignore.
> >> +      echo "not supported"
> >> +      continue
> >> +    elif echo "$output" | grep -q "No permission to enable"
> >> +    then
> >> +      # No permissions, so ignore.
> >> +      echo "no permission to enable"
> >> +      continue
> >> +    elif echo "$output" | grep -q "Bad event name"
> >> +    then
> >> +      # Non-existent event.
> >> +      echo "Error: Bad event name"
> >> +      echo "$output"
> >> +      err=1
> >> +      continue
> >> +    fi
> >>    fi
> >> -  if echo "$result" | grep -q "Access to performance monitoring and observability operations is limited."
> >> +
> >> +  if echo "$output" | grep -q "Access to performance monitoring and observability operations is limited."
> >>    then
> >>      # Access is limited, so ignore.
> >> +    echo "access limited"
> >>      continue
> >>    fi
> >>  
> >>    # We failed to see the event and it is supported. Possibly the workload was
> >>    # too small so retry with something longer.
> >> -  result=$(perf stat -e "$p" perf bench internals synthesize 2>&1)
> >> -  if echo "$result" | grep -q "$p"
> >> +  output=$(perf stat -e "$p" perf bench internals synthesize 2>&1)
> >> +  if echo "$output" | grep -q "$p"
> >>    then
> >>      # Event seen in output.
> >> +    echo "supported"
> >>      continue
> >>    fi
> >>    echo "Error: event '$p' not printed in:"
> >> -  echo "$result"
> >> +  echo "$output"
> >>    err=1
> >>  done
> >>  
>