Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value

From: Ian Rogers <irogers@google.com>
Date: 2022-10-04 21:15:19

Possibly related (same subject, not in this thread)

2022-10-01 · Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value · Ian Rogers <irogers@google.com>
2022-09-29 · Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value · James Clark <hidden>
2022-09-29 · Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value · Athira Rajeev <hidden>
2022-09-28 · Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value · James Clark <hidden>
2022-09-28 · Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value · Athira Rajeev <hidden>

On Tue, Oct 4, 2022, 12:06 AM Athira Rajeev [off-list ref]
wrote:

quoted

On 04-Oct-2022, at 12:21 AM, Ian Rogers [off-list ref] wrote:

On Mon, Oct 3, 2022 at 7:03 AM atrajeev [off-list ref]

wrote:

quoted

On 2022-10-02 05:17, Ian Rogers wrote:

quoted

On Thu, Sep 29, 2022 at 5:56 AM James Clark [off-list ref]
wrote:

quoted



On 29/09/2022 09:49, Athira Rajeev wrote:

quoted

On 28-Sep-2022, at 9:05 PM, James Clark [off-list ref]

wrote:

quoted

Hi James,

Thanks for looking at the patch and sharing review comments.

quoted

On 13/09/2022 12:57, Athira Rajeev wrote:

quoted

perf stat includes option to specify aggr_mode to display
per-socket, per-core, per-die, per-node counter details.
Also there is option -A ( AGGR_NONE, -no-aggr ), where the
counter values are displayed for each cpu along with "CPU"
value in one field of the output.

Each of the aggregate mode uses the information fetched
from "/sys/devices/system/cpu/cpuX/topology" like core_id,

I thought that this wouldn't apply to the cpu field because cpu is
basically interchangeable as an index in cpumap, rather than

anything

quoted

being read from the topology file.

The cpu value is filled in this function:

Function : aggr_cpu_id__cpu
Code: util/cpumap.c

quoted

physical_package_id. Utility functions in "cpumap.c" fetches
this information and populates the socket id, core id, cpu etc.
If the platform does not expose the topology information,
these values will be set to -1. Example, in case of powerpc,
details like physical_package_id is restricted to be exposed
in pSeries platform. So id.socket, id.core, id.cpu all will
be set as -1.

In case of displaying socket or die value, there is no check
done in the "aggr_printout" function to see if it points to
valid socket id or die. But for displaying "cpu" value, there
is a check for "if (id.core > -1)". In case of powerpc pSeries
where detail like physical_package_id is restricted to be
exposed, id.core will be set to -1. Hence the column or field
itself for CPU won't be displayed in the output.

Result for per-socket:

<<>>
perf stat -e branches --per-socket -a true

Performance counter stats for 'system wide':

S-1      32            416,851      branches
<<>>

Here S has -1 in above result. But with -A option which also
expects CPU in one column in the result, below is observed.

<<>>
/bin/perf stat -e instructions -A -a true

Performance counter stats for 'system wide':

          47,146      instructions
          45,226      instructions
          43,354      instructions
          45,184      instructions
<<>>

If the cpu id value is pointing to -1 also, it makes sense
to display the column in the output to replicate the behaviour
or to be in precedence with other aggr options(like per-socket,
per-core). Remove the check "id.core" so that CPU field gets
displayed in the output.

Why would you want to print -1 out? Seems like the if statement was

quoted

quoted
quoted
quoted
quoted
quoted
good one to me, otherwise the output looks a bit broken to users.

Are

quoted

the other aggregation modes even working if -1 is set for socket and
die? Maybe we need to not print -1 in those cases or exit earlier

with a

quoted

failure.

The -1 value has a specific internal meaning which is "to not
aggregate". It doesn't mean "not set".

Currently, this check is done only for printing cpu value.
For socket/die/core values, this check is not done. Pasting an
example snippet from a powerpc system ( specifically from pseries

platform where

quoted

the value is set to -1 )

./perf stat --per-core -a -C 1 true

Performance counter stats for 'system wide':

S-1-D-1-C-1          1               1.06 msec cpu-clock

            #    1.018 CPUs utilized

quoted

S-1-D-1-C-1          1                  2      context-switches

           #    1.879 K/sec

quoted

S-1-D-1-C-1          1                  0      cpu-migrations

           #    0.000 /sec

quoted

Here though the value is -1, we are displaying it. Where as in case

of cpu, the first column will be

quoted

empty since we do a check before printing.

Example:

./perf stat --per-core -A -C 1 true

Performance counter stats for 'CPU(s) 1':

             0.88 msec cpu-clock                        #    1.022

CPUs utilized

quoted

                2      context-switches
                0      cpu-migrations


No sure, whether there are scripts out there, which consume the

current format and

quoted

not displaying -1 may break it. That is why we tried with change to

remove check for cpu, similar to

quoted

other modes like socket, die, core etc.

I wouldn't worry about that because there are json and CSV modes which
are machine readable, and -1 is already not always displayed. If
anything this change here is also likely to break parsing by adding -1
where it wasn't before.

