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

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

[vmolnaro]

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.
---
 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
+    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
+    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] perf test stat_all_pmu.sh: Parse return value of perf stat

[Arnaldo Carvalho de Melo]

On Tue, Apr 09, 2024 at 03:02:40PM +0200, vmolnaro@redhat.com wrote:
> From: Veronika Molnarova <vmolnaro@redhat.com>
> 
> With the MR a381bd3615de6 ('powerpc/hv-gpci: Fix the

Is this MR publicly accessible? If so, please provide an URL, otherwise
it will be interesting to state if this problem takes place upstream and
how to reproduce it, do we need a specific environmnt (I assume so, from
looking at "powerpc" in the mentioned MR, etc.

- Arnaldo

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

[PATCH v2] 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 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 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.

Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
event causes failure after the MR as the zero return value was required.
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 do not support
some of the hv_gpci events.

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 a381bd3615de6:
https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916

Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
---
 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
+    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
+    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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Arnaldo Carvalho de Melo]

On Mon, Apr 15, 2024 at 11:42:20AM +0200, vmolnaro@redhat.com wrote:
> From: Veronika Molnarova <vmolnaro@redhat.com>
> 
> With the upstream MR !3916 of commit a381bd3615de6 ('powerpc/hv-gpci:

I'm still not finding this commit upstream:

⬢[acme@toolbox perf-tools-next]$ git remote update torvalds
Fetching torvalds
remote: Enumerating objects: 298, done.
remote: Counting objects: 100% (173/173), done.
remote: Compressing objects: 100% (18/18), done.
remote: Total 298 (delta 156), reused 168 (delta 155), pack-reused 125
Receiving objects: 100% (298/298), 208.17 KiB | 859.00 KiB/s, done.
Resolving deltas: 100% (213/213), completed with 71 local objects.
From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux
   586b5dfb51b962c1..0bbac3facb5d6cc0  master     -> torvalds/master
 * [new tag]                           v6.9-rc4   -> v6.9-rc4
⬢[acme@toolbox perf-tools-next]$ git show a381bd3615de6
fatal: ambiguous argument 'a381bd3615de6': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
⬢[acme@toolbox perf-tools-next]$

⬢[acme@toolbox perf-tools-next]$

IS this one:

⬢[acme@toolbox perf-tools-next]$ git log | grep "Fix the H_GET_PERF_COUNTER_INFO hcall return value checks"
      powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks
    powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks
^C
⬢[acme@toolbox perf-tools-next]$

⬢[acme@toolbox perf-tools-next]$ git log | grep -B4 "Fix the H_GET_PERF_COUNTER_INFO hcall return value checks"
      powerpc/fsl: Fix mfpmr build errors with newer binutils
      powerpc/64s: Use .machine power4 around dcbt
      powerpc/64s: Move dcbt/dcbtst sequence into a macro
      powerpc/mm: Code cleanup for __hash_page_thp
      powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks
--
commit ad86d7ee43b22aa2ed60fb982ae94b285c1be671
Author: Kajol Jain <kjain@linux.ibm.com>
Date:   Thu Feb 29 17:58:47 2024 +0530

    powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks
^C
⬢[acme@toolbox perf-tools-next]$

I think it is, now looking at
https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
I see the reference to ad86d7ee43b22aa2ed60fb982ae94b285c1be671

I'm replacing this:

> With the upstream MR !3916 of commit a381bd3615de6 ('powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks')

with:

> With the upstream MR !3916 of commit ad86d7ee43b22aa2 ("powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks")

Ok?

- Arnaldo

> 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.
> 
> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
> event causes failure after the MR as the zero return value was required.
> 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 do not support
> some of the hv_gpci events.
> 
> 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 a381bd3615de6:
> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
> 
> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> ---
>  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
> +    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
> +    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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Arnaldo Carvalho de Melo]

On Mon, Apr 15, 2024 at 11:42:20AM +0200, vmolnaro@redhat.com wrote:
> From: Veronika Molnarova <vmolnaro@redhat.com>
> 
> With the upstream MR !3916 of commit a381bd3615de6 ('powerpc/hv-gpci:
> Fix the H_GET_PERF_COUNTER_INFO hcall return value checks') the perf

Athira/Kajol,

	I don't have access to such machines, since you did work on this
area, could you please take a look and maybe test it?

	The upstream patch mentioned is in fact:

  ad86d7ee43b22aa2 ("powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks")

Thanks,

- Arnaldo

> 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.
> 
> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
> event causes failure after the MR as the zero return value was required.
> 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 do not support
> some of the hv_gpci events.
> 
> 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 a381bd3615de6:
> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
> 
> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> ---
>  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
> +    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
> +    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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[kajoljain]

On 4/16/24 20:03, Arnaldo Carvalho de Melo wrote:
> On Mon, Apr 15, 2024 at 11:42:20AM +0200, vmolnaro@redhat.com wrote:
>> From: Veronika Molnarova <vmolnaro@redhat.com>
>>
>> With the upstream MR !3916 of commit a381bd3615de6 ('powerpc/hv-gpci:
>> Fix the H_GET_PERF_COUNTER_INFO hcall return value checks') the perf
> 
> Athira/Kajol,
> 
> 	I don't have access to such machines, since you did work on this
> area, could you please take a look and maybe test it?
> 
> 	The upstream patch mentioned is in fact:
> 
>   ad86d7ee43b22aa2 ("powerpc/hv-gpci: Fix the H_GET_PERF_COUNTER_INFO hcall return value checks")
> 
> Thanks,
> 
> - Arnaldo

Hi Arnaldo,
 Sure we will review and test the patch.

Thanks,
Kajol Jain
> 
>> 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.
>>
>> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
>> event causes failure after the MR as the zero return value was required.
>> 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 do not support
>> some of the hv_gpci events.
>>
>> 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 a381bd3615de6:
>> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
>>
>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
>> ---
>>  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
>> +    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
>> +    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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[kajoljain]

Hi Veronika,
   I was trying to test this patch. But I am not able to apply this
patch cleanly on both upstream and acme's tmp.perf-tools-next branch.

error: patch failed: tools/perf/tests/shell/stat_all_pmu.sh:2
error: tools/perf/tests/shell/stat_all_pmu.sh: patch does not apply

Can you check that part, but since its simple change I added it manually
and could test the patch.

Patch looks fine to me. I tested it by disabling hv-gpci events in HMC

Without patch changes:

localhost:/dev/shm/linux/tools/perf # ./perf test 95 -vv
 95: perf all PMU test:
--- start ---
test child forked, pid 2234
---
Testing cpu/stalled-cycles-frontend/
Testing
hv_gpci/system_hypervisor_times_time_spent_managing_partitions_over_entitlement/
---- end(-1) ----
 95: perf all PMU test                                               :
FAILED!

with patch changes:

Testing event
'hv_gpci/system_hypervisor_times_time_spent_managing_partitions_over_entitlement/'
-- no permission to enable
Testing event
'hv_gpci/system_hypervisor_times_time_spent_on_system_management/' -- no
permission to enable
Testing event
'hv_gpci/system_hypervisor_times_time_spent_processing_virtual_processor_timers/'
-- no permission to enable
Testing event
'hv_gpci/system_hypervisor_times_time_spent_to_dispatch_virtual_processors/'
-- no permission to enable
Testing event
'hv_gpci/system_tlbie_count_and_time_time_spent_issuing_tlbies/' -- no
permission to enable
Testing event
'hv_gpci/system_tlbie_count_and_time_tlbie_instructions_issued/' -- no
permission to enable
Testing event 'pm_br_mpred_cmpl' -- supported

On 4/15/24 15:12, vmolnaro@redhat.com wrote:
> From: Veronika Molnarova <vmolnaro@redhat.com>
> 
> With the upstream MR !3916 of commit 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 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.
> 
> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
> event causes failure after the MR as the zero return value was required.
> 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 do not support
> some of the hv_gpci events.
> 
> 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 a381bd3615de6:
> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
> 
> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> ---
>  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

It will create extra blank line can you remove that. Also make sure to
test the patch with checkpatch script to avoid any format issue.

Thanks,
Kajol Jain

>  
>  # 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
> +    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
> +    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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Veronika Molnarova]

Hi Jain,

Thanks for letting me know, there was just some typo fixed in the comment upstream,
which caused it to fail. Will fix the patch, also add Ian's suggestion and 
resend it shortly.

Thanks,
Veronika

On 4/23/24 10:01, kajoljain wrote:
> Hi Veronika,
>    I was trying to test this patch. But I am not able to apply this
> patch cleanly on both upstream and acme's tmp.perf-tools-next branch.
> 
> error: patch failed: tools/perf/tests/shell/stat_all_pmu.sh:2
> error: tools/perf/tests/shell/stat_all_pmu.sh: patch does not apply
> 
> Can you check that part, but since its simple change I added it manually
> and could test the patch.
> 
> Patch looks fine to me. I tested it by disabling hv-gpci events in HMC
> 
> Without patch changes:
> 
> localhost:/dev/shm/linux/tools/perf # ./perf test 95 -vv
>  95: perf all PMU test:
> --- start ---
> test child forked, pid 2234
> ---
> Testing cpu/stalled-cycles-frontend/
> Testing
> hv_gpci/system_hypervisor_times_time_spent_managing_partitions_over_entitlement/
> ---- end(-1) ----
>  95: perf all PMU test                                               :
> FAILED!
> 
> with patch changes:
> 
> Testing event
> 'hv_gpci/system_hypervisor_times_time_spent_managing_partitions_over_entitlement/'
> -- no permission to enable
> Testing event
> 'hv_gpci/system_hypervisor_times_time_spent_on_system_management/' -- no
> permission to enable
> Testing event
> 'hv_gpci/system_hypervisor_times_time_spent_processing_virtual_processor_timers/'
> -- no permission to enable
> Testing event
> 'hv_gpci/system_hypervisor_times_time_spent_to_dispatch_virtual_processors/'
> -- no permission to enable
> Testing event
> 'hv_gpci/system_tlbie_count_and_time_time_spent_issuing_tlbies/' -- no
> permission to enable
> Testing event
> 'hv_gpci/system_tlbie_count_and_time_tlbie_instructions_issued/' -- no
> permission to enable
> Testing event 'pm_br_mpred_cmpl' -- supported
> 
> On 4/15/24 15:12, vmolnaro@redhat.com wrote:
>> From: Veronika Molnarova <vmolnaro@redhat.com>
>>
>> With the upstream MR !3916 of commit 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 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.
>>
>> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
>> event causes failure after the MR as the zero return value was required.
>> 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 do not support
>> some of the hv_gpci events.
>>
>> 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 a381bd3615de6:
>> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/3916
>>
>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
>> ---
>>  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
> 
> It will create extra blank line can you remove that. Also make sure to
> test the patch with checkpatch script to avoid any format issue.
> 
> Thanks,
> Kajol Jain
> 
>>  
>>  # 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
>> +    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
>> +    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] perf test stat_all_pmu.sh: Parse return value of perf stat

[Ian Rogers]

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/

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

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

[Arnaldo Carvalho de Melo]

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

[PATCH v2] 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.

Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
event causes failure after the MR as the zero return value was required.
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 do not support
some of the hv_gpci events.

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

Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
---
Fixed the issue with applying due to a fixed comment typo upstream and
added Ian's suggestion for the 'device busy' issue during parallel
testing.

 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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Arnaldo Carvalho de Melo]

