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

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
>

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

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.


>

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

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

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

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