Re: [PATCH 2/2 v3] perf test: Change event in perf test 114 perf record test subtest test_leader_sampling

[Liang, Kan]

On 2025-02-06 12:42 a.m., Mi, Dapeng wrote:
> 
> On 2/4/2025 11:55 PM, Liang, Kan wrote:
>>
>> On 2025-02-03 10:42 p.m., Namhyung Kim wrote:
>>> Add Kan and Dapeng to CC.
>>>
>>> Thanks,
>>> Namhyung
>>>
>>>
>>> On Fri, Jan 31, 2025 at 11:27:56AM +0100, Thomas Richter wrote:
>>>> On s390 the event instructions can not be used for recording.
>>>> This event is only supported by perf stat.
>>>>
>>>> Change the event from instructions to cycles in
>>>> subtest test_leader_sampling.
>>>>
>>>> Signed-off-by: Thomas Richter <tmricht@xxxxxxxxxxxxx>
>>>> Suggested-by: James Clark <james.clark@xxxxxxxxxx>
>>>> Reviewed-by: James Clark <james.clark@xxxxxxxxxx>
>>>> ---
>>>>  tools/perf/tests/shell/record.sh | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
>>>> index fe2d05bcbb1f..ba8d873d3ca7 100755
>>>> --- a/tools/perf/tests/shell/record.sh
>>>> +++ b/tools/perf/tests/shell/record.sh
>>>> @@ -231,7 +231,7 @@ test_cgroup() {
>>>>  
>>>>  test_leader_sampling() {
>>>>    echo "Basic leader sampling test"
>>>> -  if ! perf record -o "${perfdata}" -e "{instructions,instructions}:Su" -- \
>>>> +  if ! perf record -o "${perfdata}" -e "{cycles,cycles}:Su" -- \
>>>>      perf test -w brstack 2> /dev/null
>>
>> As a non-precise test, using cycles instead should be fine. But we
>> should never use it for precise test, e.g., with p. Because cycles is a
>> non-precise event. It would not surprise me if there is a skid when
>> reading two cycles events at the point when the event overflow occurs.
>>
>> Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> 
> Kan, I suppose you mean only the case without counter snapshot, right? With
> counter snapshot's help, there would be same period even for non-precise
> events, right?

No, the counter-snapshot doesn't help. That's why I suggested to not
utilize it via enabling the modifier p. It should work for most of the
cases. But it's not 100% guaranteed for some non-precise events that the
same period is got at overflow. Since it's a test that could be run
everywhere, the occasional false alarm would just bring troubles.

Without p, it falls back to the traditional way of handling the sampling
read. In the PMI handler, the global control is disabled first, then all
the counters are read. The value may not be very accurate, since it's
stopped at the PMI handler, not the counter overflow. But because of the
global control, all the counters stop at the same time. The skid would
be the same. The test should work.

Thanks,
Kan
> 
> 
>>
>> Thanks,
>> Kan
>>
>>>>    then
>>>>      echo "Leader sampling [Failed record]"
>>>> @@ -243,15 +243,15 @@ test_leader_sampling() {
>>>>    while IFS= read -r line
>>>>    do
>>>>      # Check if the two instruction counts are equal in each record
>>>> -    instructions=$(echo $line | awk '{for(i=1;i<=NF;i++) if($i=="instructions:") print $(i-1)}')
>>>> -    if [ $(($index%2)) -ne 0 ] && [ ${instructions}x != ${prev_instructions}x ]
>>>> +    cycles=$(echo $line | awk '{for(i=1;i<=NF;i++) if($i=="cycles:") print $(i-1)}')
>>>> +    if [ $(($index%2)) -ne 0 ] && [ ${cycles}x != ${prev_cycles}x ]
>>>>      then
>>>> -      echo "Leader sampling [Failed inconsistent instructions count]"
>>>> +      echo "Leader sampling [Failed inconsistent cycles count]"
>>>>        err=1
>>>>        return
>>>>      fi
>>>>      index=$(($index+1))
>>>> -    prev_instructions=$instructions
>>>> +    prev_cycles=$cycles
>>>>    done < $script_output
>>>>    echo "Basic leader sampling test [Success]"
>>>>  }
>>>> -- 
>>>> 2.48.1
> 
> The code changes look good for me.
> 
> 
>>