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

[Namhyung Kim <namhyung@kernel.org>]

On Wed, May 28, 2025 at 01:47:47PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 1:38 PM Namhyung Kim <namhyung@kernel.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@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?
> 
> 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.

Well I think perf record is the only place to set it.  But I won't
insist on it strongly.  Let's go with negative if you like.

Thanks,
Namhyung