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@huawei.com 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@google.com/ 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@mail.gmail.com Looking up https://lore.kernel.org/r/CAP-5%3DfWT_19OfZTTjvLUcChV4nDwqc5Zq4VE93Gak6OO4NO... 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@huawei.com + Signed-off-by: Arnaldo Carvalho de Melo acme@redhat.com + Link: https://lore.kernel.org/r/20220105061351.120843-2-irogers@google.com
<BIG SNIP> Cover: ./v4_20220104_irogers_refactor_perf_cpumap.cover Link: https://lore.kernel.org/r/20220105061351.120843-1-irogers@google.com 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