Re: [PATCH v4 02/48] perf stat: Add aggr creators that are passed a cpu.

From: Arnaldo Carvalho de Melo
Date: Mon Jan 10 2022 - 13:57:01 EST

Em Mon, Jan 10, 2022 at 03:53:45PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 10, 2022 at 03:52:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Jan 10, 2022 at 09:36:49AM -0800, Ian Rogers escreveu:
> > > On Mon, Jan 10, 2022 at 9:10 AM John Garry <john.garry@xxxxxxxxxx> wrote:
> > > >
> > > > On 05/01/2022 06:13, Ian Rogers wrote:
> > > > >
> > > > > +struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
> > > > > + void *data)
> > > > > +{
> > > > > + if (idx < 0 || idx > map->nr)
> > > > > + return cpu_map__empty_aggr_cpu_id();
> > > > > +
> > > > > + return cpu_map__get_socket_aggr_by_cpu(map->map[idx], data);
> > > > > +}
> > > > > +
> > > >
> > > >
> > > > This is later deleted in the series. Can the series be reworked so that
> > > > we don't add stuff and then later delete it? One reason for that
> > > > approach is that we don't spend time reviewing something which will be
> > > > deleted, especially in such a big series...
> > >
> > > Hi John,
> > >
> > > I think you are asking to squash:
> > > https://lore.kernel.org/lkml/20220105061351.120843-8-irogers@xxxxxxxxxx/
> > > into this change. There are other similar related changes that may
> > > also be squashed. The changes are trying to introduce a new API and
> > > then add changes to switch over to using it. This is with a view to
> > > making bisection easier, have each change only do 1 thing and so on. I
> > > believe the format of the patches is house style, but it is fine to
> > > squash changes together too. Having sent patches to Arnaldo and having
> > > had them split I'm reluctant to do a v5 with them squashed without him
> > > expressing a preference.
> >
> > Right, sometimes this is needed, I'm getting the patchkit now to test
> > build it in my containers and will go patch by patch reviewing.
>
> Good start:
>
> ⬢[acme@toolbox perf]$ b4 am -ctsl --cc-trailers CAP-5=fWT_19OfZTTjvLUcChV4nDwqc5Zq4VE93Gak6OO4NORsA@xxxxxxxxxxxxxx
> Looking up https://lore.kernel.org/r/CAP-5%3DfWT_19OfZTTjvLUcChV4nDwqc5Zq4VE93Gak6OO4NORsA%40mail.gmail.com
> Grabbing thread from lore.kernel.org/all/CAP-5%3DfWT_19OfZTTjvLUcChV4nDwqc5Zq4VE93Gak6OO4NORsA%40mail.gmail.com/t.mbox.gz
> Checking for newer revisions on https://lore.kernel.org/all/
> Analyzing 58 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
> ✓ [PATCH v4 1/48] libperf: Add comments to perf_cpu_map.
> + Reviewed-by: John Garry <john.garry@xxxxxxxxxx>
> + Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> + Link: https://lore.kernel.org/r/20220105061351.120843-2-irogers@xxxxxxxxxx
> <BIG SNIP>
> Cover: ./v4_20220104_irogers_refactor_perf_cpumap.cover
> Link: https://lore.kernel.org/r/20220105061351.120843-1-irogers@xxxxxxxxxx
> Base: applies clean to current tree
> git checkout -b v4_20220104_irogers_google_com HEAD
> git am ./v4_20220104_irogers_refactor_perf_cpumap.mbx
>

oops:

CC /tmp/build/perf/util/unwind-libunwind-local.o
util/bpf_counter_cgroup.c: In function ‘bperf_load_program’:
util/bpf_counter_cgroup.c:51:26: error: incompatible types when initializing type ‘int’ using type ‘struct perf_cpu’
51 | int total_cpus = cpu__max_cpu();
| ^~~~~~~~~~~~
util/bpf_counter_cgroup.c:127:74: error: invalid operands to binary + (have ‘int’ and ‘struct perf_cpu’)
127 | __u32 idx = evsel->core.idx * total_cpus +
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
| |
| int
128 | evlist->core.all_cpus->map[cpu];
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| struct perf_cpu
util/bpf_counter_cgroup.c: In function ‘bperf_cgrp__sync_counters’:
util/bpf_counter_cgroup.c:215:23: error: incompatible types when assigning to type ‘int’ from type ‘struct perf_cpu’
215 | cpu = evlist->core.all_cpus->map[i];
| ^~~~~~
util/bpf_counter_cgroup.c: In function ‘bperf_cgrp__read’:
util/bpf_counter_cgroup.c:248:26: error: incompatible types when initializing type ‘int’ using type ‘struct perf_cpu’
248 | int total_cpus = cpu__max_cpu();
| ^~~~~~~~~~~~
util/bpf_counter_cgroup.c:275:31: error: incompatible types when assigning to type ‘int’ from type ‘struct perf_cpu’
275 | cpu = evlist->core.all_cpus->map[i];
| ^~~~~~
make[4]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:97: /tmp/build/perf/util/bpf_counter_cgroup.o] Error 1
make[4]: *** Waiting for unfinished jobs....
util/bpf_counter.c: In function ‘bperf__load’:
util/bpf_counter.c:543:31: error: incompatible types when assigning to type ‘__u32’ {aka ‘unsigned int’} from type ‘struct perf_cpu’
543 | key = evsel->core.cpus->map[i];
| ^~~~~
util/bpf_counter.c: In function ‘bperf_sync_counters’:
util/bpf_counter.c:587:23: error: incompatible types when assigning to type ‘int’ from type ‘struct perf_cpu’
587 | cpu = all_cpu_map->map[i];
| ^~~~~~~~~~~
util/bpf_counter.c: In function ‘bperf__read’:
util/bpf_counter.c:608:29: error: incompatible types when initializing type ‘__u32’ {aka ‘unsigned int’} using type ‘struct perf_cpu’
608 | __u32 num_cpu_bpf = cpu__max_cpu();
| ^~~~~~~~~~~~
In file included from /var/home/acme/git/perf/tools/perf/util/cpumap.h:8,
from /var/home/acme/git/perf/tools/perf/util/env.h:7,
from util/cgroup.h:8,
from util/bpf_counter.c:22:
/var/home/acme/git/perf/tools/lib/perf/include/perf/cpumap.h:25:33: error: incompatible types when assigning to type ‘__u32’ {aka ‘unsigned int’} from type ‘struct perf_cpu’
25 | for ((idx) = 0, (cpu) = perf_cpu_map__cpu(cpus, idx); \
| ^~~~~~~~~~~~~~~~~
util/bpf_counter.c:626:25: note: in expansion of macro ‘perf_cpu_map__for_each_cpu’
626 | perf_cpu_map__for_each_cpu(cpu, j, all_cpu_map) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
/var/home/acme/git/perf/tools/lib/perf/include/perf/cpumap.h:26:20: error: comparison of integer expressions of different signedness: ‘__u32’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
26 | (idx) < perf_cpu_map__nr(cpus); \
| ^
util/bpf_counter.c:626:25: note: in expansion of macro ‘perf_cpu_map__for_each_cpu’
626 | perf_cpu_map__for_each_cpu(cpu, j, all_cpu_map) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
/var/home/acme/git/perf/tools/lib/perf/include/perf/cpumap.h:27:31: error: incompatible types when assigning to type ‘__u32’ {aka ‘unsigned int’} from type ‘struct perf_cpu’
27 | (idx)++, (cpu) = perf_cpu_map__cpu(cpus, idx))
| ^~~~~~~~~~~~~~~~~
util/bpf_counter.c:626:25: note: in expansion of macro ‘perf_cpu_map__for_each_cpu’
626 | perf_cpu_map__for_each_cpu(cpu, j, all_cpu_map) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
util/bpf_counter.c:633:31: error: incompatible types when assigning to type ‘__u32’ {aka ‘unsigned int’} from type ‘struct perf_cpu’
633 | cpu = evsel->core.cpus->map[i];
| ^~~~~
util/bpf_counter.c:611:21: error: unused variable ‘num_cpu’ [-Werror=unused-variable]
611 | __u32 i, j, num_cpu;
| ^~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/bpf_counter.o] Error 1
util/bpf_ftrace.c: In function ‘perf_ftrace__latency_prepare_bpf’:
util/bpf_ftrace.c:66:31: error: implicit declaration of function ‘cpu_map__cpu’; did you mean ‘cpu__max_cpu’? [-Werror=implicit-function-declaration]
66 | cpu = cpu_map__cpu(ftrace->evlist->core.cpus, i);
| ^~~~~~~~~~~~
| cpu__max_cpu
util/bpf_ftrace.c: In function ‘perf_ftrace__latency_read_bpf’:
util/bpf_ftrace.c:125:21: error: incompatible types when initializing type ‘int’ using type ‘struct perf_cpu’
125 | int ncpus = cpu__max_cpu();
| ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/bpf_ftrace.o] Error 1
LD /tmp/build/perf/util/intel-pt-decoder/perf-in.o
LD /tmp/build/perf/util/scripting-engines/perf-in.o
make[3]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:139: util] Error 2
make[2]: *** [Makefile.perf:665: /tmp/build/perf/perf-in.o] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile.perf:240: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/var/home/acme/git/perf/tools/perf'

Performance counter stats for 'make -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf -C tools/perf install-bin':

279,675,447,200 cycles:u
348,898,435,627 instructions:u # 1.25 insn per cycle

4.978587957 seconds time elapsed

68.698979000 seconds user
12.541229000 seconds sys


⬢[acme@toolbox perf]$

This is building with:

$ make BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf -C tools/perf install-bin


In general for such refactorings its important to run:

make -C tools/perf build-test


But there is no entry there with BUILD_BPF_SKEL=1, will fix that now and
try to fix this in the patchkit as well.

- Arnaldo