On Mon, Apr 29, 2024 at 12:56:24PM +0200, 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.

Is this v2 or v3? Kajol, can I have your tested-by?

Thanks,

- Arnaldo
 
> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
> event causes failure after the MR as the zero return value was required.
> 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 do not support
> some of the hv_gpci events.
> 
> 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
> 
> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> ---
> Fixed the issue with applying due to a fixed comment typo upstream and
> added Ian's suggestion for the 'device busy' issue during parallel
> testing.
> 
>  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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Veronika Molnarova]

On 5/3/24 22:25, Arnaldo Carvalho de Melo wrote:
> On Mon, Apr 29, 2024 at 12:56:24PM +0200, 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.
> 
> Is this v2 or v3? Kajol, can I have your tested-by?
> 
> Thanks,
> 
> - Arnaldo
>  
From my point of view it was v2 of the patch, but now I noticed that the a v2
was created when the commit hash was firstly fixed, so maybe its then a v3?

Sorry for confusion,
Veronika

>> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
>> event causes failure after the MR as the zero return value was required.
>> 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 do not support
>> some of the hv_gpci events.
>>
>> 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
>>
>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
>> ---
>> Fixed the issue with applying due to a fixed comment typo upstream and
>> added Ian's suggestion for the 'device busy' issue during parallel
>> testing.
>>
>>  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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Arnaldo Carvalho de Melo]

