linux-kernel - Re: [PATCH] libperf: Add back guard on MAX_NR

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]

Message-ID: <fa5dfd6d-3b8d-4086-9604-e5abddd324c9@csgroup.eu>
Date: Tue, 7 Jan 2025 09:31:41 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
 "Liang, Kan" <kan.liang@...ux.intel.com>, Leo Yan <leo.yan@....com>,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 linuxppc-dev@...ts.ozlabs.org, Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH] libperf: Add back guard on MAX_NR_CPUS



Le 06/01/2025 à 21:05, Ian Rogers a écrit :
> On Mon, Jan 6, 2025 at 11:38 AM Christophe Leroy
> <christophe.leroy@...roup.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@...roup.eu>
>> Reviewed-by: Arnaldo Carvalho de Melo <acme@...hat.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