quoted

Also perf code ie “aggr_cpu_id__empty” in util/cpumap.c initialises

the

quoted

values to -1 . I was checking to see where we are mapping -1 to “to

not aggregate”.

quoted

What I could find is AGGR_NONE ( which is for no-aggr ) has value as

zero.

quoted

Reference: defined in util/stat.h

enum aggr_mode {
       AGGR_NONE,

That enum is never written to any of the cpumap members, that defines
the mode of how to fill the cpu map instead. 0 is a valid value, for
example "CPU 0". -1 is used as a special case and shouldn't be
displayed
IMO.

Did you see my comment in the code below about the bad merge? Could
that
not be related to your issue?

I'm suspicious of this too. In Claire's patch:

       case AGGR_NONE:
-               if (evsel->percore && !config->percore_show_thread) {
-                       fprintf(config->output, "S%d-D%d-C%*d%s",
-                               id.socket,
-                               id.die,
-                               config->csv_output ? 0 : -3,
-                               id.core, config->csv_sep);
-               } else if (id.cpu.cpu > -1) {
-                       fprintf(config->output, "CPU%*d%s",
-                               config->csv_output ? 0 : -7,
-                               id.cpu.cpu, config->csv_sep);
+               if (config->json_output) {
+                       if (evsel->percore &&
!config->percore_show_thread) {
+                               fprintf(config->output, "\"core\" :
\"S%d-D%d-C%d\"",
+                                       id.socket,
+                                       id.die,
+                                       id.core);
+                       } else if (id.core > -1) {
+                               fprintf(config->output, "\"cpu\" :
\"%d\", ",
+                                       id.cpu.cpu);
+                       }
+               } else {
+                       if (evsel->percore &&
!config->percore_show_thread) {
+                               fprintf(config->output,
"S%d-D%d-C%*d%s",
+                                       id.socket,
+                                       id.die,
+                                       config->csv_output ? 0 : -3,
+                                       id.core, config->csv_sep);
+                       } else if (id.core > -1) {
+                               fprintf(config->output, "CPU%*d%s",
+                                       config->csv_output ? 0 : -7,
+                                       id.cpu.cpu, config->csv_sep);
+                       }
              }
              break;

The old code was using "id.cpu.cpu > -1" while the new code is
"id.core > -1". The value printed is id.cpu.cpu and so testing id.core
makes less sense to me. Going back to the original patch:

https://lore.kernel.org/lkml/20210811224317.1811618-1-cjense@google.com/ (local)

quoted

 case AGGR_NONE:
- if (evsel->percore && !config->percore_show_thread) {
- fprintf(config->output, "S%d-D%d-C%*d%s",
- id.socket,
- id.die,
- config->csv_output ? 0 : -3,
- id.core, config->csv_sep);
+ if (config->json_output) {
+ if (evsel->percore && !config->percore_show_thread) {
+ fprintf(config->output, "\"core\" : \"S%d-D%d-C%d\"",
+ id.socket,
+ id.die,
+ id.core);
+ } else if (id.core > -1) {
+ fprintf(config->output, "\"cpu\" : \"%d\", ",
+ evsel__cpus(evsel)->map[id.core]);
+ }
+ } else {
+ if (evsel->percore && !config->percore_show_thread) {
+ fprintf(config->output, "S%d-D%d-C%*d%s",
+ id.socket,
+ id.die,
+ config->csv_output ? 0 : -3,
+ id.core, config->csv_sep);
 } else if (id.core > -1) {
 fprintf(config->output, "CPU%*d%s",
 config->csv_output ? 0 : -7,
 evsel__cpus(evsel)->map[id.core],
 config->csv_sep);
- }
+ }
+ }
+
 break;

So testing the id.core isn't a bad index makes sense. However, we
changed from core to CPU here:

https://lore.kernel.org/all/20220105061351.120843-26-irogers@google.com/ (local)

quoted

and that was because of:
https://lore.kernel.org/r/20220105061351.120843-25-irogers@google.com (local)

So I think the code needs to test CPU and not core. Whether that is
addressing the Power test failures is another matter, as James said we
may need a fix in the tests for that.

Hi Ian, James

Thanks for the reviews and suggestions.

After checking through the original commits for id.core vs cpu check,
sharing patch below to test CPU and not core.

From 4dd98d953940deb2f85176cb6b4ecbfd18dbdbf9 Mon Sep 17 00:00:00 2001
From: Athira Rajeev <redacted>
Date: Mon, 3 Oct 2022 15:47:27 +0530
Subject: [PATCH] tools/perf: Fix cpu check to use id.cpu.cpu in
aggr_printout

perf stat has options to aggregate the counts in different
modes like per socket, per core etc. The function "aggr_printout"
in util/stat-display.c which is used to print the aggregates,
has a check for cpu in case of AGGR_NONE. This check was
originally using condition : "if (id.cpu.cpu > -1)". But
this got changed after commit df936cadfb58 ("perf stat: Add
JSON output option"), which added option to output json format
for different aggregation modes. After this commit, the
check in "aggr_printout" is using "if (id.core > -1)".

The old code was using "id.cpu.cpu > -1" while the new code
is using "id.core > -1". But since the value printed is
id.cpu.cpu, fix this check to use cpu and not core.

Signed-off-by: Athira Rajeev <redacted>
Suggested-by: James Clark <redacted>
Suggested-by: Ian Rogers <irogers@google.com>

The change below works on my dual socket SkylakeX:
..
85: perf stat CSV output linter                                     :
Ok
86: perf stat csv summary test                                      : Ok
87: perf stat JSON output linter                                    : Ok
..
I don't see anything else out of the ordinary.

Thanks,
Ian

Hi Ian,
Thanks for helping with testing. Can I add your Tested-by for the patch ?

Yep.

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

Thanks,
Ian

Arnaldo,

Please suggest if I have to send as separate patch for the cpu check fix
patch pasted above:
 "tools/perf: Fix cpu check to use id.cpu.cpu in aggr_printout”

Thanks
Athira

quoted

---
 tools/perf/util/stat-display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat-display.c

b/tools/perf/util/stat-display.c
index b82844cb0ce7..cf28020798ec 100644

--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c

@@ -168,7 +168,7 @@ static void aggr_printout(struct perf_stat_config

*config,
                                       id.socket,
                                       id.die,
                                       id.core);
-                       } else if (id.core > -1) {
+                       } else if (id.cpu.cpu > -1) {
                               fprintf(config->output, "\"cpu\" :

\"%d\", ",

quoted

                                       id.cpu.cpu);
                       }

@@ -179,7 +179,7 @@ static void aggr_printout(struct perf_stat_config

*config,
                                       id.die,
                                       config->csv_output ? 0 : -3,
                                       id.core, config->csv_sep);
-                       } else if (id.core > -1) {
+                       } else if (id.cpu.cpu > -1) {
                               fprintf(config->output, "CPU%*d%s",
                                       config->csv_output ? 0 : -7,
                                       id.cpu.cpu, config->csv_sep);
--
2.31.1

Can you suggest or help to test this patch change.

To address the test failure, as James suggested, I will handle fix in
testcases and post them
as a separate patch. Plan is to add a sanity check in the tests to see
if the "physical_packagge_id" ( ie socket id ) in topology points to -1
and if so skip the test. Also in parallel, checking to see how we can
handle the aggregation modes to work incase of "-1" value for socket or
die

Thanks
Athira

quoted

Thanks,
Ian

quoted

Or the one about fixing it in the test instead? Or failing early if
the
topology can't be read?

I'm still not convinced that any of the modes where -1 is printed are
even working properly so it might be best to fix that rather than just
the printout.

quoted

James, can you point me to reference for that meaning if I have

missed anything.

quoted

It's here:

 /** Identify where counts are aggregated, -1 implies not to
aggregate. */
 struct aggr_cpu_id {

quoted

Thanks
Athira

quoted

After the fix:

<<>>
perf stat -e instructions -A -a true

Performance counter stats for 'system wide':

CPU-1                  64,034      instructions
CPU-1                  68,941      instructions
CPU-1                  59,418      instructions
CPU-1                  70,478      instructions
CPU-1                  65,201      instructions
CPU-1                  63,704      instructions
<<>>

This is caught while running "perf test" for
"stat+json_output.sh" and "stat+csv_output.sh".

Is it possible to fix the issue by making the tests cope with the

lack

quoted

of the CPU id?

quoted

Reported-by: Disha Goel <redacted>
Signed-off-by: Athira Rajeev <redacted>
---
tools/perf/util/stat-display.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/stat-display.c

b/tools/perf/util/stat-display.c

quoted

index b82844cb0ce7..1b751a730271 100644

--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c

@@ -168,10 +168,9 @@ static void aggr_printout(struct

perf_stat_config *config,

quoted

                                   id.socket,
                                   id.die,
                                   id.core);
-                   } else if (id.core > -1) {
+                   } else

This should have been "id.cpu.cpu > -1". Looks like it was changed

by

quoted

some kind of bad merge or rebase in df936cadfb because there is no
obvious justification for the change to .core in that commit.

quoted

                           fprintf(config->output, "\"cpu\" :

\"%d\", ",

quoted

                                   id.cpu.cpu);
-                   }
           } else {
                   if (evsel->percore &&

!config->percore_show_thread) {

quoted

                           fprintf(config->output,

"S%d-D%d-C%*d%s",

quoted

quoted
quoted
quoted
quoted
quoted
quoted
@@ -179,11 +178,10 @@ static void aggr_printout(struct

perf_stat_config *config,

quoted

                                   id.die,
                                   config->csv_output ? 0 : -3,
                                   id.core, config->csv_sep);
-                   } else if (id.core > -1) {
+                   } else
                           fprintf(config->output, "CPU%*d%s",
                                   config->csv_output ? 0 : -7,
                                   id.cpu.cpu, config->csv_sep);
-                   }
           }
           break;
   case AGGR_THREAD:

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help