On Fri, May 03, 2024 at 05:25:37PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Apr 29, 2024 at 12:56:24PM +0200, 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.
> 
> Is this v2 or v3? Kajol, can I have your tested-by?

Oops, Kajol wasn't on the CC list, nor Ian, I'm adding them, maybe Kajol
can test it on those machines and Ian, who wrote the test can take a
look, also adding all the perf tools reviewers, as listed in
MAINTAINERS.

Thanks,

- Arnaldo
 
> > Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
> > event causes failure after the MR as the zero return value was required.
> > 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 do not support
> > some of the hv_gpci events.
> > 
> > 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
> > 
> > Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> > ---
> > Fixed the issue with applying due to a fixed comment typo upstream and
> > added Ian's suggestion for the 'device busy' issue during parallel
> > testing.
> > 
> >  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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[kajoljain]

On 8/12/24 18:43, Arnaldo Carvalho de Melo wrote:
> On Fri, May 03, 2024 at 05:25:37PM -0300, Arnaldo Carvalho de Melo wrote:
>> On Mon, Apr 29, 2024 at 12:56:24PM +0200, 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.
>>
>> Is this v2 or v3? Kajol, can I have your tested-by?
> 
> Oops, Kajol wasn't on the CC list, nor Ian, I'm adding them, maybe Kajol
> can test it on those machines and Ian, who wrote the test can take a
> look, also adding all the perf tools reviewers, as listed in
> MAINTAINERS.
> 
> Thanks,
> 
> - Arnaldo
>  
Hi Veronika,
I checked the patch, code wise patch looks fine to me. I also tested it
on powerpc system.

