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

[Ian Rogers <irogers@google.com>]

[-- Attachment #1: Type: text/plain, Size: 18965 bytes --]

On Tue, Oct 4, 2022, 12:06 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com>
wrote:

>
>
> > On 04-Oct-2022, at 12:21 AM, Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Oct 3, 2022 at 7:03 AM atrajeev <atrajeev@imap.linux.ibm.com>
> wrote:
> >>
> >> On 2022-10-02 05:17, Ian Rogers wrote:
> >>> On Thu, Sep 29, 2022 at 5:56 AM James Clark <james.clark@arm.com>
> >>> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 29/09/2022 09:49, Athira Rajeev wrote:
> >>>>>
> >>>>>
> >>>>>> On 28-Sep-2022, at 9:05 PM, James Clark <james.clark@arm.com>
> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> Hi James,
> >>>>>
> >>>>> Thanks for looking at the patch and sharing review comments.
> >>>>>
> >>>>>> On 13/09/2022 12:57, Athira Rajeev wrote:
> >>>>>>> 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
> >>>>>> being read from the topology file.
> >>>>>
> >>>>> The cpu value is filled in this function:
> >>>>>
> >>>>> Function : aggr_cpu_id__cpu
> >>>>> Code: util/cpumap.c
> >>>>>
> >>>>>>
> >>>>>>> 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
> a
> >>>>>> good one to me, otherwise the output looks a bit broken to users.
> Are
> >>>>>> 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
> >>>>>> 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
> >>>>> 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
> >>>>> S-1-D-1-C-1          1                  2      context-switches
>            #    1.879 K/sec
> >>>>> S-1-D-1-C-1          1                  0      cpu-migrations
>            #    0.000 /sec
> >>>>>
> >>>>> Here though the value is -1, we are displaying it. Where as in case
> of cpu, the first column will be
> >>>>> 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
> >>>>>                 2      context-switches
> >>>>>                 0      cpu-migrations
> >>>>>
> >>>>>
> >>>>> No sure, whether there are scripts out there, which consume the
> current format and
> >>>>> not displaying -1 may break it. That is why we tried with change to
> remove check for cpu, similar to
> >>>>> 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.
> >>>>
> >>>>>
> >>>>> Also perf code ie “aggr_cpu_id__empty” in util/cpumap.c initialises
> the
> >>>>> values to -1 . I was checking to see where we are mapping -1 to “to
> not aggregate”.
> >>>>> What I could find is AGGR_NONE ( which is for no-aggr ) has value as
> zero.
> >>>>>
> >>>>> 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/
> >>>  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/
> >>> and that was because of:
> >>> https://lore.kernel.org/r/20220105061351.120843-25-irogers@google.com
> >>>
> >>> 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 <atrajeev@linux.vnet.ibm.com>
> >> 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 <atrajeev@linux.vnet.ibm.com>
> >> Suggested-by: James Clark <james.clark@arm.com>
> >> 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
> >> ---
> >>  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\", ",
> >>                                        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
> >>
> >>> Thanks,
> >>> Ian
> >>>
> >>>> 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.
> >>>>
> >>>>> James, can you point me to reference for that meaning if I have
> missed anything.
> >>>>
> >>>> It's here:
> >>>>
> >>>>  /** Identify where counts are aggregated, -1 implies not to
> >>>> aggregate. */
> >>>>  struct aggr_cpu_id {
> >>>>
> >>>>>
> >>>>> Thanks
> >>>>> Athira
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> 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
> >>>>>> of the CPU id?
> >>>>>>
> >>>>>>>
> >>>>>>> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> >>>>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> >>>>>>> ---
> >>>>>>> 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
> >>>>>>> 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,
> >>>>>>>                                    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
> >>>>>> some kind of bad merge or rebase in df936cadfb because there is no
> >>>>>> obvious justification for the change to .core in that commit.
> >>>>>
> >>>>>>
> >>>>>>>                            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",
> >>>>>>> @@ -179,11 +178,10 @@ 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
> >>>>>>>                            fprintf(config->output, "CPU%*d%s",
> >>>>>>>                                    config->csv_output ? 0 : -7,
> >>>>>>>                                    id.cpu.cpu, config->csv_sep);
> >>>>>>> -                   }
> >>>>>>>            }
> >>>>>>>            break;
> >>>>>>>    case AGGR_THREAD:
> >>>>>
>
>

[-- Attachment #2: Type: text/html, Size: 29653 bytes --]