diff mbox series

[1/2] perf test: Reorder event name checks in stat STD output linter

Message ID 20230623230139.985594-1-namhyung@kernel.org
State New
Headers
Series [1/2] perf test: Reorder event name checks in stat STD output linter |

Commit Message

Namhyung Kim June 23, 2023, 11:01 p.m. UTC 
  On AMD machines, the perf stat STD output test failed like below:

  $ sudo ./perf test -v 98
   98: perf stat STD output linter                                     :
  --- start ---
  test child forked, pid 1841901
  Checking STD output: no argswrong event metric.
    expected 'GHz' in 108,121 stalled-cycles-frontend  # 10.88% frontend cycles idle
  test child finished with -1
  ---- end ----
  perf stat STD output linter: FAILED!

This is because there are stalled-cycles-{frontend,backend} events are
used by default.  The current logic checks the event_name array to find
which event it's running.  But 'cycles' event comes before those stalled
cycles event and it matches first.  So it tries to find 'GHz' metric
in the output (which is for the 'cycles') and fails.

Move the stalled-cycles-{frontend,backend} events before 'cycles' so
that it can find the stalled cycles events first.

Also add a space after 'no args' test name for consistency.

Fixes: 99a04a48f225 ("perf test: Add test case for the standard 'perf stat' output")
Cc: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/lib/stat_output.sh | 2 +-
 tools/perf/tests/shell/stat+std_output.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Ian Rogers June 23, 2023, 11:22 p.m. UTC | #1 
