Re: [PATCH v3 9/9] perf record: Make --buildid-mmap the default

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Apr 28, 2025 at 02:34:08PM -0700, Ian Rogers wrote:
> Support for build IDs in mmap2 perf events has been present since
> Linux v5.12:
> https://lore.kernel.org/lkml/20210219194619.1780437-1-acme@kernel.org/
> Build ID mmap events don't avoid the need to inject build IDs for DSO
> touched by samples as the build ID cache is populated by perf
> record. They can avoid some cases of symbol mis-resolution caused by
> the file system changing from when a sample occurred and when the DSO
> is sought. To disable build ID scanning
> 
> Unlike the --buildid-mmap option, this doesn't disable the build ID
> cache but it does disable the processing of samples looking for DSOs
> to inject build IDs for. To disable the build ID cache the -B
> (--no-buildid) option should be used.
> 
> Making this option the default was raised on the list in:
> https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-record.c        | 33 +++++++++++++++++++-----------
>  tools/perf/util/symbol_conf.h      |  2 +-
>  tools/perf/util/synthetic-events.c | 16 +++++++--------
>  3 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ba20bf7c011d..7b64013ba8c0 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -169,6 +169,7 @@ struct record {
>  	bool			no_buildid_cache_set;
>  	bool			buildid_all;
>  	bool			buildid_mmap;
> +	bool			buildid_mmap_set;
>  	bool			timestamp_filename;
>  	bool			timestamp_boundary;
>  	bool			off_cpu;
> @@ -1795,6 +1796,7 @@ record__finish_output(struct record *rec)
>  			data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
>  	}
>  
> +	/* Buildid scanning disabled or build ID in kernel and synthesized map events. */
>  	if (!rec->no_buildid) {
>  		process_buildids(rec);
>  
> @@ -2966,6 +2968,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
>  			rec->no_buildid = true;
>  		else if (!strcmp(value, "mmap"))
>  			rec->buildid_mmap = true;
> +		else if (!strcmp(value, "no-mmap"))
> +			rec->buildid_mmap = false;
>  		else
>  			return -1;
>  		return 0;
> @@ -3349,6 +3353,7 @@ static struct record record = {
>  		.ctl_fd_ack          = -1,
>  		.synth               = PERF_SYNTH_ALL,
>  	},
> +	.buildid_mmap = true,
>  };
>  
>  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> @@ -3514,8 +3519,8 @@ static struct option __record_options[] = {
>  		   "file", "vmlinux pathname"),
>  	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
>  		    "Record build-id of all DSOs regardless of hits"),
> -	OPT_BOOLEAN(0, "buildid-mmap", &record.buildid_mmap,
> -		    "Record build-id in map events"),
> +	OPT_BOOLEAN_SET(0, "buildid-mmap", &record.buildid_mmap, &record.buildid_mmap_set,
> +			"Legacy record build-id in map events option which is now the default. Behaves indentically to --no-buildid. Disable with --no-buildid-mmap"),
>  	OPT_BOOLEAN(0, "timestamp-filename", &record.timestamp_filename,
>  		    "append timestamp to output filename"),
>  	OPT_BOOLEAN(0, "timestamp-boundary", &record.timestamp_boundary,
> @@ -4042,19 +4047,23 @@ int cmd_record(int argc, const char **argv)
>  		record.opts.record_switch_events = true;
>  	}
>  
> +	if (!rec->buildid_mmap) {
> +		pr_debug("Disabling build id in synthesized mmap2 events.\n");
> +		symbol_conf.no_buildid_mmap2 = true;
> +	} else if (rec->buildid_mmap_set) {
> +		/*
> +		 * Explicitly passing --buildid-mmap disables buildid processing
> +		 * and cache generation.
> +		 */
> +		rec->no_buildid = true;
> +	}
> +	if (rec->buildid_mmap && !perf_can_record_build_id()) {
> +		pr_warning("Missing support for build id in kernel mmap events. Disable this warning with --no-buildid-mmap\n");
> +		rec->buildid_mmap = false;
> +	}
>  	if (rec->buildid_mmap) {
> -		if (!perf_can_record_build_id()) {
> -			pr_err("Failed: no support to record build id in mmap events, update your kernel.\n");
> -			err = -EINVAL;
> -			goto out_opts;
> -		}
> -		pr_debug("Enabling build id in mmap2 events.\n");
> -		/* Enable mmap build id synthesizing. */
> -		symbol_conf.buildid_mmap2 = true;
>  		/* Enable perf_event_attr::build_id bit. */
>  		rec->opts.build_id = true;
> -		/* Disable build id cache. */
> -		rec->no_buildid = true;
>  	}
>  
>  	if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> index cd9aa82c7d5a..7a80d2c14d9b 100644
> --- a/tools/perf/util/symbol_conf.h
> +++ b/tools/perf/util/symbol_conf.h
> @@ -43,7 +43,7 @@ struct symbol_conf {
>  			report_individual_block,
>  			inline_name,
>  			disable_add2line_warn,
> -			buildid_mmap2,
> +			no_buildid_mmap2,
>  			guest_code,
>  			lazy_load_kernel_maps,
>  			keep_exited_threads,
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 69b98023ce74..638d7dd7fa4b 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -532,7 +532,7 @@ int perf_event__synthesize_mmap_events(const struct perf_tool *tool,
>  		event->mmap2.pid = tgid;
>  		event->mmap2.tid = pid;
>  
> -		if (symbol_conf.buildid_mmap2)
> +		if (!symbol_conf.no_buildid_mmap2)

I find the double negation confusing.  Can we keep it positive?

Thanks,
Namhyung


>  			perf_record_mmap2__read_build_id(&event->mmap2, machine, false);
>  
>  		if (perf_tool__process_synth_event(tool, event, machine, process) != 0) {
> @@ -690,7 +690,7 @@ static int perf_event__synthesize_modules_maps_cb(struct map *map, void *data)
>  		return 0;
>  
>  	dso = map__dso(map);
> -	if (symbol_conf.buildid_mmap2) {
> +	if (!symbol_conf.no_buildid_mmap2) {
>  		size = PERF_ALIGN(dso__long_name_len(dso) + 1, sizeof(u64));
>  		event->mmap2.header.type = PERF_RECORD_MMAP2;
>  		event->mmap2.header.size = (sizeof(event->mmap2) -
> @@ -734,9 +734,9 @@ int perf_event__synthesize_modules(const struct perf_tool *tool, perf_event__han
>  		.process = process,
>  		.machine = machine,
>  	};
> -	size_t size = symbol_conf.buildid_mmap2
> -		? sizeof(args.event->mmap2)
> -		: sizeof(args.event->mmap);
> +	size_t size = symbol_conf.no_buildid_mmap2
> +		? sizeof(args.event->mmap)
> +		: sizeof(args.event->mmap2);
>  
>  	args.event = zalloc(size + machine->id_hdr_size);
>  	if (args.event == NULL) {
> @@ -1124,8 +1124,8 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
>  						struct machine *machine)
>  {
>  	union perf_event *event;
> -	size_t size = symbol_conf.buildid_mmap2 ?
> -			sizeof(event->mmap2) : sizeof(event->mmap);
> +	size_t size = symbol_conf.no_buildid_mmap2 ?
> +			sizeof(event->mmap) : sizeof(event->mmap2);
>  	struct map *map = machine__kernel_map(machine);
>  	struct kmap *kmap;
>  	int err;
> @@ -1159,7 +1159,7 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
>  		event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;
>  	}
>  
> -	if (symbol_conf.buildid_mmap2) {
> +	if (!symbol_conf.no_buildid_mmap2) {
>  		size = snprintf(event->mmap2.filename, sizeof(event->mmap2.filename),
>  				"%s%s", machine->mmap_name, kmap->ref_reloc_sym->name) + 1;
>  		size = PERF_ALIGN(size, sizeof(u64));
> -- 
> 2.49.0.901.g37484f566f-goog
>