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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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