On Mon, Oct 3, 2022 at 7:03 AM atrajeev <atraj...@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.cl...@arm.com>
> > wrote:
> >>
> >>
> >>
> >> On 29/09/2022 09:49, Athira Rajeev wrote:
> >> >
> >> >
> >> >> On 28-Sep-2022, at 9:05 PM, James Clark <james.cl...@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-cje...@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-irog...@google.com/
> > and that was because of:
> > https://lore.kernel.org/r/20220105061351.120843-25-irog...@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 <atraj...@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 <atraj...@linux.vnet.ibm.com>
> Suggested-by: James Clark <james.cl...@arm.com>
> Suggested-by: Ian Rogers <irog...@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
> ---
> 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 <disg...@linux.vnet.ibm.com>
> >> >>> Signed-off-by: Athira Rajeev <atraj...@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:
> >> >
