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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Reviewed-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
> 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