lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 10 Jan 2022 15:56:55 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     John Garry <john.garry@...wei.com>,
        Andi Kleen <ak@...ux.intel.com>, Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Kajol Jain <kjain@...ux.ibm.com>,
        "Paul A . Clarke" <pc@...ibm.com>,
        Riccardo Mancini <rickyman7@...il.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vineet Singh <vineet.singh@...el.com>,
        James Clark <james.clark@....com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>, coresight@...ts.linaro.org,
        linux-arm-kernel@...ts.infradead.org, zhengjun.xing@...el.com,
        eranian@...gle.com
Subject: Re: [PATCH v4 02/48] perf stat: Add aggr creators that are passed a
 cpu.

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@...wei.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@...lbox perf]$ b4 am -ctsl --cc-trailers CAP-5=fWT_19OfZTTjvLUcChV4nDwqc5Zq4VE93Gak6OO4NORsA@...l.gmail.com
> 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@...wei.com>
>     + Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.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@...lbox 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

Powered by blists - more mailing lists