diff mbox series

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

Message ID 20220105061351.120843-3-irogers@google.com (mailing list archive)
State New, archived
Headers show
Series Refactor perf cpumap | expand

Commit Message

Ian Rogers Jan. 5, 2022, 6:13 a.m. UTC 
The cpu_map and index can get confused. Add variants of the cpu_map__get
routines that are passed a cpu. Make the existing cpu_map__get routines
use the new functions with a view to remove them when no longer used.

Reviewed-by: James Clark <james.clark@arm.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++-----------------
 tools/perf/util/cpumap.h |  6 ++-
 2 files changed, 51 insertions(+), 34 deletions(-)

Comments

John Garry Jan. 10, 2022, 5:10 p.m. UTC | #1 
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...

If it really makes sense to do it this way then fine.

Thanks,
John
Ian Rogers Jan. 10, 2022, 5:36 p.m. UTC | #2 
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.

Thanks,
Ian

> If it really makes sense to do it this way then fine.
>
> Thanks,
> John
John Garry Jan. 10, 2022, 5:51 p.m. UTC | #3 
On 10/01/2022 17:36, Ian Rogers wrote:
> I think you are asking to squash:
> https://lore.kernel.org/lkml/20220105061351.120843-8-irogers@google.com/
> into this change. 

That's the general idea.

 > 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.
 >

I don't feel so strongly. But I do think that ability to review should 
take preference to providing simple bisections.

Anyway, I'll stop talking and actually have a look.

Cheers,
John
Arnaldo Carvalho de Melo Jan. 10, 2022, 6:52 p.m. UTC | #4 
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.

- Arnaldo
 
> Thanks,
> Ian
> 
> > If it really makes sense to do it this way then fine.
> >
> > Thanks,
> > John
Arnaldo Carvalho de Melo Jan. 10, 2022, 6:53 p.m. UTC | #5 
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_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@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

:-)

- Arnaldo
Arnaldo Carvalho de Melo Jan. 10, 2022, 6:56 p.m. UTC | #6 
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_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@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
John Garry Jan. 10, 2022, 7:17 p.m. UTC | #7 
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)

Incidentally should this be >= map->nr?

> +		return cpu_map__empty_aggr_cpu_id();
> +
> +	return cpu_map__get_socket_aggr_by_cpu(map->map[idx], data);
> +}
> +
Arnaldo Carvalho de Melo Jan. 11, 2022, 7:33 p.m. UTC | #8 
Em Tue, Jan 04, 2022 at 10:13:05PM -0800, Ian Rogers escreveu:
> The cpu_map and index can get confused. Add variants of the cpu_map__get
> routines that are passed a cpu. Make the existing cpu_map__get routines
> use the new functions with a view to remove them when no longer used.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++-----------------
>  tools/perf/util/cpumap.h |  6 ++-
>  2 files changed, 51 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 87d3eca9b872..49fba2c53822 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -128,21 +128,23 @@ int cpu_map__get_socket_id(int cpu)
>  	return ret ?: value;
>  }
>  
> -struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
> -					void *data __maybe_unused)
> +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused)
>  {
> -	int cpu;
>  	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
>  
> -	if (idx > map->nr)
> -		return id;
> -
> -	cpu = map->map[idx];
> -
>  	id.socket = cpu_map__get_socket_id(cpu);
>  	return id;
>  }
>  
> +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 'idx < 0' wasn't in the original code nor is described in the
comment log message, please avoid doing this, this may be harmless or
even a good hardening, but either way would be interesting to have it in
a separate patch. This eases review as in the end this code is just a
refactoring, moving things around but in the end should be equivalent code.

There are a few more, please consider this and if you agree, to speed
things up I can make the changes here, if I think this won't fallout in
changes to subsequent patches touching this area.

- Arnaldo

