[PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value
Athira Rajeev
atrajeev at linux.vnet.ibm.com
Thu Oct 6 23:46:14 AEDT 2022
> On 05-Oct-2022, at 6:05 PM, Arnaldo Carvalho de Melo <acme at 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 at 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 at linux.vnet.ibm.com>
> Suggested-by: Ian Rogers <irogers at google.com>
> Suggested-by: James Clark <james.clark at arm.com>
> Signed-off-by: Athira Jajeev <atrajeev at linux.vnet.ibm.com>
> Tested-by: Disha Goel <disgoel at linux.vnet.ibm.com>
> Tested-by: Ian Rogers <irogers at google.com>
> Cc: Jiri Olsa <jolsa at kernel.org>
> Cc: Kajol Jain <kjain at linux.ibm.com>
> Cc: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
> Cc: Michael Ellerman <mpe at ellerman.id.au>
> Cc: Nageswara R Sastry <rnsastry at linux.ibm.com>
> Cc: linuxppc-dev at 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 at 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
>
More information about the Linuxppc-dev
mailing list