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

[kajoljain <kjain@linux.ibm.com>]

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
>