>  static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer)
>  {
>  	struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer;
> @@ -200,15 +202,10 @@ int cpu_map__get_die_id(int cpu)
>  	return ret ?: value;
>  }
>  
> -struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
> +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data)
>  {
> -	int cpu, die;
> -	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> -
> -	if (idx > map->nr)
> -		return id;
> -
> -	cpu = map->map[idx];
> +	struct aggr_cpu_id id;
> +	int die;
>  
>  	die = cpu_map__get_die_id(cpu);
>  	/* There is no die_id on legacy system. */
> @@ -220,7 +217,7 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
>  	 * with the socket ID and then add die to
>  	 * make a unique ID.
>  	 */
> -	id = cpu_map__get_socket(map, idx, data);
> +	id = cpu_map__get_socket_aggr_by_cpu(cpu, data);
>  	if (cpu_map__aggr_cpu_id_is_empty(id))
>  		return id;
>  
> @@ -228,6 +225,15 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
>  	return id;
>  }
>  
> +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx,
> +				    void *data)
> +{
> +	if (idx < 0 || idx > map->nr)
> +		return cpu_map__empty_aggr_cpu_id();

Ditto

> +
> +	return cpu_map__get_die_aggr_by_cpu(map->map[idx], data);
> +}
> +
>  int cpu_map__get_core_id(int cpu)
>  {
>  	int value, ret = cpu__get_topology_int(cpu, "core_id", &value);
> @@ -239,20 +245,13 @@ int cpu_map__get_node_id(int cpu)
>  	return cpu__get_node(cpu);
>  }
>  
> -struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data)
>  {
> -	int cpu;
> -	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> -
> -	if (idx > map->nr)
> -		return id;
> -
> -	cpu = map->map[idx];
> -
> -	cpu = cpu_map__get_core_id(cpu);
> +	struct aggr_cpu_id id;
> +	int core = cpu_map__get_core_id(cpu);
>  
>  	/* cpu_map__get_die returns a struct with socket and die set*/
> -	id = cpu_map__get_die(map, idx, data);
> +	id = cpu_map__get_die_aggr_by_cpu(cpu, data);
>  	if (cpu_map__aggr_cpu_id_is_empty(id))
>  		return id;
>  
> @@ -260,19 +259,33 @@ struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da
>  	 * core_id is relative to socket and die, we need a global id.
>  	 * So we combine the result from cpu_map__get_die with the core id
>  	 */
> -	id.core = cpu;
> +	id.core = core;
>  	return id;
> +
>  }
>  
> -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused)
> +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> +{
> +	if (idx < 0 || idx > map->nr)
> +		return cpu_map__empty_aggr_cpu_id();

Ditto

> +
> +	return cpu_map__get_core_aggr_by_cpu(map->map[idx], data);
> +}
> +
> +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused)
>  {
>  	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
>  
> +	id.node = cpu_map__get_node_id(cpu);
> +	return id;
> +}
> +
> +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data)
> +{
>  	if (idx < 0 || idx >= map->nr)
> -		return id;
> +		return cpu_map__empty_aggr_cpu_id();
>  
> -	id.node = cpu_map__get_node_id(map->map[idx]);
> -	return id;
> +	return cpu_map__get_node_aggr_by_cpu(map->map[idx], data);
>  }
>  
>  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp)
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index a27eeaf086e8..c62d67704425 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -31,13 +31,17 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
>  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
>  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
>  int cpu_map__get_socket_id(int cpu);
> +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data);
>  struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data);
>  int cpu_map__get_die_id(int cpu);
> +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data);
>  struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data);
>  int cpu_map__get_core_id(int cpu);
> +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data);
>  struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data);
>  int cpu_map__get_node_id(int cpu);
> -struct aggr_cpu_id  cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data);
> +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
>  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp);
>  int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep);
>  int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep);
> -- 
> 2.34.1.448.ga2b2bfdf31-goog
Ian Rogers Jan. 11, 2022, 7:36 p.m. UTC | #9 
On Tue, Jan 11, 2022 at 11:33 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Jan 04, 2022 at 10:13:05PM -0800, Ian Rogers escreveu:
> > The cpu_map and index can get confused. Add variants of the cpu_map__get
> > routines that are passed a cpu. Make the existing cpu_map__get routines
> > use the new functions with a view to remove them when no longer used.
> >
> > Reviewed-by: James Clark <james.clark@arm.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++-----------------
> >  tools/perf/util/cpumap.h |  6 ++-
> >  2 files changed, 51 insertions(+), 34 deletions(-)
> >
> > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > index 87d3eca9b872..49fba2c53822 100644
> > --- a/tools/perf/util/cpumap.c
> > +++ b/tools/perf/util/cpumap.c
> > @@ -128,21 +128,23 @@ int cpu_map__get_socket_id(int cpu)
> >       return ret ?: value;
> >  }
> >
> > -struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
> > -                                     void *data __maybe_unused)
> > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused)
> >  {
> > -     int cpu;
> >       struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> >
> > -     if (idx > map->nr)
> > -             return id;
> > -
> > -     cpu = map->map[idx];
> > -
> >       id.socket = cpu_map__get_socket_id(cpu);
> >       return id;
> >  }
> >
> > +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 'idx < 0' wasn't in the original code nor is described in the
> comment log message, please avoid doing this, this may be harmless or
> even a good hardening, but either way would be interesting to have it in
> a separate patch. This eases review as in the end this code is just a
> refactoring, moving things around but in the end should be equivalent code.
>
> There are a few more, please consider this and if you agree, to speed
> things up I can make the changes here, if I think this won't fallout in
> changes to subsequent patches touching this area.
>
> - Arnaldo

