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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Oct 05, 2022 at 09:28:52AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Oct 05, 2022 at 10:23:39AM +0530, Athira Rajeev escreveu:
> > 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\", ",
> > 					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);
> > -- 
> > If it is confusing, shall I send it as a separate patch along with Tested-by from Ian ?
> 
> I'll have to do this by hand, tried pointing b4 to this message and it
> picked the old one, also tried to save the message and apply by hand,
> its mangled.

This is what I have now, will force push later, please triple check :-)

- Arnaldo

From b7dd96f9211e4ddbd6fa080da8dec2eac98d3f2a Mon Sep 17 00:00:00 2001
From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Date: Tue, 13 Sep 2022 17:27:17 +0530
Subject: [PATCH 1/1] perf stat: Fix aggr_printout to display CPU field
 irrespective of core value

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

  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.

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

Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Suggested-by: Ian Rogers <irogers@google.com>
Suggested-by: James Clark <james.clark@arm.com>
Signed-off-by: Athira Jajeev <atrajeev@linux.vnet.ibm.com>
Tested-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Tested-by: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nageswara R Sastry <rnsastry@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Link: https://lore.kernel.org/r/20220913115717.36191-1-atrajeev@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 df26fb5eb072be9f..5c47ee9963a7c04c 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\", ",
 					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.37.3