[PATCH] libperf: Add back guard on MAX_NR_CPUS

[PATCH] libperf: Add back guard on MAX_NR_CPUS

[Christophe Leroy]

Building perf with EXTRA_CFLAGS="-DMAX_NR_CPUS=1" fails:

	  CC      /home/chleroy/linux-powerpc/tools/perf/libperf/cpumap.o
	cpumap.c:16: error: "MAX_NR_CPUS" redefined [-Werror]
	   16 | #define MAX_NR_CPUS 4096
	      |
	<command-line>: note: this is the location of the previous definition

Commit e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
moved definition of MAX_NR_CPUS from lib/perf/include/internal/cpumap.h
to lib/perf/cpumap.c but the guard surrounding that definition got lost
in the move.

See commit 21b8732eb447 ("perf tools: Allow overriding MAX_NR_CPUS at
compile time") to see why it is needed.

Note that MAX_NR_CPUS was initialy defined in perf/perf.h and a
redundant definition was added by commit 9c3516d1b850 ("libperf:
Add perf_cpu_map__new()/perf_cpu_map__read() functions").

A cleaner fix would be to remove that duplicate but for the time
being fix the problem by bringing back the guard for when MAX_NR_CPUS
is already defined.

Fixes: e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/perf/cpumap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
index fcc47214062a..12eaa6955121 100644
--- a/tools/lib/perf/cpumap.c
+++ b/tools/lib/perf/cpumap.c
@@ -13,7 +13,9 @@
 #include "internal.h"
 #include <api/fs/fs.h>
 
+#ifndef MAX_NR_CPUS
 #define MAX_NR_CPUS 4096
+#endif
 
 void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus)
 {
-- 
2.47.0

[PATCH] perf: Fix display of kernel symbols

[Christophe Leroy]

Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
sorted array for addresses"), perf doesn't display anymore kernel
symbols on powerpc, allthough it still detects them as kernel addresses.

	# Overhead  Command     Shared Object  Symbol
	# ........  ..........  ............. ......................................
	#
	    80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
	     3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
	     1.72%  Coeur main  [unknown]      [k] 0xc005f11c
	     1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
	     0.44%  Coeur main  libc.so.6      [.] epoll_wait
	     0.38%  Coeur main  [unknown]      [k] 0xc0011718
	     0.36%  Coeur main  [unknown]      [k] 0xc01f45c0

This is because function maps__find_next_entry() now returns current
entry instead of next entry, leading to kernel map end address
getting mis-configured with its own start address instead of the
start address of the following map.

Fix it by really taking the next entry.

Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/maps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 432399cbe5dd..d39bf27a5fdd 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -1137,7 +1137,7 @@ struct map *maps__find_next_entry(struct maps *maps, struct map *map)
 
 	down_read(maps__lock(maps));
 	i = maps__by_address_index(maps, map);
-	if (i < maps__nr_maps(maps))
+	if (++i < maps__nr_maps(maps))
 		result = map__get(maps__maps_by_address(maps)[i]);
 
 	up_read(maps__lock(maps));
-- 
2.47.0

Re: [PATCH] libperf: Add back guard on MAX_NR_CPUS

[Ian Rogers]

On Mon, Jan 6, 2025 at 11:38 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> Building perf with EXTRA_CFLAGS="-DMAX_NR_CPUS=1" fails:
>
>           CC      /home/chleroy/linux-powerpc/tools/perf/libperf/cpumap.o
>         cpumap.c:16: error: "MAX_NR_CPUS" redefined [-Werror]
>            16 | #define MAX_NR_CPUS 4096
>               |
>         <command-line>: note: this is the location of the previous definition
>
> Commit e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
> moved definition of MAX_NR_CPUS from lib/perf/include/internal/cpumap.h
> to lib/perf/cpumap.c but the guard surrounding that definition got lost
> in the move.
>
> See commit 21b8732eb447 ("perf tools: Allow overriding MAX_NR_CPUS at
> compile time") to see why it is needed.
>
> Note that MAX_NR_CPUS was initialy defined in perf/perf.h and a
> redundant definition was added by commit 9c3516d1b850 ("libperf:
> Add perf_cpu_map__new()/perf_cpu_map__read() functions").
>
> A cleaner fix would be to remove that duplicate but for the time
> being fix the problem by bringing back the guard for when MAX_NR_CPUS
> is already defined.
>
> Fixes: e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Hello,

I believe this change might be unnecessary. The only use of
MAX_NR_CPUS is in a warning message within perf_cpu_map__new, which
takes a string and produces a perf_cpu_map. Other similar functions
like cpu_map__new_sysconf don't check MAX_NR_CPUS. Therefore,
specifying a -DMAX_NR_CPUS value on the build command line has little
effect—it only impacts a warning message for certain kinds of
perf_cpu_map creation. It's also unclear what the intended outcome is
on the build command line.

Given that specifying the value doesn't seem to have a clear purpose,
allowing the build to break might be the best option. This would alert
the person building perf that they are doing something that doesn't
make sense.

Thanks,
Ian

Re: [PATCH] perf: Fix display of kernel symbols

[Ian Rogers]

On Mon, Jan 6, 2025 at 11:38 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
> sorted array for addresses"), perf doesn't display anymore kernel
> symbols on powerpc, allthough it still detects them as kernel addresses.
>
>         # Overhead  Command     Shared Object  Symbol
>         # ........  ..........  ............. ......................................
>         #
>             80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
>              3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
>              1.72%  Coeur main  [unknown]      [k] 0xc005f11c
>              1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
>              0.44%  Coeur main  libc.so.6      [.] epoll_wait
>              0.38%  Coeur main  [unknown]      [k] 0xc0011718
>              0.36%  Coeur main  [unknown]      [k] 0xc01f45c0
>
> This is because function maps__find_next_entry() now returns current
> entry instead of next entry, leading to kernel map end address
> getting mis-configured with its own start address instead of the
> start address of the following map.
>
> Fix it by really taking the next entry.
>
> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/maps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 432399cbe5dd..d39bf27a5fdd 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -1137,7 +1137,7 @@ struct map *maps__find_next_entry(struct maps *maps, struct map *map)
>
>         down_read(maps__lock(maps));
>         i = maps__by_address_index(maps, map);
> -       if (i < maps__nr_maps(maps))
> +       if (++i < maps__nr_maps(maps))
>                 result = map__get(maps__maps_by_address(maps)[i]);

Thanks for diagnosing this and sorry for the bug! Using the next entry
in this way won't work if the entries aren't sorted. I think the code
needs to be a little more complex, something like:
```
while (1) {
    down_read(maps__lock(maps));
    if (!maps__maps_by_address_sorted(maps)) {
        up_read(maps__lock(maps));
        maps__sort_by_address(maps);
        continue;
    }
    i = maps__by_address_index(maps, map) + 1;
    if (i < maps__nr_maps(maps))
        result = map__get(maps__maps_by_address(maps)[i]);
    up_read(maps__lock(maps));
    break;
}
```
We could also implement the code similar to maps__by_address_index but
with some kind of best next value in the unsorted case. Given the
function has a single caller then this is probably overkill, but we've
seen performance issues in this code before.

Thanks,
Ian

Re: [PATCH] libperf: Add back guard on MAX_NR_CPUS

[Christophe Leroy]

Le 06/01/2025 à 21:05, Ian Rogers a écrit :
> On Mon, Jan 6, 2025 at 11:38 AM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> Building perf with EXTRA_CFLAGS="-DMAX_NR_CPUS=1" fails:
>>
>>            CC      /home/chleroy/linux-powerpc/tools/perf/libperf/cpumap.o
>>          cpumap.c:16: error: "MAX_NR_CPUS" redefined [-Werror]
>>             16 | #define MAX_NR_CPUS 4096
>>                |
>>          <command-line>: note: this is the location of the previous definition
>>
>> Commit e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
>> moved definition of MAX_NR_CPUS from lib/perf/include/internal/cpumap.h
>> to lib/perf/cpumap.c but the guard surrounding that definition got lost
>> in the move.
>>
>> See commit 21b8732eb447 ("perf tools: Allow overriding MAX_NR_CPUS at
>> compile time") to see why it is needed.
>>
>> Note that MAX_NR_CPUS was initialy defined in perf/perf.h and a
>> redundant definition was added by commit 9c3516d1b850 ("libperf:
>> Add perf_cpu_map__new()/perf_cpu_map__read() functions").
>>
>> A cleaner fix would be to remove that duplicate but for the time
>> being fix the problem by bringing back the guard for when MAX_NR_CPUS
>> is already defined.
>>
>> Fixes: e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Hello,
> 
> I believe this change might be unnecessary. The only use of
> MAX_NR_CPUS is in a warning message within perf_cpu_map__new, which
> takes a string and produces a perf_cpu_map. Other similar functions
> like cpu_map__new_sysconf don't check MAX_NR_CPUS. Therefore,
> specifying a -DMAX_NR_CPUS value on the build command line has little
> effect—it only impacts a warning message for certain kinds of
> perf_cpu_map creation. It's also unclear what the intended outcome is
> on the build command line.
> 
> Given that specifying the value doesn't seem to have a clear purpose,
> allowing the build to break might be the best option. This would alert
> the person building perf that they are doing something that doesn't
> make sense.
> 

Ok so I looked at it once more and indeed it looks like it has changed 
since 2017. See commit 21b8732eb447 ("perf tools: Allow overriding 
MAX_NR_CPUS at compile time") to understand why it was required at that 
time.

Now I don't have much size difference anymore between a build with 
MAX_NR_CPUS=1 and the default MAX_NR_CPUS=4096:

$ size perf perf-1cpu
    text	   data	    bss	    dec	    hex	filename
3415908	 104164	  17148	3537220	 35f944	perf
3415904	 104164	  16124	3536192	 35f540	perf-1cpu

Apparently that was changed by commit 6a1e2c5c2673 ("perf stat: Remove a 
set of shadow stats static variables")

So I agree with you, it is apparently not worth reducing MAX_NR_CPUS 
anymore, I'll give it a try.

Thanks
Christophe

Re: [PATCH] libperf: Add back guard on MAX_NR_CPUS

[Ian Rogers]

On Tue, Jan 7, 2025 at 12:50 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> Le 06/01/2025 à 21:05, Ian Rogers a écrit :
> > On Mon, Jan 6, 2025 at 11:38 AM Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> >>
> >> Building perf with EXTRA_CFLAGS="-DMAX_NR_CPUS=1" fails:
> >>
> >>            CC      /home/chleroy/linux-powerpc/tools/perf/libperf/cpumap.o
> >>          cpumap.c:16: error: "MAX_NR_CPUS" redefined [-Werror]
> >>             16 | #define MAX_NR_CPUS 4096
> >>                |
> >>          <command-line>: note: this is the location of the previous definition
> >>
> >> Commit e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
> >> moved definition of MAX_NR_CPUS from lib/perf/include/internal/cpumap.h
> >> to lib/perf/cpumap.c but the guard surrounding that definition got lost
> >> in the move.
> >>
> >> See commit 21b8732eb447 ("perf tools: Allow overriding MAX_NR_CPUS at
> >> compile time") to see why it is needed.
> >>
> >> Note that MAX_NR_CPUS was initialy defined in perf/perf.h and a
> >> redundant definition was added by commit 9c3516d1b850 ("libperf:
> >> Add perf_cpu_map__new()/perf_cpu_map__read() functions").
> >>
> >> A cleaner fix would be to remove that duplicate but for the time
> >> being fix the problem by bringing back the guard for when MAX_NR_CPUS
> >> is already defined.
> >>
> >> Fixes: e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > Hello,
> >
> > I believe this change might be unnecessary. The only use of
> > MAX_NR_CPUS is in a warning message within perf_cpu_map__new, which
> > takes a string and produces a perf_cpu_map. Other similar functions
> > like cpu_map__new_sysconf don't check MAX_NR_CPUS. Therefore,
> > specifying a -DMAX_NR_CPUS value on the build command line has little
> > effect—it only impacts a warning message for certain kinds of
> > perf_cpu_map creation. It's also unclear what the intended outcome is
> > on the build command line.
> >
> > Given that specifying the value doesn't seem to have a clear purpose,
> > allowing the build to break might be the best option. This would alert
> > the person building perf that they are doing something that doesn't
> > make sense.
> >
>
> Ok so I looked at it once more and indeed it looks like it has changed
> since 2017. See commit 21b8732eb447 ("perf tools: Allow overriding
> MAX_NR_CPUS at compile time") to understand why it was required at that
> time.
>
> Now I don't have much size difference anymore between a build with
> MAX_NR_CPUS=1 and the default MAX_NR_CPUS=4096:
>
> $ size perf perf-1cpu
>     text           data     bss     dec     hex filename
> 3415908  104164   17148 3537220  35f944 perf
> 3415904  104164   16124 3536192  35f540 perf-1cpu
>
> Apparently that was changed by commit 6a1e2c5c2673 ("perf stat: Remove a
> set of shadow stats static variables")
>
> So I agree with you, it is apparently not worth reducing MAX_NR_CPUS
> anymore, I'll give it a try.

Thanks for figuring this out and the explanation. Fwiw, the shadow
stats should be gone. Perf obviously reads counters, as there may be
counters on multiple CPUs then it aggregates these counts together.
The shadow stats were another aggregation mechanism used for metrics
and being a simple person I managed to land just having the single
aggregation approach although there's some cleanup in this area hiding
as tech debt in perf script. The support was removed around 12 minor
versions ago, iirc.

As a heads up given the interest in binary size, most of perf's binary
size comes from the json events. Off the top of my head I'd say it is
like 70% on x86. We avoid string relocations on those by storing
constant offsets and at some point I'd like to get compression going -
although I'd like to compress across files, something 7-zip's format
at least attempts but there was recent 7-zip security news which may
mean a dependency is a bad thing. Anyway, most of the json events
likely aren't needed by the platform you are building for and I added
some build support for only including the ones you are. As you may not
know your CPU's model name I added a mechanism to use CPUIDs:
https://lore.kernel.org/r/20240904044351.712080-1-irogers@google.com
But perhaps we can use x86-64 version levels to make this a more
automated approach, if we see say a "-march=x86-64-v4" in the CFLAGS -
the idea being that say nehalem doesn't support x86-64-v4 so we can
safely drop the nehalem json data. That said x86-64 version levels
have been described as, "some crazy glibc artifact and is stupid and
needs to die":
https://lore.kernel.org/lkml/CAHk-=wh_b8b1qZF8_obMKpF+xfYnPZ6t38F1+5pK-eXNyCdJ7g@mail.gmail.com/
For metrics I have a similar problem, knowing which models support
which events, but to handle that I use the json and see if it has an
event, if not the metric isn't created. This causes issues when new
models rename events. Anyway, those patches haven't landed, here are
the Intel versions:
https://lore.kernel.org/lkml/20240926175035.408668-1-irogers@google.com/

Thanks,
Ian