[PATCH v3 0/3] Fix incorrect counts when count the same uncore event multiple times

Chun-Tse Shao posted 3 patches 7 months, 1 week ago
tools/perf/builtin-record.c                   |   7 +-
tools/perf/builtin-top.c                      |   7 +-
.../tests/shell/stat+event_uniquifying.sh     |  69 ++++++++
tools/perf/util/evlist.c                      |  66 +++++---
tools/perf/util/evlist.h                      |   3 +-
tools/perf/util/evsel.c                       | 119 ++++++++++++-
tools/perf/util/evsel.h                       |  11 +-
tools/perf/util/parse-events.c                |  90 +++++++---
tools/perf/util/stat-display.c                | 160 ++----------------
tools/perf/util/stat.c                        |  40 +----
10 files changed, 332 insertions(+), 240 deletions(-)
create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh
[PATCH v3 0/3] Fix incorrect counts when count the same uncore event multiple times
Posted by Chun-Tse Shao 7 months, 1 week ago
Let's take a look an example, the machine is SKX with 6 IMC devices.

  perf stat -e clockticks,clockticks -I 1000
  #           time             counts unit events
       1.001127430      6,901,503,174      uncore_imc_0/clockticks/
       1.001127430      3,940,896,301      uncore_imc_0/clockticks/
       2.002649722        988,376,876      uncore_imc_0/clockticks/
       2.002649722        988,376,141      uncore_imc_0/clockticks/
       3.004071319      1,000,292,675      uncore_imc_0/clockticks/
       3.004071319      1,000,294,160      uncore_imc_0/clockticks/

1) The events name should not be uniquified.
2) The initial count for the first `clockticks` is doubled.
3) Subsequent count only report for the first IMC device.

The first patch fixes 1) and 3), and the second patch fixes 2).

After these fix:

  perf stat -e clockticks,clockticks -I 1000
  #           time             counts unit events
       1.001127586      4,126,938,857      clockticks
       1.001127586      4,121,564,277      clockticks
       2.001686014      3,953,806,350      clockticks
       2.001686014      3,953,809,541      clockticks
       3.003121403      4,137,750,252      clockticks
       3.003121403      4,137,749,048      clockticks

I also tested `-A`, `--per-socket`, `--per-die` and `--per-core`, all
looks good.

Ian tested `hybrid-merge` and `hwmon`, all looks good as well.

Chun-Tse Shao (1):
  perf test: Add stat uniquifying test

Ian Rogers (2):
  perf evlist: Make uniquifying counter names consistent
  perf parse-events: Use wildcard processing to set an event to merge
    into

 tools/perf/builtin-record.c                   |   7 +-
 tools/perf/builtin-top.c                      |   7 +-
 .../tests/shell/stat+event_uniquifying.sh     |  69 ++++++++
 tools/perf/util/evlist.c                      |  66 +++++---
 tools/perf/util/evlist.h                      |   3 +-
 tools/perf/util/evsel.c                       | 119 ++++++++++++-
 tools/perf/util/evsel.h                       |  11 +-
 tools/perf/util/parse-events.c                |  90 +++++++---
 tools/perf/util/stat-display.c                | 160 ++----------------
 tools/perf/util/stat.c                        |  40 +----
 10 files changed, 332 insertions(+), 240 deletions(-)
 create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh

--
v3: Rebase with tmp.perf-tools-next. Since most of the conflicts are from
lore.kernel.org/20250403194337.40202-5-irogers@google.com, tested v3
patches with:

  perf stat -A -C 0,4-5,8 -e "instructions/cpu=0/,l1d-misses/cpu=4,cpu=5/,inst_retired.any/cpu=8/,cycles" -a sleep 0.1

   Performance counter stats for 'system wide':

  CPU0              682,860      instructions/cpu=0/              #    0.27  insn per cycle
  CPU4               53,774      l1d-misses
  CPU5               18,725      l1d-misses
  CPU8              608,698      inst_retired.any/cpu=8/
  CPU0            2,574,325      cycles
  CPU4            4,267,115      cycles
  CPU5            1,741,536      cycles
  CPU8            1,969,547      cycles

         0.102746958 seconds time elapsed

