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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Tue, Apr 16, 2024 at 08:03:41AM -0700, Ian Rogers wrote:
> On Tue, Apr 9, 2024 at 6:03 AM <vmolnaro@redhat.com> wrote:
> >
> > From: Veronika Molnarova <vmolnaro@redhat.com>
> >
> > With the MR a381bd3615de6 ('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, however, exits immediately if any command exits with
> > a non-zero value due to 'set -e' option.
> >
> > 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 appriopriate action.
> 
> nit: s/appriopriate/appropriate/
> 
> There was a similar issue with metric groups that should be on its way
> to landing:
> https://lore.kernel.org/lkml/20240403164818.3431325-1-irogers@google.com/

Cool, looks similar indeed, Veronika, can you consider reviewing Ian's
patch and providing a Reviewed-by: for it?

And also please check Ian's suggestions for your patch, below:
 
> > ---
> >  tools/perf/tests/shell/stat_all_pmu.sh | 36 ++++++++++++++++++--------
> >  1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/perf/tests/shell/stat_all_pmu.sh b/tools/perf/tests/shell/stat_all_pmu.sh
> > index c77955419173..d9f0d2100baa 100755
> > --- a/tools/perf/tests/shell/stat_all_pmu.sh
> > +++ b/tools/perf/tests/shell/stat_all_pmu.sh
> > @@ -2,21 +2,35 @@
> >  # perf all PMU test
> >  # SPDX-License-Identifier: GPL-2.0
> >
> > -set -e
> >
> >  # Test all PMU events; however exclude parametrized 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)
> > -    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 gets 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 pemission for the event is not set
> 
> nit: s/pemission/permission/
> 
> > +    elif [ $stat_result -eq 255 ] && echo "$stat_output" | grep -q "No permission"; then
> > +        echo "no permission to enable"
> > +    # 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
> 
> Something we've been seeing with parallel testing is getting busy
> PMUs. The error looks like:
> ```
> $ sudo bash -c "perf stat -e intel_bts// -a sleep 1; echo $?" & sudo
> bash -c "perf stat -e intel_bts// -a sleep 1; echo $?"
> [2] 953102
> Error:
> The sys_perf_event_open() syscall returned with 16 (Device or resource
> busy) for event (intel_bts//).
> /bin/dmesg | grep -i perf may provide additional information.
> 
> 255
> 
> Performance counter stats for 'system wide':
> 
>     <not counted>      intel_bts//
>                         (0.00%)
> 
>       1.002334792 seconds time elapsed
> 
> 0
> [2]+  Done                    sudo bash -c "perf stat -e intel_bts//
> -a sleep 1; echo $?"
> ```
> 
> To avoid failing for this we probably need something like:
> ```
>    elif [ $stat_result -eq 255 ] && echo "$stat_output" | grep -q
> "Device or resource busy"; then
>        echo "device busy"
> ```
> It'd be great if you could add this.
> 
> Thanks,
> Ian
> 
> > +    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
> >
> > --
> > 2.43.0
> >
> >