Fwiw, there's the same issue in cpu_map__get_die that's also in
cpu_map__get_core, but weirdly not copied into cpu_map__get_node. As
these functions are removed later I think doing nothing is best here.

Thanks,
Ian

> >  static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer)
> >  {
> >       struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer;
> > @@ -200,15 +202,10 @@ int cpu_map__get_die_id(int cpu)
> >       return ret ?: value;
> >  }
> >
> > -struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
> > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data)
> >  {
> > -     int cpu, die;
> > -     struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > -
> > -     if (idx > map->nr)
> > -             return id;
> > -
> > -     cpu = map->map[idx];
> > +     struct aggr_cpu_id id;
> > +     int die;
> >
> >       die = cpu_map__get_die_id(cpu);
> >       /* There is no die_id on legacy system. */
> > @@ -220,7 +217,7 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
> >        * with the socket ID and then add die to
> >        * make a unique ID.
> >        */
> > -     id = cpu_map__get_socket(map, idx, data);
> > +     id = cpu_map__get_socket_aggr_by_cpu(cpu, data);
> >       if (cpu_map__aggr_cpu_id_is_empty(id))
> >               return id;
> >
> > @@ -228,6 +225,15 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
> >       return id;
> >  }
> >
> > +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx,
> > +                                 void *data)
> > +{
> > +     if (idx < 0 || idx > map->nr)
> > +             return cpu_map__empty_aggr_cpu_id();
>
> Ditto
>
> > +
> > +     return cpu_map__get_die_aggr_by_cpu(map->map[idx], data);
> > +}
> > +
> >  int cpu_map__get_core_id(int cpu)
> >  {
> >       int value, ret = cpu__get_topology_int(cpu, "core_id", &value);
> > @@ -239,20 +245,13 @@ int cpu_map__get_node_id(int cpu)
> >       return cpu__get_node(cpu);
> >  }
> >
> > -struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data)
> >  {
> > -     int cpu;
> > -     struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > -
> > -     if (idx > map->nr)
> > -             return id;
> > -
> > -     cpu = map->map[idx];
> > -
> > -     cpu = cpu_map__get_core_id(cpu);
> > +     struct aggr_cpu_id id;
> > +     int core = cpu_map__get_core_id(cpu);
> >
> >       /* cpu_map__get_die returns a struct with socket and die set*/
> > -     id = cpu_map__get_die(map, idx, data);
> > +     id = cpu_map__get_die_aggr_by_cpu(cpu, data);
> >       if (cpu_map__aggr_cpu_id_is_empty(id))
> >               return id;
> >
> > @@ -260,19 +259,33 @@ struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da
> >        * core_id is relative to socket and die, we need a global id.
> >        * So we combine the result from cpu_map__get_die with the core id
> >        */
> > -     id.core = cpu;
> > +     id.core = core;
> >       return id;
> > +
> >  }
> >
> > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused)
> > +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> > +{
> > +     if (idx < 0 || idx > map->nr)
> > +             return cpu_map__empty_aggr_cpu_id();
>
> Ditto
>
> > +
> > +     return cpu_map__get_core_aggr_by_cpu(map->map[idx], data);
> > +}
> > +
> > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused)
> >  {
> >       struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> >
> > +     id.node = cpu_map__get_node_id(cpu);
> > +     return id;
> > +}
> > +
> > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data)
> > +{
> >       if (idx < 0 || idx >= map->nr)
> > -             return id;
> > +             return cpu_map__empty_aggr_cpu_id();
> >
> > -     id.node = cpu_map__get_node_id(map->map[idx]);
> > -     return id;
> > +     return cpu_map__get_node_aggr_by_cpu(map->map[idx], data);
> >  }
> >
> >  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp)
> > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> > index a27eeaf086e8..c62d67704425 100644
> > --- a/tools/perf/util/cpumap.h
> > +++ b/tools/perf/util/cpumap.h
> > @@ -31,13 +31,17 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> >  int cpu_map__get_socket_id(int cpu);
> > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data);
> >  struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__get_die_id(int cpu);
> > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data);
> >  struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__get_core_id(int cpu);
> > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data);
> >  struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__get_node_id(int cpu);
> > -struct aggr_cpu_id  cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data);
> > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp);
> >  int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep);
> >  int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep);
> > --
> > 2.34.1.448.ga2b2bfdf31-goog
>
> --
>
> - Arnaldo
Arnaldo Carvalho de Melo Jan. 11, 2022, 7:49 p.m. UTC | #10 
Em Tue, Jan 11, 2022 at 04:33:01PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jan 04, 2022 at 10:13:05PM -0800, Ian Rogers escreveu:
> > The cpu_map and index can get confused. Add variants of the cpu_map__get
> > routines that are passed a cpu. Make the existing cpu_map__get routines
> > use the new functions with a view to remove them when no longer used.
> > 
> > Reviewed-by: James Clark <james.clark@arm.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++-----------------
> >  tools/perf/util/cpumap.h |  6 ++-
> >  2 files changed, 51 insertions(+), 34 deletions(-)
> > 
> > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > index 87d3eca9b872..49fba2c53822 100644
> > --- a/tools/perf/util/cpumap.c
> > +++ b/tools/perf/util/cpumap.c
> > @@ -128,21 +128,23 @@ int cpu_map__get_socket_id(int cpu)
> >  	return ret ?: value;
> >  }
> >  
> > -struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
> > -					void *data __maybe_unused)
> > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused)
> >  {
> > -	int cpu;
> >  	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> >  
> > -	if (idx > map->nr)
> > -		return id;
> > -
> > -	cpu = map->map[idx];
> > -
> >  	id.socket = cpu_map__get_socket_id(cpu);
> >  	return id;
> >  }
> >  
> > +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 'idx < 0' wasn't in the original code nor is described in the
> comment log message, please avoid doing this, this may be harmless or
> even a good hardening, but either way would be interesting to have it in
> a separate patch. This eases review as in the end this code is just a
> refactoring, moving things around but in the end should be equivalent code.
> 
> There are a few more, please consider this and if you agree, to speed
> things up I can make the changes here, if I think this won't fallout in
> changes to subsequent patches touching this area.

