* [PATCH 1/2] perf test: Reorder event name checks in stat STD output linter
@ 2023-06-23 23:01 Namhyung Kim
2023-06-23 23:01 ` [PATCH 2/2] perf test: Skip metrics w/o event name " Namhyung Kim
2023-06-23 23:22 ` [PATCH 1/2] perf test: Reorder event name checks " Ian Rogers
0 siblings, 2 replies; 5+ messages in thread
From: Namhyung Kim @ 2023-06-23 23:01 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers
Cc: Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users, Kan Liang
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] perf test: Skip metrics w/o event name in stat STD output linter
2023-06-23 23:01 [PATCH 1/2] perf test: Reorder event name checks in stat STD output linter Namhyung Kim
@ 2023-06-23 23:01 ` Namhyung Kim
2023-06-23 23:23 ` Ian Rogers
2023-06-23 23:22 ` [PATCH 1/2] perf test: Reorder event name checks " Ian Rogers
1 sibling, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2023-06-23 23:01 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers
Cc: Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users, Kan Liang
This test checks if the output of perf stat to match event names and
metrics. So it wants the output lines to have both event name and
metric. Otherwise it should skip the line.
On AMD machines, the instruction event has two metrics and they are printed
in separate lines. It makes the line without event name like below:
# perf stat -a sleep 1
Performance counter stats for 'system wide':
64,383.34 msec cpu-clock # 64.048 CPUs utilized
14,526 context-switches # 225.617 /sec
112 cpu-migrations # 1.740 /sec
190 page-faults # 2.951 /sec
807,558,652 cycles # 0.013 GHz (83.30%)
69,809,799 stalled-cycles-frontend # 8.64% frontend cycles idle (83.30%)
196,983,266 stalled-cycles-backend # 24.39% backend cycles idle (83.30%)
424,876,008 instructions # 0.53 insn per cycle
(here) ---> # 0.46 stalled cycles per insn (83.30%)
97,788,321 branches # 1.519 M/sec (83.34%)
4,147,377 branch-misses # 4.24% of all branches (83.46%)
1.005241409 seconds time elapsed
Also modern Intel machines have TopDown metrics which also don't have
event names.
# perf stat -a sleep 1
Performance counter stats for 'system wide':
8,015.39 msec cpu-clock # 7.996 CPUs utilized
5,823 context-switches # 726.477 /sec
189 cpu-migrations # 23.580 /sec
139 page-faults # 17.342 /sec
435,139,308 cycles # 0.054 GHz
193,891,345 instructions # 0.45 insn per cycle
42,773,028 branches # 5.336 M/sec
2,298,113 branch-misses # 5.37% of all branches
TopdownL1 # 25.5 % tma_backend_bound
/--> # 7.9 % tma_bad_speculation
(here) --+ # 55.7 % tma_frontend_bound
\--> # 10.9 % tma_retiring
1.002395924 seconds time elapsed
There is a check to skip TopdownL1 and TopdownL2 specifically but it
does not cover every affected lines.
So there is another check to skip the line if it has nothing on the left
side of # sign. Well.. it seems ok but that's not enough too.
When aggregation mode (like --per-socket or --per-thread) is used, it
adds some prefix (e.g. CPU socket, task name and PID) in the output
line. So the test code ignores them to normalize result.
A problem can happen for per-thread mode when task name contains one or
more spaces. It'd only ignore the first part of the task name, and it
thinks there's something more in the line so it would not skip.
# perf stat -a --perf-thread sleep 1
...
perf-21276 # 70.2 % tma_backend_bound
perf-21276 # 3.9 % tma_bad_speculation
perf-21276 # 10.5 % tma_frontend_bound
perf-21276 # 15.3 % tma_retiring
^^^^^^^^^^
(ignored)
my task-21328 # 70.2 % tma_backend_bound
my task-21328 # 3.9 % tma_bad_speculation
my task-21328 # 10.5 % tma_frontend_bound
my task-21328 # 15.3 % tma_retiring
^^
(ignored)
So I think it should look at the metric names instead. Add skip_metric
to hold the list of names to skip. It would contain 'stalled cycles per
insn' and metrics started by 'tma_'.
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/stat+std_output.sh | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/perf/tests/shell/stat+std_output.sh b/tools/perf/tests/shell/stat+std_output.sh
index 1f70aab45184..f972b31fa0c2 100755
--- a/tools/perf/tests/shell/stat+std_output.sh
+++ b/tools/perf/tests/shell/stat+std_output.sh
@@ -12,8 +12,7 @@ stat_output=$(mktemp /tmp/__perf_test.stat_output.std.XXXXX)
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)
+skip_metric=("stalled cycles per insn" "tma_")
cleanup() {
rm -f "${stat_output}"
@@ -58,13 +57,14 @@ function commachecker()
main_body=$(echo $line | cut -d' ' -f$prefix-)
x=${main_body%#*}
- # Check default metricgroup
- y=$(echo $x | tr -d ' ')
- [ "$y" = "" ] && continue
- for i in "${!metricgroup_name[@]}"; do
- [[ "$y" == *"${metricgroup_name[$i]}"* ]] && break
+ [ "$x" = "" ] && continue
+
+ # Skip metrics without event name
+ y=${main_body#*#}
+ for i in "${!skip_metric[@]}"; do
+ [[ "$y" == *"${skip_metric[$i]}"* ]] && break
done
- [[ "$y" == *"${metricgroup_name[$i]}"* ]] && continue
+ [[ "$y" == *"${skip_metric[$i]}"* ]] && continue
# Check default event
for i in "${!event_name[@]}"; do
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] perf test: Reorder event name checks in stat STD output linter
2023-06-23 23:01 [PATCH 1/2] perf test: Reorder event name checks in stat STD output linter Namhyung Kim
2023-06-23 23:01 ` [PATCH 2/2] perf test: Skip metrics w/o event name " Namhyung Kim
@ 2023-06-23 23:22 ` Ian Rogers
2023-06-24 23:39 ` Namhyung Kim
1 sibling, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2023-06-23 23:22 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Kan Liang
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] perf test: Skip metrics w/o event name in stat STD output linter
2023-06-23 23:01 ` [PATCH 2/2] perf test: Skip metrics w/o event name " Namhyung Kim
@ 2023-06-23 23:23 ` Ian Rogers
0 siblings, 0 replies; 5+ messages in thread
From: Ian Rogers @ 2023-06-23 23:23 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Kan Liang
On Fri, Jun 23, 2023 at 4:01 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> This test checks if the output of perf stat to match event names and
> metrics. So it wants the output lines to have both event name and
> metric. Otherwise it should skip the line.
>
> On AMD machines, the instruction event has two metrics and they are printed
> in separate lines. It makes the line without event name like below:
>
> # perf stat -a sleep 1
>
> Performance counter stats for 'system wide':
>
> 64,383.34 msec cpu-clock # 64.048 CPUs utilized
> 14,526 context-switches # 225.617 /sec
> 112 cpu-migrations # 1.740 /sec
> 190 page-faults # 2.951 /sec
> 807,558,652 cycles # 0.013 GHz (83.30%)
> 69,809,799 stalled-cycles-frontend # 8.64% frontend cycles idle (83.30%)
> 196,983,266 stalled-cycles-backend # 24.39% backend cycles idle (83.30%)
> 424,876,008 instructions # 0.53 insn per cycle
> (here) ---> # 0.46 stalled cycles per insn (83.30%)
> 97,788,321 branches # 1.519 M/sec (83.34%)
> 4,147,377 branch-misses # 4.24% of all branches (83.46%)
>
> 1.005241409 seconds time elapsed
>
> Also modern Intel machines have TopDown metrics which also don't have
> event names.
>
> # perf stat -a sleep 1
>
> Performance counter stats for 'system wide':
>
> 8,015.39 msec cpu-clock # 7.996 CPUs utilized
> 5,823 context-switches # 726.477 /sec
> 189 cpu-migrations # 23.580 /sec
> 139 page-faults # 17.342 /sec
> 435,139,308 cycles # 0.054 GHz
> 193,891,345 instructions # 0.45 insn per cycle
> 42,773,028 branches # 5.336 M/sec
> 2,298,113 branch-misses # 5.37% of all branches
> TopdownL1 # 25.5 % tma_backend_bound
> /--> # 7.9 % tma_bad_speculation
> (here) --+ # 55.7 % tma_frontend_bound
> \--> # 10.9 % tma_retiring
>
> 1.002395924 seconds time elapsed
>
> There is a check to skip TopdownL1 and TopdownL2 specifically but it
> does not cover every affected lines.
>
> So there is another check to skip the line if it has nothing on the left
> side of # sign. Well.. it seems ok but that's not enough too.
>
> When aggregation mode (like --per-socket or --per-thread) is used, it
> adds some prefix (e.g. CPU socket, task name and PID) in the output
> line. So the test code ignores them to normalize result.
>
> A problem can happen for per-thread mode when task name contains one or
> more spaces. It'd only ignore the first part of the task name, and it
> thinks there's something more in the line so it would not skip.
>
> # perf stat -a --perf-thread sleep 1
> ...
> perf-21276 # 70.2 % tma_backend_bound
> perf-21276 # 3.9 % tma_bad_speculation
> perf-21276 # 10.5 % tma_frontend_bound
> perf-21276 # 15.3 % tma_retiring
> ^^^^^^^^^^
> (ignored)
>
> my task-21328 # 70.2 % tma_backend_bound
> my task-21328 # 3.9 % tma_bad_speculation
> my task-21328 # 10.5 % tma_frontend_bound
> my task-21328 # 15.3 % tma_retiring
> ^^
> (ignored)
>
> So I think it should look at the metric names instead. Add skip_metric
> to hold the list of names to skip. It would contain 'stalled cycles per
> insn' and metrics started by 'tma_'.
>
> 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/stat+std_output.sh | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/tests/shell/stat+std_output.sh b/tools/perf/tests/shell/stat+std_output.sh
> index 1f70aab45184..f972b31fa0c2 100755
> --- a/tools/perf/tests/shell/stat+std_output.sh
> +++ b/tools/perf/tests/shell/stat+std_output.sh
> @@ -12,8 +12,7 @@ stat_output=$(mktemp /tmp/__perf_test.stat_output.std.XXXXX)
>
> 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)
> +skip_metric=("stalled cycles per insn" "tma_")
>
> cleanup() {
> rm -f "${stat_output}"
> @@ -58,13 +57,14 @@ function commachecker()
>
> main_body=$(echo $line | cut -d' ' -f$prefix-)
> x=${main_body%#*}
> - # Check default metricgroup
> - y=$(echo $x | tr -d ' ')
> - [ "$y" = "" ] && continue
> - for i in "${!metricgroup_name[@]}"; do
> - [[ "$y" == *"${metricgroup_name[$i]}"* ]] && break
> + [ "$x" = "" ] && continue
> +
> + # Skip metrics without event name
> + y=${main_body#*#}
> + for i in "${!skip_metric[@]}"; do
> + [[ "$y" == *"${skip_metric[$i]}"* ]] && break
> done
> - [[ "$y" == *"${metricgroup_name[$i]}"* ]] && continue
> + [[ "$y" == *"${skip_metric[$i]}"* ]] && continue
>
> # Check default event
> for i in "${!event_name[@]}"; do
> --
> 2.41.0.162.gfafddb0af9-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] perf test: Reorder event name checks in stat STD output linter
2023-06-23 23:22 ` [PATCH 1/2] perf test: Reorder event name checks " Ian Rogers
@ 2023-06-24 23:39 ` Namhyung Kim
0 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2023-06-24 23:39 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Kan Liang
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-24 23:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-23 23:01 [PATCH 1/2] perf test: Reorder event name checks in stat STD output linter Namhyung Kim
2023-06-23 23:01 ` [PATCH 2/2] perf test: Skip metrics w/o event name " Namhyung Kim
2023-06-23 23:23 ` Ian Rogers
2023-06-23 23:22 ` [PATCH 1/2] perf test: Reorder event name checks " Ian Rogers
2023-06-24 23:39 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).