v2: lore.kernel.org/20250327225651.642965-1-ctshao@google.com
  - Fixes for `hwmon` and `--hybrid-merge`.
  - Add a test for event uniquifying.

v1: lore.kernel.org/20250326234758.480431-1-ctshao@google.com

2.49.0.1045.g170613ef41-goog
Re: [PATCH v3 0/3] Fix incorrect counts when count the same uncore event multiple times
Posted by Arnaldo Carvalho de Melo 7 months, 1 week ago
On Mon, May 12, 2025 at 02:50:29PM -0700, Chun-Tse Shao wrote:
> Let's take a look an example, the machine is SKX with 6 IMC devices.
> 
>   perf stat -e clockticks,clockticks -I 1000
>   #           time             counts unit events
>        1.001127430      6,901,503,174      uncore_imc_0/clockticks/
>        1.001127430      3,940,896,301      uncore_imc_0/clockticks/
>        2.002649722        988,376,876      uncore_imc_0/clockticks/
>        2.002649722        988,376,141      uncore_imc_0/clockticks/
>        3.004071319      1,000,292,675      uncore_imc_0/clockticks/
>        3.004071319      1,000,294,160      uncore_imc_0/clockticks/
> 
> 1) The events name should not be uniquified.
> 2) The initial count for the first `clockticks` is doubled.
> 3) Subsequent count only report for the first IMC device.
> 
> The first patch fixes 1) and 3), and the second patch fixes 2).

So, after having just the first patch applied I'm getting:

  CC      /tmp/build/perf-tools-next/util/bpf-filter-flex.o
util/parse-events.c: In function ‘__parse_events’:
util/parse-events.c:2270:25: error: implicit declaration of function ‘evlist__uniquify_name’; did you mean ‘evlist__uniquify_evsel_names’? [-Wimplicit-function-declaration]
 2270 |                         evlist__uniquify_name(evlist);
      |                         ^~~~~~~~~~~~~~~~~~~~~
      |                         evlist__uniquify_evsel_names
