| 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
| ||
|
Message-ID: <CAP-5=fUMiWu5gSfBSTq+OnTRQ2E2gCw6gi7dmOxeTD-+UyYtFw@mail.gmail.com>
Date: Wed, 28 May 2025 13:47:47 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...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>, Kan Liang <kan.liang@...ux.intel.com>,
Athira Rajeev <atrajeev@...ux.ibm.com>, Kajol Jain <kjain@...ux.ibm.com>,
Li Huafei <lihuafei1@...wei.com>, "Steinar H. Gunderson" <sesse@...gle.com>,
James Clark <james.clark@...aro.org>, Stephen Brennan <stephen.s.brennan@...cle.com>,
Andi Kleen <ak@...ux.intel.com>, Dmitry Vyukov <dvyukov@...gle.com>,
Zhongqiu Han <quic_zhonhan@...cinc.com>, Yicong Yang <yangyicong@...ilicon.com>,
Krzysztof Łopatowski <krzysztof.m.lopatowski@...il.com>,
"Dr. David Alan Gilbert" <linux@...blig.org>, Zixian Cai <fzczx123@...il.com>,
Steve Clevenger <scclevenger@...amperecomputing.com>,
Thomas Falcon <thomas.falcon@...el.com>, Martin Liska <martin.liska@....com>,
Martin Liška <m.liska@...link.cz>,
Song Liu <song@...nel.org>, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 9/9] perf record: Make --buildid-mmap the default
On Wed, May 28, 2025 at 1:38 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> 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@...gle.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?
Agreed double negation is broadly bad. Here I changed buildid_mmap2 to
no_buildid_mmap2 to convey that using build ID with mmap2 events was
the default and needed opting out of. Getting rid of the 'no_' is of
course possible, it will mean all symbol_confs will need to initialize
the value to true (which has minor binary size implications), I also
think it makes the intention of the code harder to understand.
Thanks,
Ian
> 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
> >
Powered by blists - more mailing lists