[PATCH 0/2 v3] perf test: perf record tests (114) changes

[PATCH 0/2 v3] perf test: perf record tests (114) changes

[Thomas Richter]

Change event intructions to cycles for subtests
 - precise_max attribute
 - Basic leader sampling
as event instructions can not be used for sampling on s390.

Thomas Richter (2):
  perf test: Fix perf test 114 perf record test subtest precise_max
  perf test: Change event in perf test 114 perf record test subtest
    test_leader_sampling

 tools/perf/tests/shell/record.sh | 53 ++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 19 deletions(-)

-- 
2.48.1

[PATCH 1/2 v3] perf test: Fix perf test 114 perf record test subtest precise_max

[Thomas Richter]

On s390 the event instructions can not be used for recording.
This event is only supported by perf stat.

Test that each event cycles and instructions supports sampling.
If the event can not be sampled, skip it.

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Suggested-by: James Clark <james.clark@linaro.org>
Reviewed-by: James Clark <james.clark@linaro.org>
---
 tools/perf/tests/shell/record.sh | 43 +++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 0fc7a909ae9b..fe2d05bcbb1f 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -273,27 +273,42 @@ test_topdown_leader_sampling() {
 }
 
 test_precise_max() {
+  local -i skipped=0
+
   echo "precise_max attribute test"
-  if ! perf stat -e "cycles,instructions" true 2> /dev/null
+  # Just to make sure event cycles is supported for sampling
+  if perf record -o "${perfdata}" -e "cycles" true 2> /dev/null
   then
-    echo "precise_max attribute [Skipped no hardware events]"
-    return
+    if ! perf record -o "${perfdata}" -e "cycles:P" true 2> /dev/null
+    then
+      echo "precise_max attribute [Failed cycles:P event]"
+      err=1
+      return
+    fi
+  else
+    echo "precise_max attribute [Skipped no cycles:P event]"
+    ((skipped+=1))
   fi
-  # Just to make sure it doesn't fail
-  if ! perf record -o "${perfdata}" -e "cycles:P" true 2> /dev/null
+  # On s390 event instructions is not supported for perf record
+  if perf record -o "${perfdata}" -e "instructions" true 2> /dev/null
   then
-    echo "precise_max attribute [Failed cycles:P event]"
-    err=1
-    return
+    # On AMD, cycles and instructions events are treated differently
+    if ! perf record -o "${perfdata}" -e "instructions:P" true 2> /dev/null
+    then
+      echo "precise_max attribute [Failed instructions:P event]"
+      err=1
+      return
+    fi
+  else
+    echo "precise_max attribute [Skipped no instructions:P event]"
+    ((skipped+=1))
   fi
-  # On AMD, cycles and instructions events are treated differently
-  if ! perf record -o "${perfdata}" -e "instructions:P" true 2> /dev/null
+  if [ $skipped -eq 2 ]
   then
-    echo "precise_max attribute [Failed instructions:P event]"
-    err=1
-    return
+    echo "precise_max attribute [Skipped no hardware events]"
+  else
+    echo "precise_max attribute test [Success]"
   fi
-  echo "precise_max attribute test [Success]"
 }
 
 # raise the limit of file descriptors to minimum
-- 
2.48.1

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

[Thomas Richter]

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@linux.ibm.com>
Suggested-by: James Clark <james.clark@linaro.org>
Reviewed-by: James Clark <james.clark@linaro.org>
---
 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
   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

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

[Namhyung Kim]

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@linux.ibm.com>
> Suggested-by: James Clark <james.clark@linaro.org>
> Reviewed-by: James Clark <james.clark@linaro.org>
> ---
>  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
>    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
> 

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-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@linux.ibm.com>
>> Suggested-by: James Clark <james.clark@linaro.org>
>> Reviewed-by: James Clark <james.clark@linaro.org>
>> ---
>>  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@linux.intel.com>

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

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

[Namhyung Kim]

Hello Kan,

On Tue, Feb 04, 2025 at 10:55:44AM -0500, 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@linux.ibm.com>
> >> Suggested-by: James Clark <james.clark@linaro.org>
> >> Reviewed-by: James Clark <james.clark@linaro.org>
> >> ---
> >>  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.

Sorry, I don't think I'm following.  Are you saying "{cycles:p,cycles:p}:S"
cannot guarantee that they will have the same period?

> 
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks for your review and the comment!
Namhyung

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-04 2:33 p.m., Namhyung Kim wrote:
> Hello Kan,
> 
> On Tue, Feb 04, 2025 at 10:55:44AM -0500, 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@linux.ibm.com>
>>>> Suggested-by: James Clark <james.clark@linaro.org>
>>>> Reviewed-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>  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.
> 
> Sorry, I don't think I'm following.  Are you saying "{cycles:p,cycles:p}:S"
> cannot guarantee that they will have the same period?

Only sampling can supports p modifier. So it should be
{cycles:p,cycles}:S. The "{cycles:p,cycles:p}:S" will error out.
Yes, it's not guaranteed that they have the same period.

Thanks,
Kan
> 
>>
>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> 
> Thanks for your review and the comment!
> Namhyung
> 
> 

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

[Mi, Dapeng]

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@linux.ibm.com>
>>> Suggested-by: James Clark <james.clark@linaro.org>
>>> Reviewed-by: James Clark <james.clark@linaro.org>
>>> ---
>>>  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@linux.intel.com>

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?


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


>

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@linux.ibm.com>
>>>> Suggested-by: James Clark <james.clark@linaro.org>
>>>> Reviewed-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>  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@linux.intel.com>
> 
> 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.
> 
> 
>>

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

[Mi, Dapeng]

On 2/6/2025 10:25 PM, Liang, Kan wrote:
>
> 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@linux.ibm.com>
>>>>> Suggested-by: James Clark <james.clark@linaro.org>
>>>>> Reviewed-by: James Clark <james.clark@linaro.org>
>>>>> ---
>>>>>  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@linux.intel.com>
>> 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.

Got it. Thanks for explaining.


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

Re: [PATCH 0/2 v3] perf test: perf record tests (114) changes

[James Clark]

On 31/01/2025 10:27 am, Thomas Richter wrote:
> Change event intructions to cycles for subtests
>   - precise_max attribute
>   - Basic leader sampling
> as event instructions can not be used for sampling on s390.
> 
> Thomas Richter (2):
>    perf test: Fix perf test 114 perf record test subtest precise_max
>    perf test: Change event in perf test 114 perf record test subtest
>      test_leader_sampling
> 
>   tools/perf/tests/shell/record.sh | 53 ++++++++++++++++++++------------
>   1 file changed, 34 insertions(+), 19 deletions(-)
> 

LGTM

Re: [PATCH 0/2 v3] perf test: perf record tests (114) changes

[Namhyung Kim]

On Fri, 31 Jan 2025 11:27:54 +0100, Thomas Richter wrote:
> Change event intructions to cycles for subtests
>  - precise_max attribute
>  - Basic leader sampling
> as event instructions can not be used for sampling on s390.
> 
> Thomas Richter (2):
>   perf test: Fix perf test 114 perf record test subtest precise_max
>   perf test: Change event in perf test 114 perf record test subtest
>     test_leader_sampling
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung