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

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

Thanks for your review and the comment!
Namhyung