Nah, should be harmless, and its too much work already, so lets leave it
as-is, but please consider this in the future.

- Arnaldo
 
> - Arnaldo
> 
> >  static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer)
> >  {
> >  	struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer;
> > @@ -200,15 +202,10 @@ int cpu_map__get_die_id(int cpu)
> >  	return ret ?: value;
> >  }
> >  
> > -struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
> > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data)
> >  {
> > -	int cpu, die;
> > -	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > -
> > -	if (idx > map->nr)
> > -		return id;
> > -
> > -	cpu = map->map[idx];
> > +	struct aggr_cpu_id id;
> > +	int die;
> >  
> >  	die = cpu_map__get_die_id(cpu);
> >  	/* There is no die_id on legacy system. */
> > @@ -220,7 +217,7 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
> >  	 * with the socket ID and then add die to
> >  	 * make a unique ID.
> >  	 */
> > -	id = cpu_map__get_socket(map, idx, data);
> > +	id = cpu_map__get_socket_aggr_by_cpu(cpu, data);
> >  	if (cpu_map__aggr_cpu_id_is_empty(id))
> >  		return id;
> >  
> > @@ -228,6 +225,15 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
> >  	return id;
> >  }
> >  
> > +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx,
> > +				    void *data)
> > +{
> > +	if (idx < 0 || idx > map->nr)
> > +		return cpu_map__empty_aggr_cpu_id();
> 
> Ditto
> 
> > +
> > +	return cpu_map__get_die_aggr_by_cpu(map->map[idx], data);
> > +}
> > +
> >  int cpu_map__get_core_id(int cpu)
> >  {
> >  	int value, ret = cpu__get_topology_int(cpu, "core_id", &value);
> > @@ -239,20 +245,13 @@ int cpu_map__get_node_id(int cpu)
> >  	return cpu__get_node(cpu);
> >  }
> >  
> > -struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data)
> >  {
> > -	int cpu;
> > -	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > -
> > -	if (idx > map->nr)
> > -		return id;
> > -
> > -	cpu = map->map[idx];
> > -
> > -	cpu = cpu_map__get_core_id(cpu);
> > +	struct aggr_cpu_id id;
> > +	int core = cpu_map__get_core_id(cpu);
> >  
> >  	/* cpu_map__get_die returns a struct with socket and die set*/
> > -	id = cpu_map__get_die(map, idx, data);
> > +	id = cpu_map__get_die_aggr_by_cpu(cpu, data);
> >  	if (cpu_map__aggr_cpu_id_is_empty(id))
> >  		return id;
> >  
> > @@ -260,19 +259,33 @@ struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da
> >  	 * core_id is relative to socket and die, we need a global id.
> >  	 * So we combine the result from cpu_map__get_die with the core id
> >  	 */
> > -	id.core = cpu;
> > +	id.core = core;
> >  	return id;
> > +
> >  }
> >  
> > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused)
> > +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> > +{
> > +	if (idx < 0 || idx > map->nr)
> > +		return cpu_map__empty_aggr_cpu_id();
> 
> Ditto
> 
> > +
> > +	return cpu_map__get_core_aggr_by_cpu(map->map[idx], data);
> > +}
> > +
> > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused)
> >  {
> >  	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> >  
> > +	id.node = cpu_map__get_node_id(cpu);
> > +	return id;
> > +}
> > +
> > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data)
> > +{
> >  	if (idx < 0 || idx >= map->nr)
> > -		return id;
> > +		return cpu_map__empty_aggr_cpu_id();
> >  
> > -	id.node = cpu_map__get_node_id(map->map[idx]);
> > -	return id;
> > +	return cpu_map__get_node_aggr_by_cpu(map->map[idx], data);
> >  }
> >  
> >  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp)
> > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> > index a27eeaf086e8..c62d67704425 100644
> > --- a/tools/perf/util/cpumap.h
> > +++ b/tools/perf/util/cpumap.h
> > @@ -31,13 +31,17 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> >  int cpu_map__get_socket_id(int cpu);
> > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data);
> >  struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__get_die_id(int cpu);
> > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data);
> >  struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__get_core_id(int cpu);
> > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data);
> >  struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__get_node_id(int cpu);
> > -struct aggr_cpu_id  cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data);
> > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> >  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp);
> >  int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep);
> >  int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep);
> > -- 
> > 2.34.1.448.ga2b2bfdf31-goog
> 
> -- 
> 
> - Arnaldo
Arnaldo Carvalho de Melo Jan. 11, 2022, 7:56 p.m. UTC | #11 
Em Tue, Jan 11, 2022 at 11:36:02AM -0800, Ian Rogers escreveu:
> On Tue, Jan 11, 2022 at 11:33 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Tue, Jan 04, 2022 at 10:13:05PM -0800, Ian Rogers escreveu:
> > > The cpu_map and index can get confused. Add variants of the cpu_map__get
> > > routines that are passed a cpu. Make the existing cpu_map__get routines
> > > use the new functions with a view to remove them when no longer used.
> > >
> > > Reviewed-by: James Clark <james.clark@arm.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++-----------------
> > >  tools/perf/util/cpumap.h |  6 ++-
> > >  2 files changed, 51 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > > index 87d3eca9b872..49fba2c53822 100644
> > > --- a/tools/perf/util/cpumap.c
> > > +++ b/tools/perf/util/cpumap.c
> > > @@ -128,21 +128,23 @@ int cpu_map__get_socket_id(int cpu)
> > >       return ret ?: value;
> > >  }
> > >
> > > -struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
> > > -                                     void *data __maybe_unused)
> > > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused)
> > >  {
> > > -     int cpu;
> > >       struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > >
> > > -     if (idx > map->nr)
> > > -             return id;
> > > -
> > > -     cpu = map->map[idx];
> > > -
> > >       id.socket = cpu_map__get_socket_id(cpu);
> > >       return id;
> > >  }
> > >
> > > +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 'idx < 0' wasn't in the original code nor is described in the
> > comment log message, please avoid doing this, this may be harmless or
> > even a good hardening, but either way would be interesting to have it in
> > a separate patch. This eases review as in the end this code is just a
> > refactoring, moving things around but in the end should be equivalent code.
> >
> > There are a few more, please consider this and if you agree, to speed
> > things up I can make the changes here, if I think this won't fallout in
> > changes to subsequent patches touching this area.
> >
> > - Arnaldo
> 
> Fwiw, there's the same issue in cpu_map__get_die that's also in
> cpu_map__get_core, but weirdly not copied into cpu_map__get_node. As
> these functions are removed later I think doing nothing is best here.