On Fri, Jun 23, 2023 at 4:01 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On AMD machines, the perf stat STD output test failed like below:
>
>   $ sudo ./perf test -v 98
>    98: perf stat STD output linter                                     :
>   --- start ---
>   test child forked, pid 1841901
>   Checking STD output: no argswrong event metric.
>     expected 'GHz' in 108,121 stalled-cycles-frontend  # 10.88% frontend cycles idle
>   test child finished with -1
>   ---- end ----
>   perf stat STD output linter: FAILED!
>
> This is because there are stalled-cycles-{frontend,backend} events are
> used by default.  The current logic checks the event_name array to find
> which event it's running.  But 'cycles' event comes before those stalled
> cycles event and it matches first.  So it tries to find 'GHz' metric
> in the output (which is for the 'cycles') and fails.
>
> Move the stalled-cycles-{frontend,backend} events before 'cycles' so
> that it can find the stalled cycles events first.
>
> Also add a space after 'no args' test name for consistency.
>
> Fixes: 99a04a48f225 ("perf test: Add test case for the standard 'perf stat' output")
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/tests/shell/lib/stat_output.sh | 2 +-
>  tools/perf/tests/shell/stat+std_output.sh | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/shell/lib/stat_output.sh b/tools/perf/tests/shell/lib/stat_output.sh
> index 363979b1123d..698343f0ecf9 100644
> --- a/tools/perf/tests/shell/lib/stat_output.sh
> +++ b/tools/perf/tests/shell/lib/stat_output.sh
> @@ -9,7 +9,7 @@ function ParanoidAndNotRoot()
>  # $1 name $2 extra_opt
>  check_no_args()
>  {
> -        echo -n "Checking $1 output: no args"
> +        echo -n "Checking $1 output: no args "
>          perf stat $2 true
>          commachecker --no-args
>          echo "[Success]"
> diff --git a/tools/perf/tests/shell/stat+std_output.sh b/tools/perf/tests/shell/stat+std_output.sh
> index 98cc3356a04a..1f70aab45184 100755
> --- a/tools/perf/tests/shell/stat+std_output.sh
> +++ b/tools/perf/tests/shell/stat+std_output.sh
> @@ -10,8 +10,8 @@ set -e
>
>  stat_output=$(mktemp /tmp/__perf_test.stat_output.std.XXXXX)
>
> -event_name=(cpu-clock task-clock context-switches cpu-migrations page-faults cycles instructions branches branch-misses stalled-cycles-frontend stalled-cycles-backend)
> -event_metric=("CPUs utilized" "CPUs utilized" "/sec" "/sec" "/sec" "GHz" "insn per cycle" "/sec" "of all branches" "frontend cycles idle" "backend cycles idle")
> +event_name=(cpu-clock task-clock context-switches cpu-migrations page-faults stalled-cycles-frontend stalled-cycles-backend cycles instructions branches branch-misses)
> +event_metric=("CPUs utilized" "CPUs utilized" "/sec" "/sec" "/sec" "frontend cycles idle" "backend cycles idle" "GHz" "insn per cycle" "/sec" "of all branches")
>
>  metricgroup_name=(TopdownL1 TopdownL2)
>
> --
> 2.41.0.162.gfafddb0af9-goog
>
Namhyung Kim June 24, 2023, 11:39 p.m. UTC | #2 
On Fri, Jun 23, 2023 at 4:22 PM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Jun 23, 2023 at 4:01 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On AMD machines, the perf stat STD output test failed like below:
> >
> >   $ sudo ./perf test -v 98
> >    98: perf stat STD output linter                                     :
> >   --- start ---
> >   test child forked, pid 1841901
> >   Checking STD output: no argswrong event metric.
> >     expected 'GHz' in 108,121 stalled-cycles-frontend  # 10.88% frontend cycles idle
> >   test child finished with -1
> >   ---- end ----
> >   perf stat STD output linter: FAILED!
> >
> > This is because there are stalled-cycles-{frontend,backend} events are
> > used by default.  The current logic checks the event_name array to find
> > which event it's running.  But 'cycles' event comes before those stalled
> > cycles event and it matches first.  So it tries to find 'GHz' metric
> > in the output (which is for the 'cycles') and fails.
> >
> > Move the stalled-cycles-{frontend,backend} events before 'cycles' so
> > that it can find the stalled cycles events first.
> >
> > Also add a space after 'no args' test name for consistency.
> >
> > Fixes: 99a04a48f225 ("perf test: Add test case for the standard 'perf stat' output")
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Acked-by: Ian Rogers <irogers@google.com>

Applied both to perf-tools-next, thanks!
diff mbox series

Patch

diff --git a/tools/perf/tests/shell/lib/stat_output.sh b/tools/perf/tests/shell/lib/stat_output.sh
index 363979b1123d..698343f0ecf9 100644
--- a/tools/perf/tests/shell/lib/stat_output.sh
+++ b/tools/perf/tests/shell/lib/stat_output.sh
@@ -9,7 +9,7 @@  function ParanoidAndNotRoot()
 # $1 name $2 extra_opt
 check_no_args()
 {
-        echo -n "Checking $1 output: no args"
+        echo -n "Checking $1 output: no args "
         perf stat $2 true
         commachecker --no-args
         echo "[Success]"
diff --git a/tools/perf/tests/shell/stat+std_output.sh b/tools/perf/tests/shell/stat+std_output.sh
index 98cc3356a04a..1f70aab45184 100755
--- a/tools/perf/tests/shell/stat+std_output.sh
+++ b/tools/perf/tests/shell/stat+std_output.sh
@@ -10,8 +10,8 @@  set -e
 
 stat_output=$(mktemp /tmp/__perf_test.stat_output.std.XXXXX)
 
-event_name=(cpu-clock task-clock context-switches cpu-migrations page-faults cycles instructions branches branch-misses stalled-cycles-frontend stalled-cycles-backend)
-event_metric=("CPUs utilized" "CPUs utilized" "/sec" "/sec" "/sec" "GHz" "insn per cycle" "/sec" "of all branches" "frontend cycles idle" "backend cycles idle")
+event_name=(cpu-clock task-clock context-switches cpu-migrations page-faults stalled-cycles-frontend stalled-cycles-backend cycles instructions branches branch-misses)
+event_metric=("CPUs utilized" "CPUs utilized" "/sec" "/sec" "/sec" "frontend cycles idle" "backend cycles idle" "GHz" "insn per cycle" "/sec" "of all branches")
 
 metricgroup_name=(TopdownL1 TopdownL2)