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

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

[vmolnaro]

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
---
 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)
-    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
 
-- 
2.43.0

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

[Veronika Molnarova]

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?

Thanks,
Veronika
> ---
>  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)
> -    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
>  

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

[Namhyung Kim]

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
> >  
> 

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

[Namhyung Kim]

On Tue, Oct 01, 2024 at 05:25:55PM -0700, Namhyung Kim wrote:
> 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>

Also, please collect reviewers' tags in the previous versions like
Reviewed-by and Tested-by from Kajol Jain.

Thanks,
Namhyung