Sure.
 
> Thanks,
> Ian
> 
> > >  static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer)
> > >  {
> > >       struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer;
> > > @@ -200,15 +202,10 @@ int cpu_map__get_die_id(int cpu)
> > >       return ret ?: value;
> > >  }
> > >
> > > -struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
> > > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data)
> > >  {
> > > -     int cpu, die;
> > > -     struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > > -
> > > -     if (idx > map->nr)
> > > -             return id;
> > > -
> > > -     cpu = map->map[idx];
> > > +     struct aggr_cpu_id id;
> > > +     int die;
> > >
> > >       die = cpu_map__get_die_id(cpu);
> > >       /* There is no die_id on legacy system. */
> > > @@ -220,7 +217,7 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
> > >        * with the socket ID and then add die to
> > >        * make a unique ID.
> > >        */
> > > -     id = cpu_map__get_socket(map, idx, data);
> > > +     id = cpu_map__get_socket_aggr_by_cpu(cpu, data);
> > >       if (cpu_map__aggr_cpu_id_is_empty(id))
> > >               return id;
> > >
> > > @@ -228,6 +225,15 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
> > >       return id;
> > >  }
> > >
> > > +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx,
> > > +                                 void *data)
> > > +{
> > > +     if (idx < 0 || idx > map->nr)
> > > +             return cpu_map__empty_aggr_cpu_id();
> >
> > Ditto
> >
> > > +
> > > +     return cpu_map__get_die_aggr_by_cpu(map->map[idx], data);
> > > +}
> > > +
> > >  int cpu_map__get_core_id(int cpu)
> > >  {
> > >       int value, ret = cpu__get_topology_int(cpu, "core_id", &value);
> > > @@ -239,20 +245,13 @@ int cpu_map__get_node_id(int cpu)
> > >       return cpu__get_node(cpu);
> > >  }
> > >
> > > -struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> > > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data)
> > >  {
> > > -     int cpu;
> > > -     struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > > -
> > > -     if (idx > map->nr)
> > > -             return id;
> > > -
> > > -     cpu = map->map[idx];
> > > -
> > > -     cpu = cpu_map__get_core_id(cpu);
> > > +     struct aggr_cpu_id id;
> > > +     int core = cpu_map__get_core_id(cpu);
> > >
> > >       /* cpu_map__get_die returns a struct with socket and die set*/
> > > -     id = cpu_map__get_die(map, idx, data);
> > > +     id = cpu_map__get_die_aggr_by_cpu(cpu, data);
> > >       if (cpu_map__aggr_cpu_id_is_empty(id))
> > >               return id;
> > >
> > > @@ -260,19 +259,33 @@ struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da
> > >        * core_id is relative to socket and die, we need a global id.
> > >        * So we combine the result from cpu_map__get_die with the core id
> > >        */
> > > -     id.core = cpu;
> > > +     id.core = core;
> > >       return id;
> > > +
> > >  }
> > >
> > > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused)
> > > +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> > > +{
> > > +     if (idx < 0 || idx > map->nr)
> > > +             return cpu_map__empty_aggr_cpu_id();
> >
> > Ditto
> >
> > > +
> > > +     return cpu_map__get_core_aggr_by_cpu(map->map[idx], data);
> > > +}
> > > +
> > > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused)
> > >  {
> > >       struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
> > >
> > > +     id.node = cpu_map__get_node_id(cpu);
> > > +     return id;
> > > +}
> > > +
> > > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data)
> > > +{
> > >       if (idx < 0 || idx >= map->nr)
> > > -             return id;
> > > +             return cpu_map__empty_aggr_cpu_id();
> > >
> > > -     id.node = cpu_map__get_node_id(map->map[idx]);
> > > -     return id;
> > > +     return cpu_map__get_node_aggr_by_cpu(map->map[idx], data);
> > >  }
> > >
> > >  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp)
> > > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> > > index a27eeaf086e8..c62d67704425 100644
> > > --- a/tools/perf/util/cpumap.h
> > > +++ b/tools/perf/util/cpumap.h
> > > @@ -31,13 +31,17 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
> > >  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
> > >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> > >  int cpu_map__get_socket_id(int cpu);
> > > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data);
> > >  struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data);
> > >  int cpu_map__get_die_id(int cpu);
> > > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data);
> > >  struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data);
> > >  int cpu_map__get_core_id(int cpu);
> > > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data);
> > >  struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data);
> > >  int cpu_map__get_node_id(int cpu);
> > > -struct aggr_cpu_id  cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> > > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data);
> > > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
> > >  int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp);
> > >  int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep);
> > >  int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep);
> > > --
> > > 2.34.1.448.ga2b2bfdf31-goog
> >
> > --
> >
> > - Arnaldo
diff mbox series

