Re: [PATCH v4] perf test stat_all_pmu.sh: Parse return value of perf stat

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Oct 01, 2024 at 10:35:33AM +0200, Veronika Molnarova wrote:
> 
> 
> On 8/14/24 19:01, vmolnaro@redhat.com wrote:
> > From: Veronika Molnarova <vmolnaro@redhat.com>
> > 
> > With the upstream MR !3916 of commit ad86d7ee43b22aa2 ('powerpc/hv-gpci:
> > Fix the H_GET_PERF_COUNTER_INFO hcall return value checks') the perf
> > stat for hv_gpci events without required permission set returns an
> > error value of -1 to differentiate the output from the unsupported
> > events. The stat_all_pmu test was designed in a way, that if any
> > command exits with a non-zero value the test exits with an error
> > value without any information provided due to the 'set -e' option.
> > 
> > The issue propagated upstream as the list of the files that affected perf
> > did not cover the changed files and was updated after the issue was
> > discovered. It was caught by CKI testing where it was triaged to stop
> > blocking further MRs, as most of the powerpc machines have some of
> > the hv_gpci events without the required permissions.
> > 
> > Remove the 'set -e' option from the test and rework the test case to log
> > the status of the event for better maintainability. Instead of exiting
> > immediately after 'perf stat' ends with a non-zero value, check the
> > return value and output of the 'perf stat' command with the appropriate action.
> > 
> > Link to the MR !3916 of commit ad86d7ee43b22aa2:
> > https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Michael Petlan <mpetlan@redhat.com>
> > Cc: Radostin Stoyanov <rstoyano@redhat.com>
> > Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> > ---
> > v2: Fix commit hash
> > v3: Add 'device busy' for parallel testing
> > v4: Change commit message
> 
> Is this version of the commit viable?

Sorry but we have a conflicting change in this area.  Can you please
rebase?

> > ---
> >  tools/perf/tests/shell/stat_all_pmu.sh | 41 ++++++++++++++++++--------
> >  1 file changed, 29 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/perf/tests/shell/stat_all_pmu.sh b/tools/perf/tests/shell/stat_all_pmu.sh
> > index c77955419173..a75beddda4db 100755
> > --- a/tools/perf/tests/shell/stat_all_pmu.sh
> > +++ b/tools/perf/tests/shell/stat_all_pmu.sh
> > @@ -2,21 +2,38 @@
> >  # perf all PMU test
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -set -e
> >  
> >  # 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 "$result" | grep -q "<not supported>" ; then
> > -    # 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)

I think you removed this part.  Please check.

Thanks,
Namhyung


> > -    if ! echo "$result" | grep -q "$p" ; then
> > -      echo "Event '$p' not printed in:"
> > -      echo "$result"
> > -      exit 1
> > +  echo -n "Testing event '$p' -- "
> > +  stat_output=$(perf stat -e "$p" true 2>&1)
> > +  stat_result=$?
> > +  if echo "$stat_output" | grep -q "$p"; then
> > +    # return value 0 if counters get printed either if the event is supported or not
> > +    if [ $stat_result -eq 0 ] && ! echo "$stat_output" | grep -q "<not supported>"; then
> > +        echo "supported"
> > +    elif [ $stat_result -eq 0 ]; then
> > +        echo "not supported"
> > +    # return value 255 when the required permission for the event is not set
> > +    elif [ $stat_result -eq 255 ] && echo "$stat_output" | grep -q "No permission"; then
> > +        echo "no permission to enable"
> > +    # return value 255 in case of resource busy during parallel testing of events
> > +    elif [ $stat_result -eq 255 ] && echo "$stat_output" | grep -q "Device or resource busy"; then
> > +        echo "resource busy"
> > +    # return value 129 when trying to run 'perf stat' with a non-existent event
> > +    elif [ $stat_result -eq 129 ] && echo "$stat_output" | grep -q "Bad event name"; then
> > +        echo "Fail: Bad event name"
> > +        echo "$stat_output"
> > +        exit 1
> > +    else
> > +        echo "Fail: Unknown return value $stat_result"
> > +        echo "$stat_output"
> > +        exit 1
> >      fi
> > +  else
> > +      echo "Fail: Event '$p' not printed in:"
> > +      echo "$stat_output"
> > +      exit 1
> >    fi
> >  done
> >  
>