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

[Veronika Molnarova <vmolnaro@redhat.com>]

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