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

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

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