make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:85: /tmp/build/perf-tools-next/util/parse-events.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
make[2]: *** [Makefile.perf:798: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
make[2]: *** Waiting for unfinished jobs....


  CC      /tmp/build/perf-tools-next/pmu-events/pmu-events.o
  LD      /tmp/build/perf-tools-next/pmu-events/pmu-events-in.o
make[1]: *** [Makefile.perf:290: sub-make] Error 2
make: *** [Makefile:119: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ git log --oneline -3
6ffcaec3ac0d055a (HEAD) perf evlist: Make uniquifying counter names consistent
4102ff8b1fdaa588 perf metricgroup: Binary search when resolving referred to metrics
754baf426e099fbf perf pmu: Change aliases from list to hashmap
⬢ [acme@toolbx perf-tools-next]$

When test building the second patch, it builds, so I'm now looking if
you used things from the future or if the second patch removes the
problem.

- Arnaldo
 
> After these fix:
> 
>   perf stat -e clockticks,clockticks -I 1000
>   #           time             counts unit events
>        1.001127586      4,126,938,857      clockticks
>        1.001127586      4,121,564,277      clockticks
>        2.001686014      3,953,806,350      clockticks
>        2.001686014      3,953,809,541      clockticks
>        3.003121403      4,137,750,252      clockticks
>        3.003121403      4,137,749,048      clockticks
> 
> I also tested `-A`, `--per-socket`, `--per-die` and `--per-core`, all
> looks good.
> 
> Ian tested `hybrid-merge` and `hwmon`, all looks good as well.
> 
> Chun-Tse Shao (1):
>   perf test: Add stat uniquifying test
> 
> Ian Rogers (2):
>   perf evlist: Make uniquifying counter names consistent
>   perf parse-events: Use wildcard processing to set an event to merge
>     into
> 
>  tools/perf/builtin-record.c                   |   7 +-
>  tools/perf/builtin-top.c                      |   7 +-
>  .../tests/shell/stat+event_uniquifying.sh     |  69 ++++++++
>  tools/perf/util/evlist.c                      |  66 +++++---
>  tools/perf/util/evlist.h                      |   3 +-
>  tools/perf/util/evsel.c                       | 119 ++++++++++++-
>  tools/perf/util/evsel.h                       |  11 +-
>  tools/perf/util/parse-events.c                |  90 +++++++---
>  tools/perf/util/stat-display.c                | 160 ++----------------
>  tools/perf/util/stat.c                        |  40 +----
>  10 files changed, 332 insertions(+), 240 deletions(-)
>  create mode 100755 tools/perf/tests/shell/stat+event_uniquifying.sh
> 
> --
> v3: Rebase with tmp.perf-tools-next. Since most of the conflicts are from
> lore.kernel.org/20250403194337.40202-5-irogers@google.com, tested v3
> patches with:
> 
>   perf stat -A -C 0,4-5,8 -e "instructions/cpu=0/,l1d-misses/cpu=4,cpu=5/,inst_retired.any/cpu=8/,cycles" -a sleep 0.1
> 
>    Performance counter stats for 'system wide':
> 
>   CPU0              682,860      instructions/cpu=0/              #    0.27  insn per cycle
>   CPU4               53,774      l1d-misses
>   CPU5               18,725      l1d-misses
>   CPU8              608,698      inst_retired.any/cpu=8/
>   CPU0            2,574,325      cycles
>   CPU4            4,267,115      cycles
>   CPU5            1,741,536      cycles
>   CPU8            1,969,547      cycles
> 
>          0.102746958 seconds time elapsed
> 
> v2: lore.kernel.org/20250327225651.642965-1-ctshao@google.com
>   - Fixes for `hwmon` and `--hybrid-merge`.
>   - Add a test for event uniquifying.
> 
> v1: lore.kernel.org/20250326234758.480431-1-ctshao@google.com
> 
> 2.49.0.1045.g170613ef41-goog
Re: [PATCH v3 0/3] Fix incorrect counts when count the same uncore event multiple times
Posted by Arnaldo Carvalho de Melo 7 months, 1 week ago
On Tue, May 13, 2025 at 05:02:33PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, May 12, 2025 at 02:50:29PM -0700, Chun-Tse Shao wrote:
> > Let's take a look an example, the machine is SKX with 6 IMC devices.
> > 
> >   perf stat -e clockticks,clockticks -I 1000
> >   #           time             counts unit events
> >        1.001127430      6,901,503,174      uncore_imc_0/clockticks/
> >        1.001127430      3,940,896,301      uncore_imc_0/clockticks/
> >        2.002649722        988,376,876      uncore_imc_0/clockticks/
> >        2.002649722        988,376,141      uncore_imc_0/clockticks/
> >        3.004071319      1,000,292,675      uncore_imc_0/clockticks/
> >        3.004071319      1,000,294,160      uncore_imc_0/clockticks/
> > 
> > 1) The events name should not be uniquified.
> > 2) The initial count for the first `clockticks` is doubled.
> > 3) Subsequent count only report for the first IMC device.
> > 
> > The first patch fixes 1) and 3), and the second patch fixes 2).
> 
> So, after having just the first patch applied I'm getting:
> 
>   CC      /tmp/build/perf-tools-next/util/bpf-filter-flex.o
> util/parse-events.c: In function ‘__parse_events’:
> util/parse-events.c:2270:25: error: implicit declaration of function ‘evlist__uniquify_name’; did you mean ‘evlist__uniquify_evsel_names’? [-Wimplicit-function-declaration]
>  2270 |                         evlist__uniquify_name(evlist);
>       |                         ^~~~~~~~~~~~~~~~~~~~~
>       |                         evlist__uniquify_evsel_names
> make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:85: /tmp/build/perf-tools-next/util/parse-events.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
> make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
> make[2]: *** [Makefile.perf:798: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
> make[2]: *** Waiting for unfinished jobs....
> 
> 
>   CC      /tmp/build/perf-tools-next/pmu-events/pmu-events.o
>   LD      /tmp/build/perf-tools-next/pmu-events/pmu-events-in.o
> make[1]: *** [Makefile.perf:290: sub-make] Error 2
> make: *** [Makefile:119: install-bin] Error 2
> make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ 
> ⬢ [acme@toolbx perf-tools-next]$ git log --oneline -3
> 6ffcaec3ac0d055a (HEAD) perf evlist: Make uniquifying counter names consistent
> 4102ff8b1fdaa588 perf metricgroup: Binary search when resolving referred to metrics
> 754baf426e099fbf perf pmu: Change aliases from list to hashmap
> ⬢ [acme@toolbx perf-tools-next]$
> 
> When test building the second patch, it builds, so I'm now looking if
> you used things from the future or if the second patch removes the
> problem.

At that point:

util/parse-events.c: In function ‘__parse_events’:
util/parse-events.c:2270:25: error: implicit declaration of function ‘evlist__uniquify_name’; did you mean ‘evlist__uniquify_evsel_names’? [-Wimplicit-function-declaration]
 2270 |                         evlist__uniquify_name(evlist);
      |                         ^~~~~~~~~~~~~~~~~~~~~
      |                         evlist__uniquify_evsel_names
make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:85: /tmp/build/perf-tools-next/util/parse-events.o] Error 1
make[4]: *** Waiting for unfinished jobs....
  LD      /tmp/build/perf-tools-next/util/scripting-engines/perf-util-in.o
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
make[2]: *** [Makefile.perf:798: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
make[1]: *** [Makefile.perf:290: sub-make] Error 2
make: *** [Makefile:119: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
⬢ [acme@toolbx perf-tools-next]$ 
⬢ [acme@toolbx perf-tools-next]$ git grep evlist__uniquify_name
tools/perf/util/parse-events.c:                 evlist__uniquify_name(evlist);
⬢ [acme@toolbx perf-tools-next]$

So its the later, the second patch builds because:

⬢ [acme@toolbx perf-tools-next]$ git rebase --continue
Stopped at ed3b26e31f42d1e4...  perf parse-events: Use wildcard processing to set an event to merge into
You can amend the commit now, with

  git commit --amend 

Once you are satisfied with your changes, run

  git rebase --continue
⬢ [acme@toolbx perf-tools-next]$ git grep evlist__uniquify_name
⬢ [acme@toolbx perf-tools-next]$ git show | grep evlist__uniquify_name
-			evlist__uniquify_name(evlist);
⬢ [acme@toolbx perf-tools-next]$

That function isn't there anymore.

Please try to fix this and build it patch by patch so that we don't
introduce bisection breakage patches.

Thanks,

- Arnaldo
Re: [PATCH v3 0/3] Fix incorrect counts when count the same uncore event multiple times
Posted by Chun-Tse Shao 7 months, 1 week ago
Thank you Arnaldo, sorry for my mistake. Please check the v4 patchset:
lore.kernel.org/20250513215401.2315949-1-ctshao@google.com

-CT


On Tue, May 13, 2025 at 1:04 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, May 13, 2025 at 05:02:33PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, May 12, 2025 at 02:50:29PM -0700, Chun-Tse Shao wrote:
> > > Let's take a look an example, the machine is SKX with 6 IMC devices.
> > >
> > >   perf stat -e clockticks,clockticks -I 1000
> > >   #           time             counts unit events
> > >        1.001127430      6,901,503,174      uncore_imc_0/clockticks/
> > >        1.001127430      3,940,896,301      uncore_imc_0/clockticks/
> > >        2.002649722        988,376,876      uncore_imc_0/clockticks/
> > >        2.002649722        988,376,141      uncore_imc_0/clockticks/
> > >        3.004071319      1,000,292,675      uncore_imc_0/clockticks/
> > >        3.004071319      1,000,294,160      uncore_imc_0/clockticks/
> > >
> > > 1) The events name should not be uniquified.
> > > 2) The initial count for the first `clockticks` is doubled.
> > > 3) Subsequent count only report for the first IMC device.
> > >
> > > The first patch fixes 1) and 3), and the second patch fixes 2).
> >
> > So, after having just the first patch applied I'm getting:
> >
> >   CC      /tmp/build/perf-tools-next/util/bpf-filter-flex.o
> > util/parse-events.c: In function ‘__parse_events’:
> > util/parse-events.c:2270:25: error: implicit declaration of function ‘evlist__uniquify_name’; did you mean ‘evlist__uniquify_evsel_names’? [-Wimplicit-function-declaration]
> >  2270 |                         evlist__uniquify_name(evlist);
> >       |                         ^~~~~~~~~~~~~~~~~~~~~
> >       |                         evlist__uniquify_evsel_names
> > make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:85: /tmp/build/perf-tools-next/util/parse-events.o] Error 1
> > make[4]: *** Waiting for unfinished jobs....
> > make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
> > make[2]: *** [Makefile.perf:798: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
> > make[2]: *** Waiting for unfinished jobs....
> >
> >
> >   CC      /tmp/build/perf-tools-next/pmu-events/pmu-events.o
> >   LD      /tmp/build/perf-tools-next/pmu-events/pmu-events-in.o
> > make[1]: *** [Makefile.perf:290: sub-make] Error 2
> > make: *** [Makefile:119: install-bin] Error 2
> > make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$
> > ⬢ [acme@toolbx perf-tools-next]$ git log --oneline -3
> > 6ffcaec3ac0d055a (HEAD) perf evlist: Make uniquifying counter names consistent
> > 4102ff8b1fdaa588 perf metricgroup: Binary search when resolving referred to metrics
> > 754baf426e099fbf perf pmu: Change aliases from list to hashmap
> > ⬢ [acme@toolbx perf-tools-next]$
> >
> > When test building the second patch, it builds, so I'm now looking if
> > you used things from the future or if the second patch removes the
> > problem.
>
> At that point:
>
> util/parse-events.c: In function ‘__parse_events’:
> util/parse-events.c:2270:25: error: implicit declaration of function ‘evlist__uniquify_name’; did you mean ‘evlist__uniquify_evsel_names’? [-Wimplicit-function-declaration]
>  2270 |                         evlist__uniquify_name(evlist);
>       |                         ^~~~~~~~~~~~~~~~~~~~~
>       |                         evlist__uniquify_evsel_names
> make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:85: /tmp/build/perf-tools-next/util/parse-events.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
>   LD      /tmp/build/perf-tools-next/util/scripting-engines/perf-util-in.o
> make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
> make[2]: *** [Makefile.perf:798: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
> make[1]: *** [Makefile.perf:290: sub-make] Error 2
> make: *** [Makefile:119: install-bin] Error 2
> make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> ⬢ [acme@toolbx perf-tools-next]$
> ⬢ [acme@toolbx perf-tools-next]$ git grep evlist__uniquify_name
> tools/perf/util/parse-events.c:                 evlist__uniquify_name(evlist);
> ⬢ [acme@toolbx perf-tools-next]$
>
> So its the later, the second patch builds because:
>
> ⬢ [acme@toolbx perf-tools-next]$ git rebase --continue
> Stopped at ed3b26e31f42d1e4...  perf parse-events: Use wildcard processing to set an event to merge into
> You can amend the commit now, with
>
>   git commit --amend
>
> Once you are satisfied with your changes, run
>
>   git rebase --continue
> ⬢ [acme@toolbx perf-tools-next]$ git grep evlist__uniquify_name
> ⬢ [acme@toolbx perf-tools-next]$ git show | grep evlist__uniquify_name
> -                       evlist__uniquify_name(evlist);
> ⬢ [acme@toolbx perf-tools-next]$
>
> That function isn't there anymore.
>
> Please try to fix this and build it patch by patch so that we don't
> introduce bisection breakage patches.
>
> Thanks,
>
> - Arnaldo