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

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 05-Oct-2022, at 6:05 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> 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 :-)


Sorry for all the confusion Arnaldo. Hereafter, I will resubmit the patch to avoid this.
In below patch which you shared, code change is correct. But commit message is different.
So to avoid further confusion, I went ahead and posted a separate patch in the mailing list with:

subject: [PATCH] tools/perf: Fix cpu check to use id.cpu.cpu in aggr_printout
Patch link: https://lore.kernel.org/lkml/20221006114225.66303-1-atrajeev@linux.vnet.ibm.com/T/#u

Please pick the separately send patch.

Thanks
Athira

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