Just have concern on your commit message, you added below text as part
of your commit message:

It was caught by CKI testing where it was triaged to stop
blocking further MRs, as most of the powerpc machines do not support
some of the hv_gpci events.

The hv-gpci events are supported in pseries system.
And for different power processors, we already have code to include only
supported event as part of perf list.

If you are taking about parametrized events which are skipped in this
testcase, its not that those parametrized events are not supported.
Since testing proper values of the parameters is out of scope of this
script we are skipping those events.

Can you remove that part from commit message? Rest looks fine to me.

Reviewed-by: Kajol Jain <kjain@linux.ibm.com>
Tested-by: Kajol Jain <kjain@linux.ibm.com>

Thanks,
Kajol Jain

>>> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
>>> event causes failure after the MR as the zero return value was required.
>>> 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 do not support
>>> some of the hv_gpci events.
>>>
>>> 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
>>>
>>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
>>> ---
>>> Fixed the issue with applying due to a fixed comment typo upstream and
>>> added Ian's suggestion for the 'device busy' issue during parallel
>>> testing.
>>>
>>>  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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Arnaldo Carvalho de Melo]

On Tue, Aug 13, 2024 at 01:21:14PM +0530, kajoljain wrote:
> 
> 
> On 8/12/24 18:43, Arnaldo Carvalho de Melo wrote:
> > On Fri, May 03, 2024 at 05:25:37PM -0300, Arnaldo Carvalho de Melo wrote:
> >> On Mon, Apr 29, 2024 at 12:56:24PM +0200, 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.
> >>
> >> Is this v2 or v3? Kajol, can I have your tested-by?
> > 
> > Oops, Kajol wasn't on the CC list, nor Ian, I'm adding them, maybe Kajol
> > can test it on those machines and Ian, who wrote the test can take a
> > look, also adding all the perf tools reviewers, as listed in
> > MAINTAINERS.
> > 
> > Thanks,
> > 
> > - Arnaldo
> >  
> Hi Veronika,
> I checked the patch, code wise patch looks fine to me. I also tested it
> on powerpc system.
> 
> Just have concern on your commit message, you added below text as part
> of your commit message:
> 
> It was caught by CKI testing where it was triaged to stop
> blocking further MRs, as most of the powerpc machines do not support
> some of the hv_gpci events.
> 
> The hv-gpci events are supported in pseries system.
> And for different power processors, we already have code to include only
> supported event as part of perf list.
> 
> If you are taking about parametrized events which are skipped in this
> testcase, its not that those parametrized events are not supported.
> Since testing proper values of the parameters is out of scope of this
> script we are skipping those events.
> 
> Can you remove that part from commit message? Rest looks fine to me.