Patch

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 87d3eca9b872..49fba2c53822 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -128,21 +128,23 @@  int cpu_map__get_socket_id(int cpu)
 	return ret ?: value;
 }
 
-struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
-					void *data __maybe_unused)
+struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused)
 {
-	int cpu;
 	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
 
-	if (idx > map->nr)
-		return id;
-
-	cpu = map->map[idx];
-
 	id.socket = cpu_map__get_socket_id(cpu);
 	return id;
 }
 
+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);
+}
+
 static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer)
 {
 	struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer;
@@ -200,15 +202,10 @@  int cpu_map__get_die_id(int cpu)
 	return ret ?: value;
 }
 
-struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
+struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data)
 {
-	int cpu, die;
-	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
-
-	if (idx > map->nr)
-		return id;
-
-	cpu = map->map[idx];
+	struct aggr_cpu_id id;
+	int die;
 
 	die = cpu_map__get_die_id(cpu);
 	/* There is no die_id on legacy system. */
@@ -220,7 +217,7 @@  struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
 	 * with the socket ID and then add die to
 	 * make a unique ID.
 	 */
-	id = cpu_map__get_socket(map, idx, data);
+	id = cpu_map__get_socket_aggr_by_cpu(cpu, data);
 	if (cpu_map__aggr_cpu_id_is_empty(id))
 		return id;
 
@@ -228,6 +225,15 @@  struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat
 	return id;
 }
 