Veronika, can you please address the reviewer's comment and send a v4
patch, collecting the tags he provided?

Thanks,

- Arnaldo
 
> Reviewed-by: Kajol Jain <kjain@linux.ibm.com>
> Tested-by: Kajol Jain <kjain@linux.ibm.com>
> 
> Thanks,
> Kajol Jain
> 
> >>> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
> >>> event causes failure after the MR as the zero return value was required.
> >>> 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 do not support
> >>> some of the hv_gpci events.
> >>>
> >>> 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
> >>>
> >>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> >>> ---
> >>> Fixed the issue with applying due to a fixed comment typo upstream and
> >>> added Ian's suggestion for the 'device busy' issue during parallel
> >>> testing.
> >>>
> >>>  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 v2] perf test stat_all_pmu.sh: Parse return value of perf stat

[Veronika Molnarova]

On 8/13/24 17:15, Arnaldo Carvalho de Melo wrote:
> On Tue, Aug 13, 2024 at 01:21:14PM +0530, kajoljain wrote:
>>
>>
>> On 8/12/24 18:43, Arnaldo Carvalho de Melo wrote:
>>> On Fri, May 03, 2024 at 05:25:37PM -0300, Arnaldo Carvalho de Melo wrote:
>>>> On Mon, Apr 29, 2024 at 12:56:24PM +0200, 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.
>>>>
>>>> Is this v2 or v3? Kajol, can I have your tested-by?
>>>
>>> Oops, Kajol wasn't on the CC list, nor Ian, I'm adding them, maybe Kajol
>>> can test it on those machines and Ian, who wrote the test can take a
>>> look, also adding all the perf tools reviewers, as listed in
>>> MAINTAINERS.
>>>
>>> Thanks,
>>>
>>> - Arnaldo
>>>  
>> Hi Veronika,
>> I checked the patch, code wise patch looks fine to me. I also tested it
>> on powerpc system.
>>
>> Just have concern on your commit message, you added below text as part
>> of your commit message:
>>
>> It was caught by CKI testing where it was triaged to stop
>> blocking further MRs, as most of the powerpc machines do not support
>> some of the hv_gpci events.
>>
>> The hv-gpci events are supported in pseries system.
>> And for different power processors, we already have code to include only
>> supported event as part of perf list.
>>
>> If you are taking about parametrized events which are skipped in this
>> testcase, its not that those parametrized events are not supported.
>> Since testing proper values of the parameters is out of scope of this
>> script we are skipping those events.
>>
>> Can you remove that part from commit message? Rest looks fine to me.
> 
> Veronika, can you please address the reviewer's comment and send a v4
> patch, collecting the tags he provided?
> 
> Thanks,
> 
> - Arnaldo
>  
>> Reviewed-by: Kajol Jain <kjain@linux.ibm.com>
>> Tested-by: Kajol Jain <kjain@linux.ibm.com>
>>
>> Thanks,
>> Kajol Jain
>>
>>>>> Running stat_all_pmu test on powerpc machine with unsupported hv_gpci
>>>>> event causes failure after the MR as the zero return value was required.
>>>>> 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 do not support
>>>>> some of the hv_gpci events.
>>>>>
>>>>> 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
>>>>>
>>>>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
>>>>> ---
>>>>> Fixed the issue with applying due to a fixed comment typo upstream and
>>>>> added Ian's suggestion for the 'device busy' issue during parallel
>>>>> testing.
>>>>>
>>>>>  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
>>>>>
>>>
> 

Looking at it, the issue wasn't with the unsupported hv-gpci events, but
the ones without permissions to retrieve information from their partitions
as mentioned in the first part of the commit message.

Will rework the commit message and re-send the patch.

Thanks,
-Veronika

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

[Veronika Molnarova]

Hi Ian,

Thanks for looking at this, will check the problem with busy PMUs and add your proposition.

Best,
Veronika

On 4/16/24 17:03, 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/
> 
>> ---
>>  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
>>
>>
>