+struct aggr_cpu_id cpu_map__get_die(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_die_aggr_by_cpu(map->map[idx], data);
+}
+
 int cpu_map__get_core_id(int cpu)
 {
 	int value, ret = cpu__get_topology_int(cpu, "core_id", &value);
@@ -239,20 +245,13 @@  int cpu_map__get_node_id(int cpu)
 	return cpu__get_node(cpu);
 }
 
-struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
+struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data)
 {
-	int cpu;
-	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
-
-	if (idx > map->nr)
-		return id;
-
-	cpu = map->map[idx];
-
-	cpu = cpu_map__get_core_id(cpu);
+	struct aggr_cpu_id id;
+	int core = cpu_map__get_core_id(cpu);
 
 	/* cpu_map__get_die returns a struct with socket and die set*/
-	id = cpu_map__get_die(map, idx, data);
+	id = cpu_map__get_die_aggr_by_cpu(cpu, data);
 	if (cpu_map__aggr_cpu_id_is_empty(id))
 		return id;
 
@@ -260,19 +259,33 @@  struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da
 	 * core_id is relative to socket and die, we need a global id.
 	 * So we combine the result from cpu_map__get_die with the core id
 	 */
-	id.core = cpu;
+	id.core = core;
 	return id;
+
 }
 
-struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused)
+struct aggr_cpu_id cpu_map__get_core(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_core_aggr_by_cpu(map->map[idx], data);
+}
+
+struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused)
 {
 	struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id();
 
+	id.node = cpu_map__get_node_id(cpu);
+	return id;
+}
+
+struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data)
+{
 	if (idx < 0 || idx >= map->nr)
-		return id;
+		return cpu_map__empty_aggr_cpu_id();
 
-	id.node = cpu_map__get_node_id(map->map[idx]);
-	return id;
+	return cpu_map__get_node_aggr_by_cpu(map->map[idx], data);
 }
 
 int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp)
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index a27eeaf086e8..c62d67704425 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -31,13 +31,17 @@  size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
 size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
 size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
 int cpu_map__get_socket_id(int cpu);
+struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data);
 struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data);
 int cpu_map__get_die_id(int cpu);
+struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data);
 struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data);
 int cpu_map__get_core_id(int cpu);
+struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data);
 struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data);
 int cpu_map__get_node_id(int cpu);
-struct aggr_cpu_id  cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
+struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data);
+struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data);
 int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp);
 int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep);
 int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep);